diff --git a/src/OpenTelemetry/CHANGELOG.md b/src/OpenTelemetry/CHANGELOG.md index 5bc237cebce..b9f69f91f0d 100644 --- a/src/OpenTelemetry/CHANGELOG.md +++ b/src/OpenTelemetry/CHANGELOG.md @@ -2,6 +2,9 @@ ## Unreleased +* Metrics SDK will not provide inactive Metrics to delta exporter. + ([#2629](https://github.com/open-telemetry/opentelemetry-dotnet/pull/2629)) + * Histogram bounds are validated when added to a View. ([#2573](https://github.com/open-telemetry/opentelemetry-dotnet/pull/2573)) diff --git a/src/OpenTelemetry/Metrics/AggregatorStore.cs b/src/OpenTelemetry/Metrics/AggregatorStore.cs index e035b585b51..12fc07b69c4 100644 --- a/src/OpenTelemetry/Metrics/AggregatorStore.cs +++ b/src/OpenTelemetry/Metrics/AggregatorStore.cs @@ -37,11 +37,13 @@ internal sealed class AggregatorStore private readonly AggregationTemporality temporality; private readonly bool outputDelta; private readonly MetricPoint[] metricPoints; + private readonly int[] currentMetricPointBatch; private readonly AggregationType aggType; private readonly double[] histogramBounds; private readonly UpdateLongDelegate updateLongCallback; private readonly UpdateDoubleDelegate updateDoubleCallback; private int metricPointIndex = 0; + private int batchSize = 0; private bool zeroTagMetricPointInitialized; private DateTimeOffset startTimeExclusive; private DateTimeOffset endTimeInclusive; @@ -53,6 +55,7 @@ internal AggregatorStore( string[] tagKeysInteresting = null) { this.metricPoints = new MetricPoint[MaxMetricPoints]; + this.currentMetricPointBatch = new int[MaxMetricPoints]; this.aggType = aggType; this.temporality = temporality; this.outputDelta = temporality == AggregationTemporality.Delta ? true : false; @@ -92,37 +95,63 @@ internal void Update(double value, ReadOnlySpan> ta this.updateDoubleCallback(value, tags); } - internal void SnapShot() + internal int SnapShot() { + this.batchSize = 0; var indexSnapShot = Math.Min(this.metricPointIndex, MaxMetricPoints - 1); + if (this.temporality == AggregationTemporality.Delta) + { + this.SnapShotDelta(indexSnapShot); + } + else + { + this.SnapShotCumulative(indexSnapShot); + } + this.endTimeInclusive = DateTimeOffset.UtcNow; + return this.batchSize; + } + + internal void SnapShotDelta(int indexSnapShot) + { for (int i = 0; i <= indexSnapShot; i++) { ref var metricPoint = ref this.metricPoints[i]; - if (metricPoint.StartTime == default) + if (metricPoint.MetricPointStatus == MetricPointStatus.NoCollectPending) { continue; } metricPoint.TakeSnapShot(this.outputDelta); + this.currentMetricPointBatch[this.batchSize] = i; + this.batchSize++; } - if (this.temporality == AggregationTemporality.Delta) + if (this.endTimeInclusive != default) + { + this.startTimeExclusive = this.endTimeInclusive; + } + } + + internal void SnapShotCumulative(int indexSnapShot) + { + for (int i = 0; i <= indexSnapShot; i++) { - if (this.endTimeInclusive != default) + ref var metricPoint = ref this.metricPoints[i]; + if (metricPoint.StartTime == default) { - this.startTimeExclusive = this.endTimeInclusive; + continue; } - } - DateTimeOffset dt = DateTimeOffset.UtcNow; - this.endTimeInclusive = dt; + metricPoint.TakeSnapShot(this.outputDelta); + this.currentMetricPointBatch[this.batchSize] = i; + this.batchSize++; + } } internal BatchMetricPoint GetMetricPoints() { - var indexSnapShot = Math.Min(this.metricPointIndex, MaxMetricPoints - 1); - return new BatchMetricPoint(this.metricPoints, indexSnapShot + 1, this.startTimeExclusive, this.endTimeInclusive); + return new BatchMetricPoint(this.metricPoints, this.currentMetricPointBatch, this.batchSize, this.startTimeExclusive, this.endTimeInclusive); } [MethodImpl(MethodImplOptions.AggressiveInlining)] diff --git a/src/OpenTelemetry/Metrics/BatchMetricPoint.cs b/src/OpenTelemetry/Metrics/BatchMetricPoint.cs index 99462f4df84..80d072146be 100644 --- a/src/OpenTelemetry/Metrics/BatchMetricPoint.cs +++ b/src/OpenTelemetry/Metrics/BatchMetricPoint.cs @@ -16,7 +16,6 @@ using System; using System.Collections; -using System.Diagnostics; using OpenTelemetry.Internal; namespace OpenTelemetry.Metrics @@ -24,17 +23,18 @@ namespace OpenTelemetry.Metrics public readonly struct BatchMetricPoint : IDisposable { private readonly MetricPoint[] metricsPoints; + private readonly int[] metricPointsToProcess; private readonly long targetCount; private readonly DateTimeOffset start; private readonly DateTimeOffset end; - internal BatchMetricPoint(MetricPoint[] metricsPoints, int maxSize, DateTimeOffset start, DateTimeOffset end) + internal BatchMetricPoint(MetricPoint[] metricsPoints, int[] metricPointsToProcess, long targetCount, DateTimeOffset start, DateTimeOffset end) { - Debug.Assert(maxSize > 0, $"{nameof(maxSize)} should be a positive number."); Guard.Null(metricsPoints, nameof(metricsPoints)); this.metricsPoints = metricsPoints; - this.targetCount = maxSize; + this.metricPointsToProcess = metricPointsToProcess; + this.targetCount = targetCount; this.start = start; this.end = end; } @@ -50,7 +50,7 @@ public void Dispose() /// . public Enumerator GetEnumerator() { - return new Enumerator(this.metricsPoints, this.targetCount, this.start, this.end); + return new Enumerator(this.metricsPoints, this.metricPointsToProcess, this.targetCount, this.start, this.end); } /// @@ -59,14 +59,16 @@ public Enumerator GetEnumerator() public struct Enumerator : IEnumerator { private readonly MetricPoint[] metricsPoints; + private readonly int[] metricPointsToProcess; private readonly DateTimeOffset start; private readonly DateTimeOffset end; private long targetCount; private long index; - internal Enumerator(MetricPoint[] metricsPoints, long targetCount, DateTimeOffset start, DateTimeOffset end) + internal Enumerator(MetricPoint[] metricsPoints, int[] metricPointsToProcess, long targetCount, DateTimeOffset start, DateTimeOffset end) { this.metricsPoints = metricsPoints; + this.metricPointsToProcess = metricPointsToProcess; this.targetCount = targetCount; this.index = -1; this.start = start; @@ -77,7 +79,7 @@ public ref MetricPoint Current { get { - return ref this.metricsPoints[this.index]; + return ref this.metricsPoints[this.metricPointsToProcess[this.index]]; } } @@ -93,12 +95,7 @@ public bool MoveNext() { while (++this.index < this.targetCount) { - ref var metricPoint = ref this.metricsPoints[this.index]; - if (metricPoint.StartTime == default) - { - continue; - } - + ref var metricPoint = ref this.metricsPoints[this.metricPointsToProcess[this.index]]; metricPoint.StartTime = this.start; metricPoint.EndTime = this.end; return true; diff --git a/src/OpenTelemetry/Metrics/MeterProviderSdk.cs b/src/OpenTelemetry/Metrics/MeterProviderSdk.cs index 25344365e07..85caee70329 100644 --- a/src/OpenTelemetry/Metrics/MeterProviderSdk.cs +++ b/src/OpenTelemetry/Metrics/MeterProviderSdk.cs @@ -443,19 +443,23 @@ internal Batch Collect() for (int i = 0; i < target; i++) { var metric = this.metrics[i]; + int metricPointSize = 0; if (metric != null) { if (metric.InstrumentDisposed) { - metric.SnapShot(); + metricPointSize = metric.SnapShot(); this.metrics[i] = null; } else { - metric.SnapShot(); + metricPointSize = metric.SnapShot(); } - this.metricsCurrentBatch[metricCountCurrentBatch++] = metric; + if (metricPointSize > 0) + { + this.metricsCurrentBatch[metricCountCurrentBatch++] = metric; + } } } diff --git a/src/OpenTelemetry/Metrics/Metric.cs b/src/OpenTelemetry/Metrics/Metric.cs index 50fd75ba8ec..c2f1ad9eeb8 100644 --- a/src/OpenTelemetry/Metrics/Metric.cs +++ b/src/OpenTelemetry/Metrics/Metric.cs @@ -138,9 +138,9 @@ internal void UpdateDouble(double value, ReadOnlySpan +// Copyright The OpenTelemetry Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// + +namespace OpenTelemetry.Metrics +{ + internal enum MetricPointStatus + { + /// + /// This status is applied to s with status after a Collect. + /// If an update occurs, status will be moved to . + /// + NoCollectPending, + + /// + /// The has been updated since the previous Collect cycle. + /// Collect will move it to . + /// + CollectPending, + } +} diff --git a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.netcore31.cs b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.netcore31.cs index 348d4a9e7fe..0172a9eb8f8 100644 --- a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.netcore31.cs +++ b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.netcore31.cs @@ -180,14 +180,7 @@ public async Task HttpOutCallsAreCollectedSuccessfullyAsync(HttpTestData.HttpOut } else { - Assert.Single(requestMetrics); - var metricPoints = new List(); - foreach (var p in requestMetrics[0].GetMetricPoints()) - { - metricPoints.Add(p); - } - - Assert.Empty(metricPoints); + Assert.Empty(requestMetrics); } } diff --git a/test/OpenTelemetry.Tests/Metrics/MemoryEfficiencyTests.cs b/test/OpenTelemetry.Tests/Metrics/MemoryEfficiencyTests.cs index a37e66ab089..e70e5509680 100644 --- a/test/OpenTelemetry.Tests/Metrics/MemoryEfficiencyTests.cs +++ b/test/OpenTelemetry.Tests/Metrics/MemoryEfficiencyTests.cs @@ -24,7 +24,7 @@ namespace OpenTelemetry.Metrics.Tests { public class MemoryEfficiencyTests { - [Theory(Skip = "To be run after https://github.com/open-telemetry/opentelemetry-dotnet/issues/2524 is fixed")] + [Theory] [InlineData(AggregationTemporality.Cumulative)] [InlineData(AggregationTemporality.Delta)] public void ExportOnlyWhenPointChanged(AggregationTemporality temporality) @@ -50,7 +50,14 @@ public void ExportOnlyWhenPointChanged(AggregationTemporality temporality) exportedItems.Clear(); meterProvider.ForceFlush(); - Assert.Empty(exportedItems); + if (temporality == AggregationTemporality.Cumulative) + { + Assert.Single(exportedItems); + } + else + { + Assert.Empty(exportedItems); + } } } } diff --git a/test/OpenTelemetry.Tests/Metrics/MetricAPITest.cs b/test/OpenTelemetry.Tests/Metrics/MetricAPITest.cs index c70b73feff1..630aef73826 100644 --- a/test/OpenTelemetry.Tests/Metrics/MetricAPITest.cs +++ b/test/OpenTelemetry.Tests/Metrics/MetricAPITest.cs @@ -85,7 +85,7 @@ public void ObserverCallbackExceptionTest() meter.CreateObservableGauge("myBadGauge", observeValues: () => throw new Exception("gauge read error")); meterProvider.ForceFlush(MaxTimeToAllowForFlush); - Assert.Equal(2, exportedItems.Count); + Assert.Single(exportedItems); var metric = exportedItems[0]; Assert.Equal("myGauge", metric.Name); List metricPoints = new List(); @@ -99,16 +99,6 @@ public void ObserverCallbackExceptionTest() Assert.Equal(100, metricPoint.LongValue); Assert.NotNull(metricPoint.Keys); Assert.NotNull(metricPoint.Values); - - metric = exportedItems[1]; - Assert.Equal("myBadGauge", metric.Name); - metricPoints.Clear(); - foreach (ref var mp in metric.GetMetricPoints()) - { - metricPoints.Add(mp); - } - - Assert.Empty(metricPoints); } [Theory] @@ -142,6 +132,7 @@ public void StreamNamesDuplicatesAreNotAllowedTest(AggregationTemporality tempor // Metric stream will remain one. var anotherCounterSameName = meter1.CreateCounter("name1"); anotherCounterSameName.Add(10); + counterLong.Add(10); metricItems.Clear(); metricReader.Collect(); Assert.Single(metricItems); @@ -151,6 +142,7 @@ public void StreamNamesDuplicatesAreNotAllowedTest(AggregationTemporality tempor // (the Meter name is not part of stream name) var anotherCounterSameNameDiffMeter = meter2.CreateCounter("name1"); anotherCounterSameNameDiffMeter.Add(10); + counterLong.Add(10); metricItems.Clear(); metricReader.Collect(); Assert.Single(metricItems); @@ -390,22 +382,30 @@ public void TestInstrumentDisposal(AggregationTemporality temporality) Assert.Equal(2, metricItems.Count); metricItems.Clear(); + counter1.Add(10, new KeyValuePair("key", "value")); + counter2.Add(10, new KeyValuePair("key", "value")); meter1.Dispose(); metricReader.Collect(); Assert.Equal(2, metricItems.Count); metricItems.Clear(); + counter1.Add(10, new KeyValuePair("key", "value")); + counter2.Add(10, new KeyValuePair("key", "value")); metricReader.Collect(); Assert.Single(metricItems); metricItems.Clear(); + counter1.Add(10, new KeyValuePair("key", "value")); + counter2.Add(10, new KeyValuePair("key", "value")); meter2.Dispose(); metricReader.Collect(); Assert.Single(metricItems); metricItems.Clear(); + counter1.Add(10, new KeyValuePair("key", "value")); + counter2.Add(10, new KeyValuePair("key", "value")); metricReader.Collect(); Assert.Empty(metricItems); } @@ -458,9 +458,21 @@ int MetricPointCount() Assert.Equal(AggregatorStore.MaxMetricPoints, MetricPointCount()); metricItems.Clear(); + counterLong.Add(10); + for (int i = 0; i < AggregatorStore.MaxMetricPoints + 1; i++) + { + counterLong.Add(10, new KeyValuePair("key", "value" + i)); + } + metricReader.Collect(); Assert.Equal(AggregatorStore.MaxMetricPoints, MetricPointCount()); + counterLong.Add(10); + for (int i = 0; i < AggregatorStore.MaxMetricPoints + 1; i++) + { + counterLong.Add(10, new KeyValuePair("key", "value" + i)); + } + // These updates would be dropped. counterLong.Add(10, new KeyValuePair("key", "valueA")); counterLong.Add(10, new KeyValuePair("key", "valueB"));