Skip to content

Conversation

jack-berg
Copy link
Member

Related to #7636, #7503.

ExemplarReservoir currently has the signature: ExemplarReservoir<T extends ExemplarData>, where T is either LongExemplarData or DoubleExemplarData.

This parameter is used in the collect method, which has signature: List<T> collectAndReset(Attributes pointAttributes)

This is problematic because according to the spec, ExemplarReservoir needs to be configurable via views. Views do not have a generic type indicating applicability to longs or doubles. A single view can apply to many instruments, and both double and long instruments simultaneously.

We need to resolve this mismatch before stabilizing the ExemplarReservoir interface. This PR suggests doing that by removing the generic type from ExemplarReservoir and instead giving ExemplarReservoir collection methods for both longs and doubles:

  • List<DoubleExemplarData> collectAndResetDoubles(Attributes pointAttributes)
  • List<LongExemplarData> collectAndResetLongs(Attributes pointAttributes)

This brings symmetry to recording, since ExemplarReservoir currently has both double and long methods for recording values.

Alternatively, we could have separate DoubleExemplarReservoir and LongExemplarReservoir types, and a new ExemplarReservoirFactory with methods createDoubleExemplarReservoir(..), createLongExemplarReservoir(..). Then it would be the ExemplarReservoirFactory which users would configure on views. I think this will end up being more cumbersome and awkward for users to implement.

Copy link

codecov bot commented Oct 10, 2025

Codecov Report

❌ Patch coverage is 93.97590% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.15%. Comparing base (6a7a620) to head (6d00153).
⚠️ Report is 22 commits behind head on main.

Files with missing lines Patch % Lines
.../metrics/internal/aggregator/AggregatorHandle.java 63.63% 4 Missing ⚠️
...try/sdk/metrics/internal/view/DropAggregation.java 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #7742      +/-   ##
============================================
- Coverage     90.16%   90.15%   -0.02%     
  Complexity     7195     7195              
============================================
  Files           814      814              
  Lines         21719    21719              
  Branches       2125     2128       +3     
============================================
- Hits          19584    19581       -3     
- Misses         1467     1469       +2     
- Partials        668      669       +1     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jack-berg jack-berg force-pushed the refactor-exemplar-reservoir-generics branch from 534d4ff to 6d00153 Compare October 10, 2025 15:55
List<T> collectAndReset(Attributes pointAttributes);
List<DoubleExemplarData> collectAndResetDoubles(Attributes pointAttributes);

List<LongExemplarData> collectAndResetLongs(Attributes pointAttributes);
Copy link
Contributor

Choose a reason for hiding this comment

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

Javadoc will be needed here. Could wait until we move out of internal, but just thought I'd highlight it now.

@jack-berg
Copy link
Member Author

@jkwatson I'm not completely satisfied with this. Did some brainstorming over the weekend and came up with this optional followup: #7758

WDYT? Is it better to have a single ExemplarReservoir responsible for recording / collecting both longs and doubles, or separate type-specific interfaces? Think about it from an implementation standpoint.

@jack-berg jack-berg closed this Oct 18, 2025
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.

2 participants