Add low memory temporality for metrics #6648
Conversation
github-actions
bot
added
pkg:OpenTelemetry
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6648 +/- ##
==========================================
- Coverage 86.76% 86.72% -0.04%
==========================================
Files 258 258
Lines 11958 11973 +15
==========================================
+ Hits 10375 10384 +9
- Misses 1583 1589 +6
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| private static readonly Func<Type, AggregationTemporality> LowMemoryTemporalityPreferenceFunc = (instrumentType) => | ||
| { | ||
| return instrumentType.GetGenericTypeDefinition() switch | ||
| { | ||
| var type when type == typeof(Counter<>) => AggregationTemporality.Delta, | ||
| var type when type == typeof(Histogram<>) => AggregationTemporality.Delta, | ||
|
|
||
| var type when type == typeof(UpDownCounter<>) => AggregationTemporality.Cumulative, | ||
| var type when type == typeof(ObservableCounter<>) => AggregationTemporality.Cumulative, | ||
| var type when type == typeof(ObservableUpDownCounter<>) => AggregationTemporality.Cumulative, | ||
|
|
||
| _ => AggregationTemporality.Cumulative, | ||
| }; | ||
| }; |
There was a problem hiding this comment.
I wonder if using TypeHandle property would be more performant here.
There was a problem hiding this comment.
Unless there's a huge benefit, I think it's better to make the code obvious by using ==. It's also not obvious from the source code what that property does differently as it just throws an exception.
| /// ObservableUpDownCounter instruments. This mode reduces SDK memory usage by avoiding | ||
| /// the need to store both cumulative and delta states for temporality conversion. | ||
| /// </summary> | ||
| LowMemory = 3, |
There was a problem hiding this comment.
Technically, all these 3 options are defined only for OTLP exporter https://github.com/open-telemetry/opentelemetry-specification/blob/7f6d35f758bb5d92e354460d040974665a29ba32/specification/metrics/sdk_exporters/otlp.md?plain=1#L55 and it shouldn't be part of the SDK.
Based on current design, I do not see any better option to put in the current class design.
@alanwest, do you remember any historical reasons for keeping it here?
4 participants
Implements #4109
Changes
Added low memory temporality as an option in the OTLP metrics exporter, following the specification
Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial changes