feat: metric aggregation temporality controls #2902
Conversation
seemk
changed the title
Codecov Report
@@ Coverage Diff @@
## main #2902 +/- ##
==========================================
- Coverage 93.01% 92.78% -0.23%
==========================================
Files 183 183
Lines 5909 5921 +12
Branches 1254 1257 +3
==========================================
- Hits 5496 5494 -2
- Misses 413 427 +14
|
experimental/packages/opentelemetry-exporter-metrics-otlp-grpc/test/OTLPMetricExporter.test.ts
Outdated
Show resolved
Hide resolved
experimental/packages/opentelemetry-sdk-metrics-base/src/export/MetricReader.ts
Outdated
Show resolved
Hide resolved
experimental/packages/opentelemetry-sdk-metrics-base/src/export/AggregationTemporality.ts
Outdated
Show resolved
Hide resolved
…/opentelemetry-js into instrument-aggregation-temporality
| @@ -69,7 +69,7 @@ export class TemporalMetricProcessor<T> { | |||
| sdkStartTime: HrTime, | |||
| collectionTime: HrTime, | |||
| ): Maybe<MetricData> { | |||
| const aggregationTemporality = collector.aggregatorTemporality; | |||
| const aggregationTemporality = collector.selectAggregationTemporality(instrumentDescriptor.type); | |||
There was a problem hiding this comment.
As the selectAggregationTemporality can call into user codes, I think we should memo the selection result at the creation of the instrument so that it can not be accidentally changed: like returning CUMULATIVE for first call and DELTA for second call.
There was a problem hiding this comment.
Do you mean basically attaching it to instrument descriptor at the time of creation?
There was a problem hiding this comment.
Yeah, or attaching it to the internal object MetricStorage, since the spec didn't ask to expose this info.
There was a problem hiding this comment.
Eh, sorry for the turn back. Seems this is not settled yet?
There was a problem hiding this comment.
I was just wondering that the temporality is currently retrieved via the collector, then at instrument creation we would create a mapping between each collector and temporality (based on instrument type, by calling collector's selector), store this map at the MetricStorage - we'd need to handle the case where a metric reader / collector is added after instrument creation, then go over existing storages and update the collector to temporality mapping?
If this logic seems fine, I can add it, although I'd prefer to do it without all the extra state
There was a problem hiding this comment.
Actually, it can be simpler if we add a new parameter aggregationTemporality to TemporalMetricProcessor.buildMetrics and save the aggregationTemporality to MetricStorage. Or we can construct TemporalMetricProcessor with an aggregationTemporality since we don't want it to be changed anyway.
There was a problem hiding this comment.
although I'd prefer to do it without all the extra state
In this sense, how should we handle the aggregationTemporality changes if the user-provided selector changes the returned value from time to time?
There was a problem hiding this comment.
Actually, it can be simpler if we add a new parameter
aggregationTemporalitytoTemporalMetricProcessor.buildMetricsand save theaggregationTemporalityto MetricStorage. Or we can constructTemporalMetricProcessorwith anaggregationTemporalitysince we don't want it to be changed anyway.
Yup, but in order to save aggregationTemporality to MetricStorage we need to invoke the reader's/collector's selector, right? But if I read it correctly there can be many readers for a single storage, so we'd need a mapping. Just a bit confused here
There was a problem hiding this comment.
You are right. This is making the problem complicated. I'm ok with this as is.
experimental/packages/opentelemetry-exporter-metrics-otlp-grpc/test/OTLPMetricExporter.test.ts
Outdated
Show resolved
Hide resolved
…/opentelemetry-js into instrument-aggregation-temporality
|
@seemk I think this PR is ready to go. would you mind updating the branch on top of the latest main branch and resolving the conflicts? thank you! |
|
Sorry it took so long to review. I thought I had reviewed this already so I kept skipping it in the list 😨 |
Which problem is this PR solving?
Fixes #2864
Short description of the changes
getPreferredAggregationTemporalityatMetricReaderand exporters. They now have a selector, taking in an instrument type, that specifies which temporality to use. The replacement is nowgetAggregationTemporality(instrumentType: InstrumentType)which invokes the given selector.aggregationTemporalityfrom exporters to metric data type itself. This simplifies code - now the aggregation is selected in the metrics processor and passed to the creation of metric data, otherwise conversion code in exporters would have to invoke the same selector again.Type of change
How Has This Been Tested?
Checklist: