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

[sdk-metrics] Exemplar spec improvements #5386

Merged

Conversation

CodeBlanch
Copy link
Member

@CodeBlanch CodeBlanch commented Feb 23, 2024

[Split out from #5364]

Changes

ExemplarReservoir

  • Offer method signature no longer includes the "index" parameter which is not part of the spec. Instead it now accepts struct ExemplarMeasurement which only exposes the value & tags (to match spec) but also tracks explicit histogram index as an internal thing.
  • Collect method signature no longer include "actualTags" or "reset" which are not part of the spec. The "reset" flag has become bool ResetOnCollect property on ExemplarReservoir. Tag filtering is now handled by struct ReadOnlyFilteredTagCollection which is exposed on Exemplar. Return type changed from Exemplar[] to struct ReadOnlyExemplarCollection.

Exemplar

  • Return type of FilteredTags property is now struct ReadOnlyFilteredTagCollection instead of List<KeyValuePair<string, object?>>?.
  • Now exposes long LongValue in addition to double DoubleValue.

MetricPoint

  • Exemplar[] GetExemplars() is now bool TryGetExemplars(out ReadOnlyExemplarCollection? exemplars).

Misc improvements

  • ReadOnlyExemplarCollection when enumerated only returns Exemplars which have been updated (Timestamp != default). Previously each exporter had to understand the behavior and check that.
  • Removed nasty Linq & allocations from Exemplar.FilteredTags.
  • Introduced internal FixedSizeExemplarReservoir to house the shared logic both SimpleFixedSizeExemplarReservoir & AlignedHistogramBucketExemplarReservoir share.

Merge requirement checklist

  • CONTRIBUTING guidelines followed (license requirements, nullable enabled, static analysis, etc.)
  • Unit tests added/updated
  • Appropriate CHANGELOG.md files updated for non-trivial changes
  • Changes in public API reviewed (if applicable)

@CodeBlanch CodeBlanch added pkg:OpenTelemetry Issues related to OpenTelemetry NuGet package metrics Metrics signal related labels Feb 23, 2024
@CodeBlanch CodeBlanch requested a review from a team February 23, 2024 22:08
Copy link

codecov bot commented Feb 23, 2024

Codecov Report

Attention: Patch coverage is 66.22222% with 76 lines in your changes are missing coverage. Please review.

Project coverage is 83.25%. Comparing base (6250307) to head (b10a2c9).
Report is 99 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5386      +/-   ##
==========================================
- Coverage   83.38%   83.25%   -0.14%     
==========================================
  Files         297      284      -13     
  Lines       12531    12049     -482     
==========================================
- Hits        10449    10031     -418     
+ Misses       2082     2018      -64     
Flag Coverage Δ
unittests ?
unittests-Solution-Experimental 83.17% <66.22%> (?)
unittests-Solution-Stable 83.10% <66.22%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
src/OpenTelemetry/Metrics/AggregatorStore.cs 82.08% <100.00%> (+1.69%) ⬆️
...xemplar/AlignedHistogramBucketExemplarReservoir.cs 80.00% <100.00%> (-12.86%) ⬇️
...nTelemetry/Metrics/Exemplar/ExemplarMeasurement.cs 100.00% <100.00%> (ø)
...penTelemetry/Metrics/Exemplar/ExemplarReservoir.cs 100.00% <100.00%> (ø)
...Telemetry/Metrics/MetricPointOptionalComponents.cs 100.00% <100.00%> (+22.22%) ⬆️
src/OpenTelemetry/ReadOnlyFilteredTagCollection.cs 100.00% <100.00%> (ø)
src/OpenTelemetry/ReadOnlyTagCollection.cs 100.00% <100.00%> (ø)
...try/Metrics/Exemplar/FixedSizeExemplarReservoir.cs 96.77% <96.77%> (ø)
...trics/Exemplar/SimpleFixedSizeExemplarReservoir.cs 87.50% <90.00%> (ø)
src/OpenTelemetry/Metrics/Exemplar/Exemplar.cs 91.11% <91.11%> (-8.89%) ⬇️
... and 4 more

... and 48 files with indirect coverage changes

Copy link
Member

@cijothomas cijothomas left a comment

Choose a reason for hiding this comment

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

Thanks a lot!
left couple of minor comments, lets address it as follow ups

@CodeBlanch CodeBlanch merged commit 42c593d into open-telemetry:main Feb 27, 2024
37 checks passed
@CodeBlanch CodeBlanch deleted the sdk-exemplar-spec-improvements2 branch February 27, 2024 18:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
metrics Metrics signal related pkg:OpenTelemetry Issues related to OpenTelemetry NuGet package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants