Refactor temporality to align with the spec (#2666)

open-telemetry · Nov 23, 2021 · fb161af · fb161af
1 parent 8d339bb
commit fb161af
Show file tree

Hide file tree

Showing 20 changed files with 179 additions and 231 deletions.
diff --git a/src/OpenTelemetry.Exporter.Console/ConsoleExporterMetricsExtensions.cs b/src/OpenTelemetry.Exporter.Console/ConsoleExporterMetricsExtensions.cs
@@ -42,7 +42,7 @@ public static MeterProviderBuilder AddConsoleExporter(this MeterProviderBuilder
                 ? new BaseExportingMetricReader(exporter)
                 : new PeriodicExportingMetricReader(exporter, options.PeriodicExportingMetricReaderOptions.ExportIntervalMilliseconds);
 
-            reader.PreferredAggregationTemporality = options.AggregationTemporality;
+            reader.Temporality = options.AggregationTemporality;
 
             return builder.AddReader(reader);
         }

diff --git a/src/OpenTelemetry.Exporter.Console/ConsoleMetricExporter.cs b/src/OpenTelemetry.Exporter.Console/ConsoleMetricExporter.cs
@@ -22,7 +22,6 @@
 
 namespace OpenTelemetry.Exporter
 {
-    [AggregationTemporality(AggregationTemporality.Cumulative | AggregationTemporality.Delta, AggregationTemporality.Cumulative)]
     public class ConsoleMetricExporter : ConsoleExporter<Metric>
     {
         private Resource resource;

diff --git a/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpMetricExporter.cs b/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpMetricExporter.cs
@@ -27,7 +27,6 @@ namespace OpenTelemetry.Exporter
     /// Exporter consuming <see cref="Metric"/> and exporting the data using
     /// the OpenTelemetry protocol (OTLP).
     /// </summary>
-    [AggregationTemporality(AggregationTemporality.Cumulative | AggregationTemporality.Delta, AggregationTemporality.Cumulative)]
     public class OtlpMetricExporter : BaseExporter<Metric>
     {
         private readonly IExportClient<OtlpCollector.ExportMetricsServiceRequest> exportClient;

diff --git a/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpMetricExporterExtensions.cs b/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpMetricExporterExtensions.cs
@@ -56,7 +56,7 @@ private static MeterProviderBuilder AddOtlpExporter(MeterProviderBuilder builder
 
             var metricExporter = new OtlpMetricExporter(options);
             var metricReader = new PeriodicExportingMetricReader(metricExporter, options.MetricExportIntervalMilliseconds);
-            metricReader.PreferredAggregationTemporality = options.AggregationTemporality;
+            metricReader.Temporality = options.AggregationTemporality;
             return builder.AddReader(metricReader);
         }
     }

diff --git a/src/OpenTelemetry/.publicApi/net461/PublicAPI.Unshipped.txt b/src/OpenTelemetry/.publicApi/net461/PublicAPI.Unshipped.txt
@@ -6,10 +6,8 @@ OpenTelemetry.Metrics.AggregationTemporality
 OpenTelemetry.Metrics.AggregationTemporality.Cumulative = 1 -> OpenTelemetry.Metrics.AggregationTemporality
 OpenTelemetry.Metrics.AggregationTemporality.Delta = 2 -> OpenTelemetry.Metrics.AggregationTemporality
 OpenTelemetry.Metrics.AggregationTemporalityAttribute
-OpenTelemetry.Metrics.AggregationTemporalityAttribute.AggregationTemporalityAttribute(OpenTelemetry.Metrics.AggregationTemporality supported) -> void
-OpenTelemetry.Metrics.AggregationTemporalityAttribute.AggregationTemporalityAttribute(OpenTelemetry.Metrics.AggregationTemporality supported, OpenTelemetry.Metrics.AggregationTemporality preferred) -> void
-OpenTelemetry.Metrics.AggregationTemporalityAttribute.Preferred.get -> OpenTelemetry.Metrics.AggregationTemporality
-OpenTelemetry.Metrics.AggregationTemporalityAttribute.Supported.get -> OpenTelemetry.Metrics.AggregationTemporality
+OpenTelemetry.Metrics.AggregationTemporalityAttribute.AggregationTemporalityAttribute(OpenTelemetry.Metrics.AggregationTemporality temporality) -> void
+OpenTelemetry.Metrics.AggregationTemporalityAttribute.Temporality.get -> OpenTelemetry.Metrics.AggregationTemporality
 OpenTelemetry.Metrics.BaseExportingMetricReader
 OpenTelemetry.Metrics.BaseExportingMetricReader.BaseExportingMetricReader(OpenTelemetry.BaseExporter<OpenTelemetry.Metrics.Metric> exporter) -> void
 OpenTelemetry.Metrics.BaseExportingMetricReader.SupportedExportModes.get -> OpenTelemetry.Metrics.ExportModes
@@ -74,11 +72,9 @@ OpenTelemetry.Metrics.MetricReader.Collect(int timeoutMilliseconds = -1) -> bool
 OpenTelemetry.Metrics.MetricReader.Dispose() -> void
 OpenTelemetry.Metrics.MetricReader.MetricReader() -> void
 OpenTelemetry.Metrics.MetricReader.ParentProvider.get -> OpenTelemetry.BaseProvider
-OpenTelemetry.Metrics.MetricReader.PreferredAggregationTemporality.get -> OpenTelemetry.Metrics.AggregationTemporality
-OpenTelemetry.Metrics.MetricReader.PreferredAggregationTemporality.set -> void
 OpenTelemetry.Metrics.MetricReader.Shutdown(int timeoutMilliseconds = -1) -> bool
-OpenTelemetry.Metrics.MetricReader.SupportedAggregationTemporality.get -> OpenTelemetry.Metrics.AggregationTemporality
-OpenTelemetry.Metrics.MetricReader.SupportedAggregationTemporality.set -> void
+OpenTelemetry.Metrics.MetricReader.Temporality.get -> OpenTelemetry.Metrics.AggregationTemporality
+OpenTelemetry.Metrics.MetricReader.Temporality.set -> void
 OpenTelemetry.Metrics.MetricReaderType
 OpenTelemetry.Metrics.MetricReaderType.Manual = 0 -> OpenTelemetry.Metrics.MetricReaderType
 OpenTelemetry.Metrics.MetricReaderType.Periodic = 1 -> OpenTelemetry.Metrics.MetricReaderType

diff --git a/src/OpenTelemetry/.publicApi/netstandard2.0/PublicAPI.Unshipped.txt b/src/OpenTelemetry/.publicApi/netstandard2.0/PublicAPI.Unshipped.txt
@@ -6,10 +6,8 @@ OpenTelemetry.Metrics.AggregationTemporality
 OpenTelemetry.Metrics.AggregationTemporality.Cumulative = 1 -> OpenTelemetry.Metrics.AggregationTemporality
 OpenTelemetry.Metrics.AggregationTemporality.Delta = 2 -> OpenTelemetry.Metrics.AggregationTemporality
 OpenTelemetry.Metrics.AggregationTemporalityAttribute
-OpenTelemetry.Metrics.AggregationTemporalityAttribute.AggregationTemporalityAttribute(OpenTelemetry.Metrics.AggregationTemporality supported) -> void
-OpenTelemetry.Metrics.AggregationTemporalityAttribute.AggregationTemporalityAttribute(OpenTelemetry.Metrics.AggregationTemporality supported, OpenTelemetry.Metrics.AggregationTemporality preferred) -> void
-OpenTelemetry.Metrics.AggregationTemporalityAttribute.Preferred.get -> OpenTelemetry.Metrics.AggregationTemporality
-OpenTelemetry.Metrics.AggregationTemporalityAttribute.Supported.get -> OpenTelemetry.Metrics.AggregationTemporality
+OpenTelemetry.Metrics.AggregationTemporalityAttribute.AggregationTemporalityAttribute(OpenTelemetry.Metrics.AggregationTemporality temporality) -> void
+OpenTelemetry.Metrics.AggregationTemporalityAttribute.Temporality.get -> OpenTelemetry.Metrics.AggregationTemporality
 OpenTelemetry.Metrics.BaseExportingMetricReader
 OpenTelemetry.Metrics.BaseExportingMetricReader.BaseExportingMetricReader(OpenTelemetry.BaseExporter<OpenTelemetry.Metrics.Metric> exporter) -> void
 OpenTelemetry.Metrics.BaseExportingMetricReader.SupportedExportModes.get -> OpenTelemetry.Metrics.ExportModes
@@ -74,11 +72,9 @@ OpenTelemetry.Metrics.MetricReader.Collect(int timeoutMilliseconds = -1) -> bool
 OpenTelemetry.Metrics.MetricReader.Dispose() -> void
 OpenTelemetry.Metrics.MetricReader.MetricReader() -> void
 OpenTelemetry.Metrics.MetricReader.ParentProvider.get -> OpenTelemetry.BaseProvider
-OpenTelemetry.Metrics.MetricReader.PreferredAggregationTemporality.get -> OpenTelemetry.Metrics.AggregationTemporality
-OpenTelemetry.Metrics.MetricReader.PreferredAggregationTemporality.set -> void
 OpenTelemetry.Metrics.MetricReader.Shutdown(int timeoutMilliseconds = -1) -> bool
-OpenTelemetry.Metrics.MetricReader.SupportedAggregationTemporality.get -> OpenTelemetry.Metrics.AggregationTemporality
-OpenTelemetry.Metrics.MetricReader.SupportedAggregationTemporality.set -> void
+OpenTelemetry.Metrics.MetricReader.Temporality.get -> OpenTelemetry.Metrics.AggregationTemporality
+OpenTelemetry.Metrics.MetricReader.Temporality.set -> void
 OpenTelemetry.Metrics.MetricReaderType
 OpenTelemetry.Metrics.MetricReaderType.Manual = 0 -> OpenTelemetry.Metrics.MetricReaderType
 OpenTelemetry.Metrics.MetricReaderType.Periodic = 1 -> OpenTelemetry.Metrics.MetricReaderType

diff --git a/src/OpenTelemetry/CHANGELOG.md b/src/OpenTelemetry/CHANGELOG.md
@@ -21,6 +21,9 @@
   `GetExplicitBounds` methods from `MetricPoint`.
   ([#2664](https://github.com/open-telemetry/opentelemetry-dotnet/pull/2664))
 
+* Refactored temporality setting to align with the latest spec.
+  ([#2666](https://github.com/open-telemetry/opentelemetry-dotnet/pull/2666))
+
 ## 1.2.0-beta2
 
 Released 2021-Nov-19

diff --git a/src/OpenTelemetry/Metrics/AggregationTemporalityAttribute.cs b/src/OpenTelemetry/Metrics/AggregationTemporalityAttribute.cs
@@ -21,22 +21,13 @@ namespace OpenTelemetry.Metrics
     [AttributeUsage(AttributeTargets.Class, AllowMultiple = false, Inherited = true)]
     public sealed class AggregationTemporalityAttribute : Attribute
     {
-        private AggregationTemporality preferredAggregationTemporality;
-        private AggregationTemporality supportedAggregationTemporality;
+        private AggregationTemporality temporality;
 
-        public AggregationTemporalityAttribute(AggregationTemporality supported)
-            : this(supported, supported)
+        public AggregationTemporalityAttribute(AggregationTemporality temporality)
         {
+            this.temporality = temporality;
         }
 
-        public AggregationTemporalityAttribute(AggregationTemporality supported, AggregationTemporality preferred)
-        {
-            this.supportedAggregationTemporality = supported;
-            this.preferredAggregationTemporality = preferred;
-        }
-
-        public AggregationTemporality Preferred => this.preferredAggregationTemporality;
-
-        public AggregationTemporality Supported => this.supportedAggregationTemporality;
+        public AggregationTemporality Temporality => this.temporality;
     }
 }
diff --git a/src/OpenTelemetry/Metrics/BaseExportingMetricReader.cs b/src/OpenTelemetry/Metrics/BaseExportingMetricReader.cs
@@ -38,8 +38,7 @@ public BaseExportingMetricReader(BaseExporter<Metric> exporter)
             if (attributes.Length > 0)
             {
                 var attr = (AggregationTemporalityAttribute)attributes[attributes.Length - 1];
-                this.PreferredAggregationTemporality = attr.Preferred;
-                this.SupportedAggregationTemporality = attr.Supported;
+                this.Temporality = attr.Temporality;
             }
 
             attributes = exportorType.GetCustomAttributes(typeof(ExportModesAttribute), true);

diff --git a/src/OpenTelemetry/Metrics/MetricReader.cs b/src/OpenTelemetry/Metrics/MetricReader.cs
@@ -27,34 +27,36 @@ namespace OpenTelemetry.Metrics
     /// </summary>
     public abstract partial class MetricReader : IDisposable
     {
-        private const AggregationTemporality CumulativeAndDelta = AggregationTemporality.Cumulative | AggregationTemporality.Delta;
+        private const AggregationTemporality AggregationTemporalityUnspecified = (AggregationTemporality)0;
         private readonly object newTaskLock = new object();
         private readonly object onCollectLock = new object();
         private readonly TaskCompletionSource<bool> shutdownTcs = new TaskCompletionSource<bool>();
-        private AggregationTemporality preferredAggregationTemporality = CumulativeAndDelta;
-        private AggregationTemporality supportedAggregationTemporality = CumulativeAndDelta;
+        private AggregationTemporality temporality = AggregationTemporalityUnspecified;
         private int shutdownCount;
         private TaskCompletionSource<bool> collectionTcs;
 
         public BaseProvider ParentProvider { get; private set; }
 
-        public AggregationTemporality PreferredAggregationTemporality
+        public AggregationTemporality Temporality
         {
-            get => this.preferredAggregationTemporality;
-            set
+            get
             {
-                ValidateAggregationTemporality(value, this.supportedAggregationTemporality);
-                this.preferredAggregationTemporality = value;
+                if (this.temporality == AggregationTemporalityUnspecified)
+                {
+                    this.temporality = AggregationTemporality.Cumulative;
+                }
+
+                return this.temporality;
             }
-        }
 
-        public AggregationTemporality SupportedAggregationTemporality
-        {
-            get => this.supportedAggregationTemporality;
             set
             {
-                ValidateAggregationTemporality(this.preferredAggregationTemporality, value);
-                this.supportedAggregationTemporality = value;
+                if (this.temporality != AggregationTemporalityUnspecified)
+                {
+                    throw new NotSupportedException($"The temporality cannot be modified (the current value is {this.temporality}).");
+                }
+
+                this.temporality = value;
             }
         }
 
@@ -268,28 +270,5 @@ protected virtual bool OnShutdown(int timeoutMilliseconds)
         protected virtual void Dispose(bool disposing)
         {
         }
-
-        private static void ValidateAggregationTemporality(AggregationTemporality preferred, AggregationTemporality supported)
-        {
-            Guard.Zero((int)(preferred & CumulativeAndDelta), $"PreferredAggregationTemporality has an invalid value {preferred}", nameof(preferred));
-            Guard.Zero((int)(supported & CumulativeAndDelta), $"SupportedAggregationTemporality has an invalid value {supported}", nameof(supported));
-
-            /*
-            | Preferred  | Supported  | Valid |
-            | ---------- | ---------- | ----- |
-            | Both       | Both       | true  |
-            | Both       | Cumulative | false |
-            | Both       | Delta      | false |
-            | Cumulative | Both       | true  |
-            | Cumulative | Cumulative | true  |
-            | Cumulative | Delta      | false |
-            | Delta      | Both       | true  |
-            | Delta      | Cumulative | false |
-            | Delta      | Delta      | true  |
-            */
-            string message = $"PreferredAggregationTemporality {preferred} and SupportedAggregationTemporality {supported} are incompatible";
-            Guard.Zero((int)(preferred & supported), message, nameof(preferred));
-            Guard.Range((int)preferred, nameof(preferred), max: (int)supported, maxName: nameof(supported), message: message);
-        }
     }
 }
diff --git a/src/OpenTelemetry/Metrics/MetricReaderExt.cs b/src/OpenTelemetry/Metrics/MetricReaderExt.cs
@@ -55,7 +55,7 @@ internal Metric AddMetricWithNoViews(Instrument instrument)
                 }
                 else
                 {
-                    var metric = new Metric(instrument, this.preferredAggregationTemporality, metricName, instrument.Description, this.maxMetricPointsPerMetricStream);
+                    var metric = new Metric(instrument, this.Temporality, metricName, instrument.Description, this.maxMetricPointsPerMetricStream);
                     this.metrics[index] = metric;
                     this.metricStreamNames.Add(metricStreamName);
                     return metric;
@@ -131,7 +131,7 @@ internal List<Metric> AddMetricsListWithViews(Instrument instrument, List<Metric
                         string[] tagKeysInteresting = metricStreamConfig?.TagKeys;
                         double[] histogramBucketBounds = (metricStreamConfig is ExplicitBucketHistogramConfiguration histogramConfig
                             && histogramConfig.Boundaries != null) ? histogramConfig.Boundaries : null;
-                        metric = new Metric(instrument, this.preferredAggregationTemporality, metricName, metricDescription, this.maxMetricPointsPerMetricStream, histogramBucketBounds, tagKeysInteresting);
+                        metric = new Metric(instrument, this.Temporality, metricName, metricDescription, this.maxMetricPointsPerMetricStream, histogramBucketBounds, tagKeysInteresting);
 
                         this.metrics[index] = metric;
                         metrics.Add(metric);

diff --git a/test/Benchmarks/Metrics/MetricCollectBenchmarks.cs b/test/Benchmarks/Metrics/MetricCollectBenchmarks.cs
@@ -88,7 +88,7 @@ void ProcessExport(Batch<Metric> batch)
 
             this.reader = new BaseExportingMetricReader(metricExporter)
             {
-                PreferredAggregationTemporality = AggregationTemporality.Cumulative,
+                Temporality = AggregationTemporality.Cumulative,
             };
 
             this.meter = new Meter(Utils.GetCurrentMethodName());

diff --git a/test/Benchmarks/Metrics/MetricsBenchmarks.cs b/test/Benchmarks/Metrics/MetricsBenchmarks.cs
@@ -71,8 +71,10 @@ public void Setup()
             this.meter = new Meter(Utils.GetCurrentMethodName());
 
             var exportedItems = new List<Metric>();
-            var reader = new PeriodicExportingMetricReader(new InMemoryExporter<Metric>(exportedItems), 1000);
-            reader.PreferredAggregationTemporality = this.AggregationTemporality;
+            var reader = new PeriodicExportingMetricReader(new InMemoryExporter<Metric>(exportedItems), 1000)
+            {
+                Temporality = this.AggregationTemporality,
+            };
             this.provider = Sdk.CreateMeterProviderBuilder()
                 .AddMeter(this.meter.Name) // All instruments from this meter are enabled.
                 .AddReader(reader)

diff --git a/test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/OtlpMetricsExporterTests.cs b/test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/OtlpMetricsExporterTests.cs
@@ -62,7 +62,10 @@ public void ToOtlpResourceMetricsTest(bool includeServiceNameInResource)
             using var provider = Sdk.CreateMeterProviderBuilder()
                 .SetResourceBuilder(resourceBuilder)
                 .AddMeter(meter.Name)
-                .AddReader(new BaseExportingMetricReader(new InMemoryExporter<Metric>(exportedItems)) { PreferredAggregationTemporality = AggregationTemporality.Delta })
+                .AddReader(new BaseExportingMetricReader(new InMemoryExporter<Metric>(exportedItems))
+                {
+                    Temporality = AggregationTemporality.Delta,
+                })
                 .Build();
 
             var counter = meter.CreateCounter<int>("counter");

diff --git a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/MetricTests.cs b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/MetricTests.cs
@@ -62,7 +62,7 @@ public async Task RequestMetricIsCaptured()
 
             var metricReader = new BaseExportingMetricReader(metricExporter)
             {
-                PreferredAggregationTemporality = AggregationTemporality.Cumulative,
+                Temporality = AggregationTemporality.Cumulative,
             };
             this.meterProvider = Sdk.CreateMeterProviderBuilder()
                 .AddAspNetCoreInstrumentation()

diff --git a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.netcore31.cs b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.netcore31.cs
@@ -60,7 +60,7 @@ public async Task HttpOutCallsAreCollectedSuccessfullyAsync(HttpTestData.HttpOut
 
             var metricReader = new BaseExportingMetricReader(metricExporter)
             {
-                PreferredAggregationTemporality = AggregationTemporality.Cumulative,
+                Temporality = AggregationTemporality.Cumulative,
             };
             var meterProvider = Sdk.CreateMeterProviderBuilder()
                 .AddHttpClientInstrumentation()

diff --git a/test/OpenTelemetry.Tests/Metrics/InMemoryExporterTests.cs b/test/OpenTelemetry.Tests/Metrics/InMemoryExporterTests.cs
@@ -27,14 +27,14 @@ public class InMemoryExporterTests
         [Fact(Skip = "To be run after https://github.com/open-telemetry/opentelemetry-dotnet/issues/2361 is fixed")]
         public void InMemoryExporterShouldDeepCopyMetricPoints()
         {
-            var metrics = new List<Metric>();
+            var exportedItems = new List<Metric>();
 
             using var meter = new Meter(Utils.GetCurrentMethodName());
             using var meterProvider = Sdk.CreateMeterProviderBuilder()
                 .AddMeter(meter.Name)
-                .AddReader(new BaseExportingMetricReader(new InMemoryExporter<Metric>(metrics))
+                .AddReader(new BaseExportingMetricReader(new InMemoryExporter<Metric>(exportedItems))
                 {
-                    PreferredAggregationTemporality = AggregationTemporality.Delta,
+                    Temporality = AggregationTemporality.Delta,
                 })
                 .Build();
 
@@ -45,7 +45,7 @@ public void InMemoryExporterShouldDeepCopyMetricPoints()
 
             meterProvider.ForceFlush();
 
-            var metric = metrics[0]; // Only one Metric object is added to the collection at this point
+            var metric = exportedItems[0]; // Only one Metric object is added to the collection at this point
             var metricPointsEnumerator = metric.GetMetricPoints().GetEnumerator();
             Assert.True(metricPointsEnumerator.MoveNext()); // One MetricPoint is emitted for the Metric
             ref var metricPointForFirstExport = ref metricPointsEnumerator.Current;
@@ -56,7 +56,7 @@ public void InMemoryExporterShouldDeepCopyMetricPoints()
 
             meterProvider.ForceFlush();
 
-            metric = metrics[1]; // Second Metric object is added to the collection at this point
+            metric = exportedItems[1]; // Second Metric object is added to the collection at this point
             metricPointsEnumerator = metric.GetMetricPoints().GetEnumerator();
             Assert.True(metricPointsEnumerator.MoveNext()); // One MetricPoint is emitted for the Metric
             var metricPointForSecondExport = metricPointsEnumerator.Current;

diff --git a/test/OpenTelemetry.Tests/Metrics/MemoryEfficiencyTests.cs b/test/OpenTelemetry.Tests/Metrics/MemoryEfficiencyTests.cs
@@ -38,7 +38,7 @@ public void ExportOnlyWhenPointChanged(AggregationTemporality temporality)
                 .AddReader(
                     new BaseExportingMetricReader(new InMemoryExporter<Metric>(exportedItems))
                     {
-                        PreferredAggregationTemporality = temporality,
+                        Temporality = temporality,
                     })
                 .Build();