-
Notifications
You must be signed in to change notification settings - Fork 649
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 exemplars feature #4094
Add exemplars feature #4094
Conversation
|
Dear maintainers, I would like to contribute to this project by adding the support for exemplars. Currently, I have added the base classes (based on the JavaScript SDK implementation). And now I'm facing challenges to integrate this into the code base. Analyzing the C# and Java SDK, it seems that the opentelemetry-python/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py Line 82 in 51b7e4c
But this raises 3 questions:
Any additional pointers would be appreciated. |
Thanks so much for picking this up. I am not too familiar with exemplars but happy to open up a discussion about the architectural design. A couple of questions/changes I have in mind:
This would probably entail adding a timestamp to Measurement to represent when the api call was made/when the
Most likely another field in
I am not exactly sure what "accumulated Exemplars" means but from my basic understanding it seems like we just need to return the set of
There will most likely be a single
Most likely passed down as a reference to the
Is there a need for a factory? What are you envisioning here? Feel free to add any insights or correct any misunderstandings I may have :) |
Thanks a lot @lzchen for all the information. Thanks for pointing out about the
This is also my understanding. The reservoir will provide what it collected.
👍
👍 I missed that point in the spec
This is implemented in the Java SDK - and I guess it is required to support:
Thanks a lot for the pointers, I'll work towards a first working version. |
Hi there! I have also started looking into adding exemplars as well and would love to chat with you about it (I've reached out on linkedin). |
@catherine-m-zhang you should have receive an invitation to my fork. Could you accept it so I can grant you enough rights to push on the branch directly? I updated the PR description with the changes. Here is a status of the work as far as I can tell:
|
@lzchen here is a first workish version - feel free to have a look especially at the API changes to see if they fit the current API logic. I'm off for the next 15 days but Catherine offers to help on the subject. |
Is this pr ready for review? If that's the case, feel free to mark as a PR instead of draft. |
150ae5f
to
56549ef
Compare
56549ef
to
70f8bef
Compare
@lzchen I don't think I have the correct access to mark this as ready for review but this is ready to be reviewed! |
opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/measurement.py
Outdated
Show resolved
Hide resolved
opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/exemplar/exemplar_reservoir.py
Outdated
Show resolved
Hide resolved
Dear maintainers, this PR is finalized (CI should be green 🤞) and ready for final review. Gentle ping to @lzchen @pmcollins @emdneto |
@fcollonval what else is remaining to close on this PR? |
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.
Great job in bringing this into fruition! If there are no more changes I can merge this in if you'd like.
Description
Fixes #2407
Fixes open-telemetry/opentelemetry-python-contrib#2158
API changes
opentelemetry.metrics.instrument
: Add optionalContext
argument to all record/add/set value1opentelemetry.metrics.Observation
: Add optionalContext
attributeExemplar
,ExemplarFilter
s andExemplarReservoir
s following the spec and the implementation in JS and Java SDKopentelemety.sdk.metrics.MeterProvider
: Add optionalexemplar_filter
attribute to the constructorThe filter will be stored in the
SdkConfiguration
(via a new attributeexemplar_filter
)opentelemety.sdk.metrics._internal.Measurement
: Add mandatorytime_unix_nano
andContext
attributesopentelemetry.sdk.metrics._internal.*DataPoint
: Add optionalexemplars
attributeopentelemetry.sdk.metrics._internal._Aggregation
:_reservoir
set from areservoir_factory
_collect_exemplar
that is called by the children classes oncollect
aggregate
not abstracted any longer and add an optionalshould_sample_exemplar
argument. If it is True (default), the exemplar reservoir will be offer toMeasurement
to store an exemplar. That method need to be called viasuper
on all children classesopentelemetry.sdk.metrics.view.View
: Add optional attributeexemplar_reservoir_factory
defining the exemplar reservoir factory per_Aggregation
type.should_sample_exemplar
argument on methodconsume_measurement
fromMeasurementConsumer
,MetricReaderStorage
and_ViewInstrumentMatch
Notes
MetricProvider
, hence I went for propagating theshould_sample
value along side the measurements. This is the approach chosen in the .NET SDK. Another possibility would be to mimic the Java SDK approach that is wrapping the wanted exemplar reservoir in an exemplar reservoir applying first the filter (although I don't know right now how to implement that variant).View
is based on_Aggregation
type. I find that non-optimal as those classes are meant to be protected when the factory is meant to be public. But I don't see an alternative._Aggregation
type returns a(**kwargs): ExemplarReservoir
factory because some reservoirs need to be configured based on the aggregation parameters; e.g. the exponential histogram bucket size will impact the number of buckets in the exemplar reservoir.Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Unit tests have been added for the new base class as well as for instrumentation interactions and a integration test with the console exporter.
Does This PR Require a Contrib Repo Change?
It does not require a change. But the metrics exporters in that repo will require an update to include the added exemplars; best as a follow-up of this PR.
Checklist:
Footnotes
This is similar to the Java and Javascript SDK. ↩