-
Notifications
You must be signed in to change notification settings - Fork 891
6195: DelegatingMetricData. #7229
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
6195: DelegatingMetricData. #7229
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7229 +/- ##
============================================
- Coverage 89.98% 89.96% -0.02%
- Complexity 6685 6719 +34
============================================
Files 749 765 +16
Lines 20191 20268 +77
Branches 1977 1985 +8
============================================
+ Hits 18168 18234 +66
- Misses 1431 1439 +8
- Partials 592 595 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a handful of small comments. Looks pretty good!
sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/data/DelegatingMetricData.java
Outdated
Show resolved
Hide resolved
sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/data/DoublePointData.java
Outdated
Show resolved
Hide resolved
* | ||
* @return a ExponentialHistogramBuckets. | ||
*/ | ||
@SuppressWarnings("TooManyParameters") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like you lifted this from ImmutableExponentialHistogramBuckets.create
, but I don't think its needed in either place
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried resolving it but the build was failing with parameter overload error. I had to keep this in place to resolve it. Any suggestion on how I can resolve it?
sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/data/ExponentialHistogramBuckets.java
Outdated
Show resolved
Hide resolved
sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/data/GaugeData.java
Outdated
Show resolved
Hide resolved
sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/data/SumData.java
Outdated
Show resolved
Hide resolved
sdk/testing/src/main/java/io/opentelemetry/sdk/testing/metrics/TestMetricData.java
Outdated
Show resolved
Hide resolved
This PR also resolves #4389. |
Co-authored-by: jack-berg <[email protected]>
sdk/testing/src/main/java/io/opentelemetry/sdk/testing/metrics/TestMetricData.java
Outdated
Show resolved
Hide resolved
sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/data/DelegatingMetricDataTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a handful of minor comments!
sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/data/SummaryPointDataTest.java
Outdated
Show resolved
Hide resolved
sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/data/SummaryDataTest.java
Outdated
Show resolved
Hide resolved
sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/data/HistogramPointDataTest.java
Show resolved
Hide resolved
sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/data/DoubleExemplarDataTest.java
Outdated
Show resolved
Hide resolved
sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/data/ExponentialHistogramBuckets.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
This Change
Context
This PR tries to solve this ticket. I preserved the original PR that was initially raised to add this class but I added a couple other enhancements that were mentioned in the PR. @jack-berg proposed two approaches for exposing other
PointData
interfaces:I went with the second approach because it keeps the API simpler, consistent and avoids code duplication..
Implementation Example
Finally I am thinking people might be using this class with Metric Exporter as below: