-
Notifications
You must be signed in to change notification settings - Fork 849
Add low memory temporality for metrics #6648
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
43448be
c4d661e
6fec96a
aa46340
14b7858
7fecd5e
b10b549
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| OpenTelemetry.Metrics.MetricReaderTemporalityPreference.LowMemory = 3 -> OpenTelemetry.Metrics.MetricReaderTemporalityPreference |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,4 +21,12 @@ public enum MetricReaderTemporalityPreference | |
| /// Aggregations of non-monotonic measurements use cumulative temporality. | ||
| /// </summary> | ||
| Delta = 2, | ||
|
|
||
| /// <summary> | ||
| /// Uses delta temporality for synchronous Counter and Histogram instruments and | ||
| /// cumulative temporality for synchronous UpDownCounter, ObservableCounter and | ||
| /// 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,103 @@ | ||
| // Copyright The OpenTelemetry Authors | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| using System.Diagnostics.Metrics; | ||
| using OpenTelemetry.Tests; | ||
| using Xunit; | ||
|
|
||
| namespace OpenTelemetry.Metrics.Tests; | ||
|
|
||
| public class MetricReaderTests | ||
| { | ||
| [Theory] | ||
| [InlineData("counter", typeof(long), AggregationTemporality.Delta)] | ||
| [InlineData("counter", typeof(double), AggregationTemporality.Delta)] | ||
| [InlineData("histogram", typeof(long), AggregationTemporality.Delta)] | ||
| [InlineData("histogram", typeof(double), AggregationTemporality.Delta)] | ||
| [InlineData("updowncounter", typeof(long), AggregationTemporality.Cumulative)] | ||
| [InlineData("updowncounter", typeof(double), AggregationTemporality.Cumulative)] | ||
| [InlineData("observablecounter", typeof(long), AggregationTemporality.Cumulative)] | ||
| [InlineData("observablecounter", typeof(double), AggregationTemporality.Cumulative)] | ||
| [InlineData("observableupdowncounter", typeof(long), AggregationTemporality.Cumulative)] | ||
| [InlineData("observableupdowncounter", typeof(double), AggregationTemporality.Cumulative)] | ||
| public void LowMemoryTemporality_UsesCorrectAggregationTemporality(string instrumentName, Type valueType, AggregationTemporality expectedTemporality) | ||
| { | ||
| var metrics = new List<Metric>(); | ||
| using var meter = new Meter(Utils.GetCurrentMethodName()); | ||
| using var provider = Sdk.CreateMeterProviderBuilder() | ||
| .AddMeter(meter.Name) | ||
| .AddInMemoryExporter(metrics, metricReaderOptions => | ||
| { | ||
| metricReaderOptions.TemporalityPreference = MetricReaderTemporalityPreference.LowMemory; | ||
| }) | ||
| .Build(); | ||
|
|
||
| switch (instrumentName) | ||
| { | ||
| case "counter": | ||
| if (valueType == typeof(long)) | ||
| { | ||
| meter.CreateCounter<long>("test_counter").Add(1); | ||
| } | ||
| else | ||
| { | ||
| meter.CreateCounter<double>("test_counter").Add(1); | ||
| } | ||
|
|
||
| break; | ||
|
|
||
| case "histogram": | ||
| if (valueType == typeof(long)) | ||
| { | ||
| meter.CreateHistogram<long>("test_histogram").Record(1); | ||
| } | ||
| else | ||
| { | ||
| meter.CreateHistogram<double>("test_histogram").Record(1); | ||
| } | ||
|
|
||
| break; | ||
|
|
||
| case "updowncounter": | ||
| if (valueType == typeof(long)) | ||
| { | ||
| meter.CreateUpDownCounter<long>("test_updown").Add(1); | ||
| } | ||
| else | ||
| { | ||
| meter.CreateUpDownCounter<double>("test_updown").Add(1); | ||
| } | ||
|
|
||
| break; | ||
|
|
||
| case "observablecounter": | ||
| if (valueType == typeof(long)) | ||
| { | ||
| meter.CreateObservableCounter("test_observable_counter", () => new Measurement<long>(1)); | ||
| } | ||
| else | ||
| { | ||
| meter.CreateObservableCounter("test_observable_counter", () => new Measurement<double>(1)); | ||
| } | ||
|
|
||
| break; | ||
|
|
||
| case "observableupdowncounter": | ||
| if (valueType == typeof(long)) | ||
| { | ||
| meter.CreateObservableUpDownCounter("test_observable_updown", () => new Measurement<long>(1)); | ||
| } | ||
| else | ||
| { | ||
| meter.CreateObservableUpDownCounter("test_observable_updown", () => new Measurement<double>(1)); | ||
| } | ||
|
|
||
| break; | ||
| } | ||
|
|
||
| provider.ForceFlush(); | ||
|
|
||
| var metric = Assert.Single(metrics); | ||
| Assert.Equal(expectedTemporality, metric.Temporality); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if using
TypeHandleproperty would be more performant here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.