-
Notifications
You must be signed in to change notification settings - Fork 816
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
feat(sdk-metrics): implement MetricProducer specification #4007
Conversation
0271b33
to
b0434be
Compare
* Additional MetricProducers to use as a source of aggregated metric data in addition to the | ||
* SDK's metric data. | ||
*/ | ||
metricProducers?: MetricProducer[]; |
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.
Note the specification asks for a RegisterMetricProducer
function https://github.com/open-telemetry/opentelemetry-specification/blob/v1.23.0/specification/metrics/sdk.md#registerproducermetricproducer
However, I since we already have a public setMetricProducer
which assumes it is receiving the SDK's MetricCollector
, this seems like the cleanest solution without breaking anything.
I will also check with the spec on this.
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.
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.
I also think having it as a config makes the most sense in our case. If the spec wording gets changed then I'd much prefer having it be config-only. 🙂
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.
open-telemetry/opentelemetry-specification#3613 has a few approvals now
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #4007 +/- ##
=======================================
Coverage 92.36% 92.36%
=======================================
Files 321 320 -1
Lines 9264 9251 -13
Branches 1968 1963 -5
=======================================
- Hits 8557 8545 -12
+ Misses 707 706 -1
|
81a7c59
to
49ac289
Compare
49ac289
to
2a5a31c
Compare
), | ||
]); | ||
|
||
// Merge the results, keeping the SDK's Resource |
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.
I think we should add multiple resources support to the metric reader and metric exporter instead of dropping resources silently -- the registered metric producer can be different meter providers that have different resources.
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.
I'm not sure how we could do that without making a breaking change. Also see this part of the spec:
If the MeterProvider is an instance of MetricProducer, this MAY be used to register the MeterProvider, but MUST NOT allow multiple MeterProviders to be registered with the same MetricReader.
That's not exactly the case in the JS implementation, but I think the spirit of it is to avoid this happening. I was actually thinking of making the MetricProducer only able to return ScopeMetrics to avoid the issue altogether.
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.
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.
I was actually thinking of making the MetricProducer only able to return ScopeMetrics to avoid the issue altogether.
I like this idea. However, I wonder if we could do this without breaking the MeterProvider
as it also implements the MetricProducer
interface. 🤔
It's a tricky situation, but I think we can leave this as-is and then iterate later depending on the outcome of open-telemetry/opentelemetry-specification#3636 - the code proposed here never alters anything unless the user opts into using an experimental feature by setting the metricProducers
config - I think in that case it would be okay to change the behavior later.
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.
Is this OK with you for now @legendecas?
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.
Thank you for working on this. 🙂
A few questions came up while I was reviewing:
is it always expected that people use MetricProducer with an SDK (MeterProvider), or also without one? 🤔
In that case, the PeriodicExportingMetricReader would only start once setMetricProducer()
is called manually, right? Or would MetricProducers be expected to register themselves with the reader as the MeterProvider does in some SDK implementations (ours uses setMetricProducer()
, I think in Python the MeterProvider registers a callback with the MetricReaders)?
Would it make sense to have similar ergonomics to a MeterProvider
to handle this registration? 🤔
), | ||
]); | ||
|
||
// Merge the results, keeping the SDK's Resource |
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.
I was actually thinking of making the MetricProducer only able to return ScopeMetrics to avoid the issue altogether.
I like this idea. However, I wonder if we could do this without breaking the MeterProvider
as it also implements the MetricProducer
interface. 🤔
It's a tricky situation, but I think we can leave this as-is and then iterate later depending on the outcome of open-telemetry/opentelemetry-specification#3636 - the code proposed here never alters anything unless the user opts into using an experimental feature by setting the metricProducers
config - I think in that case it would be okay to change the behavior later.
* Additional MetricProducers to use as a source of aggregated metric data in addition to the | ||
* SDK's metric data. | ||
*/ | ||
metricProducers?: MetricProducer[]; |
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.
I also think having it as a config makes the most sense in our case. If the spec wording gets changed then I'd much prefer having it be config-only. 🙂
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.
Thank you for working on this!
Thanks for the review everyone. I noticed you left a few questions I forgot to answer @pichlermarc
I don't think we should allow this, IMO passing the MetricProducers when creating the MeterProvider is cleaner for the callers. Actually I think I should add a doc comment to |
Hmm, the reason I was asking is mostly regarding a possible stand-alone use of a MetricProducer: is this something that is intended? If we decide against using the
I think this is a good idea. 👍 I think there's also a jsdoc annotation |
Pushed f095627 with this change. |
Summary from the SIG: It's like this on purpose to discourage circumventing the SDK. Users shouldn't use the MetricProducer without a MeterProvider, therefore changes to the |
Which problem is this PR solving?
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
Fixes #4006
Short description of the changes
Allows adding additional MetricProducers to a MetricReader which can be used to produce additional data beyond what the SDK produces
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Added new tests cases
Checklist: