opentelemetry-sdk: add support for metric producers#5419
Conversation
There was a problem hiding this comment.
Pull request overview
Adds MetricProducer support to the OpenTelemetry Python SDK metrics pipeline, allowing MetricReader instances to incorporate metrics from third-party sources during collection (per the spec linked in the PR description).
Changes:
- Introduces a
MetricProducerinterface and wiresmetric_producersintoMetricReader(includingInMemoryMetricReaderandPeriodicExportingMetricReader). - Updates metric collection to invoke producers, merge produced
ScopeMetricsintoMetricsData, and isolate producer failures/timeouts via logging. - Adds focused unit tests validating merge behavior, failure isolation, and timeout-budget handling; updates existing measurement consumer tests for the new reader attribute.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| opentelemetry-sdk/tests/metrics/test_metric_producer.py | New tests covering merge semantics, multi-producer behavior, producer failures, and timeout budgeting. |
| opentelemetry-sdk/tests/metrics/test_measurement_consumer.py | Updates mocks to include _metric_producers for collection paths that now consult producers. |
| opentelemetry-sdk/src/opentelemetry/sdk/metrics/export/init.py | Re-exports MetricProducer as part of the public metrics export API surface. |
| opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/measurement_consumer.py | Collects from producers during collect() and merges produced scope metrics into collected SDK metrics with timeout/failure isolation. |
| opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/export/init.py | Adds MetricProducer ABC; adds metric_producers plumbing to readers and stores them on MetricReader. |
| .changelog/5419.added | Changelog entry for MetricProducer support. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return result | ||
|
|
||
| @staticmethod | ||
| def _collect_from_producers( |
There was a problem hiding this comment.
I have opted to treat errors (including timeouts) surfaced from metric producers as "soft" errors to prevent potential disruptions to the standard SDK metric collection process.
There doesn't seem to be a consensus on how to handle this scenario across SDK implementations (C++ roughly uses the approach here, whereas Java/Go halt the entire collection process).
I'm open to suggestions and additional comments here.
|
|
||
| @abstractmethod | ||
| def produce( | ||
| self, timeout_millis: float = 10_000 |
There was a problem hiding this comment.
no metric_filter param ? https://opentelemetry.io/docs/specs/otel/metrics/sdk/#produce-batch
There was a problem hiding this comment.
it also says
If the batch of Metric Points includes resource information, Produce SHOULD require a resource as a parameter. Produce does not have any other required parameters, however, OpenTelemetry SDK authors MAY choose to add required or optional parameters (e.g. timeout).
so should there be a resource param ?
There was a problem hiding this comment.
no metric_filter param ? https://opentelemetry.io/docs/specs/otel/metrics/sdk/#produce-batch
Going to add this in a follow-up.
so should there be a resource param ?
There should only be a Resource param if the batch of metric datapoints include resource information. In our case, we expect produce() to yield a batch of ScopeMetrics which do not contain resource information, so we don't need to pass this parameter. The creation of the corresponding Resource object is handled automatically by the SDK in _merge_producer_metrics() which is why it's not needed on this method.
Description
Adds support for the
MetricProducerinterface as outlined in the OpenTelemetry spec: https://opentelemetry.io/docs/specs/otel/metrics/sdk/#metricproducerFixes #4768
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Does This PR Require a Contrib Repo Change?
Checklist: