feat(exporter/prometheus): add default_aggregation parameter to PrometheusMetricReader#5117
feat(exporter/prometheus): add default_aggregation parameter to PrometheusMetricReader#5117Manvi2402 wants to merge 9 commits intoopen-telemetry:mainfrom
Conversation
…theusMetricReader
MikeGoldsmith
left a comment
There was a problem hiding this comment.
This looks good, thanks @Manvi2402. I've left some things to tweak.
I think it's gonna conflict with @herin049's PR (#5118). Whoever's PR merges later will likely have a conflict to resolve.
MikeGoldsmith
left a comment
There was a problem hiding this comment.
Looks good - thanks @Manvi2402 👍🏻
| histogram = meter.create_histogram("test_histogram") | ||
| histogram.record(5) | ||
| result = list(reader._collector.collect()) | ||
| self.assertTrue(len(result) > 0) |
There was a problem hiding this comment.
Can we do some additional verification here to actually verify that the boundaries specified above are respected?
There was a problem hiding this comment.
Done! Added verification to check that the custom boundaries [1.0, 5.0, 10.0] are present in the Prometheus output. Thanks for the suggestion.
…om/Manvi2402/opentelemetry-python into fix/prometheus-default-aggregation
|
This PR has been automatically marked as stale because it has not had any activity for 14 days. It will be closed if no further activity occurs within 14 days of this comment. |
|
@herin049 please can you re-review, @Manvi2402 made the change you suggested. |
Description
Add
default_aggregationparameter toPrometheusMetricReaderto allow configuring default aggregation per instrument kind, aligning with the Prometheus Exporter spec.Fixes #5109
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
test_default_aggregationin test_prometheus_exporter.pythat verifies custom aggregation is correctly passed to the MetricReader.
Does This PR Require a Contrib Repo Change?
Checklist: