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.74% 86.82% +0.07%
==========================================
Files 258 258
Lines 11958 11973 +15
==========================================
+ Hits 10373 10395 +22
+ Misses 1585 1578 -7
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?
There was a problem hiding this comment.
Given the existence of delta/cumulative already in SDK, I think it is okay to add LowMemory enum setting in the SDK itself.
If we are adding ENV variable support, then it should only be in the OTLP package.
|
This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or pushing will instruct the bot to automatically remove the label. This bot runs once per day. |
github-actions
Bot
added
the
Stale
Kielek
removed
the
Stale
Kielek
left a comment
Kielek
left a comment
There was a problem hiding this comment.
LGTM, especially considering @cijothomas comment
3a96380
|
Thank you for your contribution @hannahhaering! 🎉 We would like to hear from you about your experience contributing to OpenTelemetry by taking a few minutes to fill out this survey. |
6 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