From ce8c21ef571e1e02c73bd58ace68cd0253ffe658 Mon Sep 17 00:00:00 2001 From: Muhammad Othman Date: Tue, 10 Sep 2024 00:51:25 -0400 Subject: [PATCH] [OpenTelemetry.Instrumentation.AWS] Fix Memory Leak by Reusing ActivitySources, Meters, and Instruments (#2039) --- .../CHANGELOG.md | 2 ++ .../Implementation/Metrics/AWSHistogram.cs | 19 ++++++++++++-- .../Implementation/Metrics/AWSMeter.cs | 9 +++---- .../Metrics/AWSMeterProvider.cs | 15 +++++++++-- .../Metrics/AWSMonotonicCounter.cs | 25 +++++++++++++++---- .../Metrics/AWSUpDownCounter.cs | 19 ++++++++++++-- .../Implementation/Tracing/AWSTracer.cs | 6 ++--- .../Tracing/AWSTracerProvider.cs | 15 ++++++++++- .../TestAWSClientMetricsInstrumentation.cs | 5 ++-- 9 files changed, 92 insertions(+), 23 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.AWS/CHANGELOG.md b/src/OpenTelemetry.Instrumentation.AWS/CHANGELOG.md index 9d9e3ea1b8..fb0d930d16 100644 --- a/src/OpenTelemetry.Instrumentation.AWS/CHANGELOG.md +++ b/src/OpenTelemetry.Instrumentation.AWS/CHANGELOG.md @@ -2,6 +2,8 @@ ## Unreleased +* Fix Memory Leak by Reusing ActivitySources, Meters, and Instruments + ([#2039](https://github.com/open-telemetry/opentelemetry-dotnet-contrib/pull/2039)) * Added instrumentation support for AWS Bedrock, BedrockRuntime, BedrockAgent, BedrockAgentRuntime. ([#1979](https://github.com/open-telemetry/opentelemetry-dotnet-contrib/pull/1979)) diff --git a/src/OpenTelemetry.Instrumentation.AWS/Implementation/Metrics/AWSHistogram.cs b/src/OpenTelemetry.Instrumentation.AWS/Implementation/Metrics/AWSHistogram.cs index b4bcec7936..dbab867b3f 100644 --- a/src/OpenTelemetry.Instrumentation.AWS/Implementation/Metrics/AWSHistogram.cs +++ b/src/OpenTelemetry.Instrumentation.AWS/Implementation/Metrics/AWSHistogram.cs @@ -1,6 +1,7 @@ // Copyright The OpenTelemetry Authors // SPDX-License-Identifier: Apache-2.0 +using System.Collections.Concurrent; using Amazon.Runtime.Telemetry; using Amazon.Runtime.Telemetry.Metrics; @@ -9,11 +10,25 @@ namespace OpenTelemetry.Instrumentation.AWS.Implementation.Metrics; internal sealed class AWSHistogram : Histogram where T : struct { + private static readonly ConcurrentDictionary> HistogramsDictionary + = new ConcurrentDictionary>(); + private readonly System.Diagnostics.Metrics.Histogram histogram; - public AWSHistogram(System.Diagnostics.Metrics.Histogram histogram) + public AWSHistogram( + System.Diagnostics.Metrics.Meter meter, + string name, + string? units = null, + string? description = null) { - this.histogram = histogram; + if (HistogramsDictionary.TryGetValue(name, out System.Diagnostics.Metrics.Histogram? histogram)) + { + this.histogram = histogram; + } + + this.histogram = HistogramsDictionary.GetOrAdd( + name, + meter.CreateHistogram(name, units, description)); } public override void Record(T value, Attributes? attributes = null) diff --git a/src/OpenTelemetry.Instrumentation.AWS/Implementation/Metrics/AWSMeter.cs b/src/OpenTelemetry.Instrumentation.AWS/Implementation/Metrics/AWSMeter.cs index 14f7ab1c12..db57f44c59 100644 --- a/src/OpenTelemetry.Instrumentation.AWS/Implementation/Metrics/AWSMeter.cs +++ b/src/OpenTelemetry.Instrumentation.AWS/Implementation/Metrics/AWSMeter.cs @@ -24,8 +24,7 @@ public override UpDownCounter CreateUpDownCounter( string? description = null) where T : struct { - var upDownCounter = this.meter.CreateUpDownCounter(name, units, description); - return new AWSUpDownCounter(upDownCounter); + return new AWSUpDownCounter(this.meter, name, units, description); } public override MonotonicCounter CreateMonotonicCounter( @@ -34,8 +33,7 @@ public override MonotonicCounter CreateMonotonicCounter( string? description = null) where T : struct { - var counter = this.meter.CreateCounter(name, units, description); - return new AWSMonotonicCounter(counter); + return new AWSMonotonicCounter(this.meter, name, units, description); } public override Histogram CreateHistogram( @@ -44,8 +42,7 @@ public override Histogram CreateHistogram( string? description = null) where T : struct { - var histogram = this.meter.CreateHistogram(name, units, description); - return new AWSHistogram(histogram); + return new AWSHistogram(this.meter, name, units, description); } protected override void Dispose(bool disposing) diff --git a/src/OpenTelemetry.Instrumentation.AWS/Implementation/Metrics/AWSMeterProvider.cs b/src/OpenTelemetry.Instrumentation.AWS/Implementation/Metrics/AWSMeterProvider.cs index c86388503e..240dc565f3 100644 --- a/src/OpenTelemetry.Instrumentation.AWS/Implementation/Metrics/AWSMeterProvider.cs +++ b/src/OpenTelemetry.Instrumentation.AWS/Implementation/Metrics/AWSMeterProvider.cs @@ -1,6 +1,7 @@ // Copyright The OpenTelemetry Authors // SPDX-License-Identifier: Apache-2.0 +using System.Collections.Concurrent; using Amazon.Runtime.Telemetry; using Amazon.Runtime.Telemetry.Metrics; @@ -8,6 +9,8 @@ namespace OpenTelemetry.Instrumentation.AWS.Implementation.Metrics; internal sealed class AWSMeterProvider : MeterProvider { + private static readonly ConcurrentDictionary MetersDictionary = new ConcurrentDictionary(); + public override Meter GetMeter(string scope, Attributes? attributes = null) { // Passing attributes to the Meter is currently not possible due to version limitations @@ -17,7 +20,15 @@ public override Meter GetMeter(string scope, Attributes? attributes = null) // update OpenTelemetry core component version(s) to `1.9.0` and allow passing tags to // the meter constructor. - var meter = new System.Diagnostics.Metrics.Meter(scope); - return new AWSMeter(meter); + if (MetersDictionary.TryGetValue(scope, out AWSMeter? meter)) + { + return meter; + } + + var awsMeter = MetersDictionary.GetOrAdd( + scope, + new AWSMeter(new System.Diagnostics.Metrics.Meter(scope))); + + return awsMeter; } } diff --git a/src/OpenTelemetry.Instrumentation.AWS/Implementation/Metrics/AWSMonotonicCounter.cs b/src/OpenTelemetry.Instrumentation.AWS/Implementation/Metrics/AWSMonotonicCounter.cs index dcbaba5d07..7de5568996 100644 --- a/src/OpenTelemetry.Instrumentation.AWS/Implementation/Metrics/AWSMonotonicCounter.cs +++ b/src/OpenTelemetry.Instrumentation.AWS/Implementation/Metrics/AWSMonotonicCounter.cs @@ -1,6 +1,7 @@ // Copyright The OpenTelemetry Authors // SPDX-License-Identifier: Apache-2.0 +using System.Collections.Concurrent; using Amazon.Runtime.Telemetry; using Amazon.Runtime.Telemetry.Metrics; @@ -9,11 +10,25 @@ namespace OpenTelemetry.Instrumentation.AWS.Implementation.Metrics; internal sealed class AWSMonotonicCounter : MonotonicCounter where T : struct { - private readonly System.Diagnostics.Metrics.Counter counter; + private static readonly ConcurrentDictionary> MonotonicCountersDictionary + = new ConcurrentDictionary>(); - public AWSMonotonicCounter(System.Diagnostics.Metrics.Counter counter) + private readonly System.Diagnostics.Metrics.Counter monotonicCounter; + + public AWSMonotonicCounter( + System.Diagnostics.Metrics.Meter meter, + string name, + string? units = null, + string? description = null) { - this.counter = counter; + if (MonotonicCountersDictionary.TryGetValue(name, out System.Diagnostics.Metrics.Counter? monotonicCounter)) + { + this.monotonicCounter = monotonicCounter; + } + + this.monotonicCounter = MonotonicCountersDictionary.GetOrAdd( + name, + meter.CreateCounter(name, units, description)); } public override void Add(T value, Attributes? attributes = null) @@ -21,11 +36,11 @@ public override void Add(T value, Attributes? attributes = null) if (attributes != null) { // TODO: remove ToArray call and use when AttributesAsSpan expected to be added at AWS SDK v4. - this.counter.Add(value, attributes.AllAttributes.ToArray()); + this.monotonicCounter.Add(value, attributes.AllAttributes.ToArray()); } else { - this.counter.Add(value); + this.monotonicCounter.Add(value); } } } diff --git a/src/OpenTelemetry.Instrumentation.AWS/Implementation/Metrics/AWSUpDownCounter.cs b/src/OpenTelemetry.Instrumentation.AWS/Implementation/Metrics/AWSUpDownCounter.cs index 02901349ac..2800ebacee 100644 --- a/src/OpenTelemetry.Instrumentation.AWS/Implementation/Metrics/AWSUpDownCounter.cs +++ b/src/OpenTelemetry.Instrumentation.AWS/Implementation/Metrics/AWSUpDownCounter.cs @@ -1,6 +1,7 @@ // Copyright The OpenTelemetry Authors // SPDX-License-Identifier: Apache-2.0 +using System.Collections.Concurrent; using Amazon.Runtime.Telemetry; using Amazon.Runtime.Telemetry.Metrics; @@ -9,11 +10,25 @@ namespace OpenTelemetry.Instrumentation.AWS.Implementation.Metrics; internal sealed class AWSUpDownCounter : UpDownCounter where T : struct { + private static readonly ConcurrentDictionary> UpDownCountersDictionary + = new ConcurrentDictionary>(); + private readonly System.Diagnostics.Metrics.UpDownCounter upDownCounter; - public AWSUpDownCounter(System.Diagnostics.Metrics.UpDownCounter upDownCounter) + public AWSUpDownCounter( + System.Diagnostics.Metrics.Meter meter, + string name, + string? units = null, + string? description = null) { - this.upDownCounter = upDownCounter; + if (UpDownCountersDictionary.TryGetValue(name, out System.Diagnostics.Metrics.UpDownCounter? upDownCounter)) + { + this.upDownCounter = upDownCounter; + } + + this.upDownCounter = UpDownCountersDictionary.GetOrAdd( + name, + meter.CreateUpDownCounter(name, units, description)); } public override void Add(T value, Attributes? attributes = null) diff --git a/src/OpenTelemetry.Instrumentation.AWS/Implementation/Tracing/AWSTracer.cs b/src/OpenTelemetry.Instrumentation.AWS/Implementation/Tracing/AWSTracer.cs index 6c8c1610a6..ecf8418e6a 100644 --- a/src/OpenTelemetry.Instrumentation.AWS/Implementation/Tracing/AWSTracer.cs +++ b/src/OpenTelemetry.Instrumentation.AWS/Implementation/Tracing/AWSTracer.cs @@ -14,10 +14,10 @@ internal sealed class AWSTracer : Tracer /// /// Initializes a new instance of the class. /// - /// The name of the instrumentation scope that uniquely identifies the tracer. - public AWSTracer(string scope) + /// The ActivitySource used for creating and tracking the activities. + public AWSTracer(ActivitySource activitySource) { - this.activitySource = new ActivitySource(scope); + this.activitySource = activitySource ?? throw new ArgumentNullException(nameof(activitySource)); } public override TraceSpan CreateSpan( diff --git a/src/OpenTelemetry.Instrumentation.AWS/Implementation/Tracing/AWSTracerProvider.cs b/src/OpenTelemetry.Instrumentation.AWS/Implementation/Tracing/AWSTracerProvider.cs index 7e11ce0afc..44fa4986f2 100644 --- a/src/OpenTelemetry.Instrumentation.AWS/Implementation/Tracing/AWSTracerProvider.cs +++ b/src/OpenTelemetry.Instrumentation.AWS/Implementation/Tracing/AWSTracerProvider.cs @@ -1,14 +1,27 @@ // Copyright The OpenTelemetry Authors // SPDX-License-Identifier: Apache-2.0 +using System.Collections.Concurrent; +using System.Diagnostics; using Amazon.Runtime.Telemetry.Tracing; namespace OpenTelemetry.Instrumentation.AWS.Implementation.Tracing; internal sealed class AWSTracerProvider : TracerProvider { + private static readonly ConcurrentDictionary TracersDictionary = new ConcurrentDictionary(); + public override Tracer GetTracer(string scope) { - return new AWSTracer(scope); + if (TracersDictionary.TryGetValue(scope, out AWSTracer? awsTracer)) + { + return awsTracer; + } + + awsTracer = TracersDictionary.GetOrAdd( + scope, + new AWSTracer(new ActivitySource(scope))); + + return awsTracer; } } diff --git a/test/OpenTelemetry.Instrumentation.AWS.Tests/TestAWSClientMetricsInstrumentation.cs b/test/OpenTelemetry.Instrumentation.AWS.Tests/TestAWSClientMetricsInstrumentation.cs index 6872c46d0f..5f2f8b922c 100644 --- a/test/OpenTelemetry.Instrumentation.AWS.Tests/TestAWSClientMetricsInstrumentation.cs +++ b/test/OpenTelemetry.Instrumentation.AWS.Tests/TestAWSClientMetricsInstrumentation.cs @@ -186,7 +186,8 @@ public void TestAWSUpDownCounterIsntCalledAfterMeterDispose() var countAmount = 7; var counterName = "TestCounter"; - var meter = AWSConfigs.TelemetryProvider.MeterProvider.GetMeter($"{TelemetryConstants.TelemetryScopePrefix}.TestDisposedMeter"); + var meterName = $"{TelemetryConstants.TelemetryScopePrefix}.TestDisposedMeter"; + var meter = AWSConfigs.TelemetryProvider.MeterProvider.GetMeter(meterName); var counter = meter.CreateUpDownCounter(counterName); meter.Dispose(); @@ -194,7 +195,7 @@ public void TestAWSUpDownCounterIsntCalledAfterMeterDispose() meterProvider.ForceFlush(); - var counterMetric = exportedItems.FirstOrDefault(i => i.Name == counterName); + var counterMetric = exportedItems.FirstOrDefault(i => i.MeterName == meterName && i.Name == counterName); Assert.Null(counterMetric); }