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

Add selector of exemplar reservoir providers to metric.Stream configuration #5861

Merged
merged 8 commits into from
Oct 18, 2024

Conversation

dashpole
Copy link
Contributor

@dashpole dashpole commented Oct 1, 2024

Resolve #5249

Spec

exemplar_reservoir: A functional type that generates an exemplar reservoir a MeterProvider will use when storing exemplars. This functional type needs to be a factory or callback similar to aggregation selection functionality which allows different reservoirs to be chosen by the aggregation.

Users can provide an exemplar_reservoir, but it is up to their discretion. Therefore, the stream configuration parameter needs to be structured to accept an exemplar_reservoir, but MUST NOT obligate a user to provide one. If the user does not provide an exemplar_reservoir value, the MeterProvider MUST apply a default exemplar reservoir.

Also,

the reservoir MUST be given the Attributes associated with its timeseries point either at construction so that additional sampling performed by the reservoir has access to all attributes from a measurement in the "offer" method.

Changes

In sdk/metric/exemplar, add:

  • exemplar.ReservoirProvider
  • exemplar.HistogramReservoirProvider
  • exemplar.FixedSizeReservoirProvider

In sdk/metric, add:

  • metric.ExemplarReservoirProviderSelector (func Aggregation -> ReservoirProvider)
  • metric.DefaultExemplarReservoirProviderSelector (our default implementation)
  • ExemplarReservoirProviderSelector added to metric.Stream

Note: the only required public types are metric.ExemplarReservoirProviderSelector and ExemplarReservoirProviderSelector in metric.Stream. Others are for convenience and readability.

Alternatives considered

Add ExemplarReservoirProvider directly to metric.Stream, instead of ExemplarReservoirProviderSelector

This would mean users can configure a func() exemplar.Reservoir instead of a func(Aggregation) func() exemplar.Reservoir.

I don't think this complies with the statement: This functional type needs to be a factory or callback similar to aggregation selection functionality which allows different reservoirs to be chosen by the aggregation.. I'm interpreting "allows different reservoirs to be chosen by the aggregation" as meaning "allows different reservoirs to be chosen based on the aggregation", rather than meaning that the aggregation is somehow choosing the reservoir.

Future work

There is some refactoring I plan to do after this to simplify the interaction between the internal/aggregate and exemplar package. I've omitted that from this PR to keep the diff smaller.

Copy link

codecov bot commented Oct 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.5%. Comparing base (d70f3da) to head (46de771).
Report is 2 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #5861   +/-   ##
=====================================
  Coverage   84.5%   84.5%           
=====================================
  Files        272     272           
  Lines      22840   22853   +13     
=====================================
+ Hits       19314   19325   +11     
- Misses      3177    3179    +2     
  Partials     349     349           

see 8 files with indirect coverage changes

@dashpole dashpole mentioned this pull request Oct 1, 2024
6 tasks
@dashpole dashpole force-pushed the stream_reservoir branch 3 times, most recently from 5a6a897 to 7acaf00 Compare October 14, 2024 20:46
@dashpole dashpole marked this pull request as ready for review October 15, 2024 17:08
@dashpole dashpole changed the title [PoC] Add selector of exemplar reservoir providers to metric.Stream configuration Add selector of exemplar reservoir providers to metric.Stream configuration Oct 15, 2024
@dashpole
Copy link
Contributor Author

This is now ready for review! There is one notable change to a public function, but it does not change the public interface:

  • exemplar.NewHistogramReservoir now expects bounds to be sorted before they are passed to the function.

I don't expect this behavioral change to impact any users since the stream configuration (this PR) is the only reason why people would need to call NewHistogramReservoir.

@dashpole
Copy link
Contributor Author

xref: java prototype open-telemetry/opentelemetry-java#5960

sdk/metric/exemplar.go Outdated Show resolved Hide resolved
sdk/metric/exemplar/histogram_reservoir.go Show resolved Hide resolved
sdk/metric/exemplar.go Outdated Show resolved Hide resolved
dashpole and others added 2 commits October 17, 2024 13:30
@dashpole dashpole merged commit 81b2a33 into open-telemetry:main Oct 18, 2024
30 checks passed
@dashpole dashpole deleted the stream_reservoir branch October 18, 2024 13:05
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.

Stabilize exemplars
5 participants