Skip to content
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

Added DelegatingMetricData & TestMetricData Classes #6225

Draft
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

ThomasJazz
Copy link

Hello! This is my first attempt to contribute to an open source project, so pls be nice :). I have read through the contributor steps, but I apologize in advance if I have missed anything.

  • Changes should address issue: Add DelegatingMetricData #6195
  • I used DelegatingSpanData as a guide for this. I am not familiar enough with the OTEL codebase yet to know the full big picture, so if there's any code in there that doesn't make sense, that's why

Copy link

codecov bot commented Feb 15, 2024

Codecov Report

Attention: 35 lines in your changes are missing coverage. Please review.

Comparison is base (48d41d3) 91.05% compared to head (d75178a) 90.87%.

Files Patch % Lines
...lemetry/sdk/metrics/data/DelegatingMetricData.java 23.91% 26 Missing and 9 partials ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #6225      +/-   ##
============================================
- Coverage     91.05%   90.87%   -0.18%     
- Complexity     5687     5698      +11     
============================================
  Files           621      623       +2     
  Lines         16642    16699      +57     
  Branches       1703     1711       +8     
============================================
+ Hits          15153    15175      +22     
- Misses          998     1024      +26     
- Partials        491      500       +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@jack-berg jack-berg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this @ThomasJazz!

* A {@link MetricData} which delegates all methods to another {@link MetricData}. Extend this class
* to modify the {@link MetricData} that will be exported.
*/
public abstract class DelegatingMetricData implements MetricData {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may want to provide delegating version of the *PointData interfaces as well (or maybe just make the existing immutable implementations public!). This PR will allow you to easily override resource, scope, name, description, unit, but you still need to bring your own implementation of the Data and PointData interfaces.


class DelegatingMetricDataTest {

private static final class NoOpDelegatingMetricData extends DelegatingMetricData {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#nit: Can you move these static classes to the bottom of the file?

.setDescription("description")
.setUnit("1")
.setData(ImmutableSummaryData.empty())
.setType(MetricDataType.SUMMARY);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure setting data and type to arbitrary selections is better than omitting them.

Also, the value of this is limited without usable Data and PointData implementations.

@jkwatson WDYT about adding static create methods to all the interfaces in the io.opentelemetry.sdk.metrics.data? These could return the internal immutable implementations, without requiring that we make all those classes public.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants