Skip to content
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

[api] Fix memory leaks in TracerProvider.GetTracer API #4906

Merged
merged 13 commits into from
Oct 5, 2023
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
override OpenTelemetry.Trace.TracerProvider.Dispose(bool disposing) -> void
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
override OpenTelemetry.Trace.TracerProvider.Dispose(bool disposing) -> void
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
override OpenTelemetry.Trace.TracerProvider.Dispose(bool disposing) -> void
4 changes: 4 additions & 0 deletions src/OpenTelemetry.Api/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@
trace was running (`Activity.Current != null`).
([#4890](https:/open-telemetry/opentelemetry-dotnet/pull/4890))

* Added a `Tracer` cache inside of `TracerProvider` to prevent repeated calls to
`GetTracer` from leaking memory.
([#4906](https:/open-telemetry/opentelemetry-dotnet/pull/4906))

## 1.6.0

Released 2023-Sep-05
Expand Down
10 changes: 6 additions & 4 deletions src/OpenTelemetry.Api/Trace/Tracer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,9 @@ namespace OpenTelemetry.Trace;
/// <remarks>Tracer is a wrapper around <see cref="System.Diagnostics.ActivitySource"/> class.</remarks>
public class Tracer
{
internal readonly ActivitySource ActivitySource;
internal ActivitySource? ActivitySource;

internal Tracer(ActivitySource activitySource)
internal Tracer(ActivitySource? activitySource)
{
this.ActivitySource = activitySource;
}
Expand Down Expand Up @@ -213,7 +213,9 @@ private TelemetrySpan StartSpanHelper(
IEnumerable<Link>? links = null,
DateTimeOffset startTime = default)
{
if (!this.ActivitySource.HasListeners())
var activitySource = this.ActivitySource;

if (!(activitySource?.HasListeners() ?? false))
{
return TelemetrySpan.NoopInstance;
}
Expand All @@ -230,7 +232,7 @@ private TelemetrySpan StartSpanHelper(

try
{
var activity = this.ActivitySource.StartActivity(name, activityKind, parentContext.ActivityContext, initialAttributes?.Attributes ?? null, activityLinks, startTime);
var activity = activitySource.StartActivity(name, activityKind, parentContext.ActivityContext, initialAttributes?.Attributes ?? null, activityLinks, startTime);
return activity == null
? TelemetrySpan.NoopInstance
: new TelemetrySpan(activity);
Expand Down
98 changes: 96 additions & 2 deletions src/OpenTelemetry.Api/Trace/TracerProvider.cs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,10 @@

#nullable enable

using System.Diagnostics;
using System.Collections;
#if NET6_0_OR_GREATER
using System.Diagnostics.CodeAnalysis;
#endif

namespace OpenTelemetry.Trace;

Expand All @@ -25,6 +28,10 @@ namespace OpenTelemetry.Trace;
/// </summary>
public class TracerProvider : BaseProvider
{
[ThreadStatic]
private static TracerKey? threadTracerKey;
private Hashtable? tracers = new();

/// <summary>
/// Initializes a new instance of the <see cref="TracerProvider"/> class.
/// </summary>
Expand All @@ -44,5 +51,92 @@ protected TracerProvider()
/// <param name="version">Version of the instrumentation library.</param>
/// <returns>Tracer instance.</returns>
public Tracer GetTracer(string name, string? version = null)
=> new(new ActivitySource(name ?? string.Empty, version));
{
var tracers = this.tracers;
if (tracers == null)
{
// Note: Returns a no-op Tracer once dispose has been called.
return new(activitySource: null);
}

var key = threadTracerKey == null
? (threadTracerKey = new TracerKey(name, version))
: threadTracerKey.Update(name, version);

if (tracers[key] is not Tracer tracer)
{
lock (tracers)
{
tracer = (tracers[key] as Tracer)!;
if (tracer == null)
CodeBlanch marked this conversation as resolved.
Show resolved Hide resolved
{
if (this.tracers != null)
CodeBlanch marked this conversation as resolved.
Show resolved Hide resolved
{
tracer = new(new(key.Name, key.Version));

// Note: key is copied if we write into the hashtable
// because it must hold onto something immutable.
tracers[key.Copy()] = tracer;
}
}
}
}

return tracer ?? new(activitySource: null);
}

/// <inheritdoc/>
protected override void Dispose(bool disposing)
{
if (disposing)
{
var tracers = Interlocked.CompareExchange(ref this.tracers, null, this.tracers);
CodeBlanch marked this conversation as resolved.
Show resolved Hide resolved
if (tracers != null)
{
lock (tracers)
{
foreach (DictionaryEntry entry in tracers)
{
var tracer = (Tracer)entry.Value!;
var activitySource = tracer.ActivitySource;
tracer.ActivitySource = null;
activitySource?.Dispose();
}

tracers.Clear();
}
}
}

base.Dispose(disposing);
}

private sealed record class TracerKey
{
public TracerKey(string? name, string? version)
{
this.Update(name, version);
}

public string Name { get; private set; }
#if !NET6_0_OR_GREATER
= string.Empty;
#endif

public string? Version { get; private set; }

#if NET6_0_OR_GREATER
[MemberNotNull(nameof(Name))]
#endif
public TracerKey Update(string? name, string? version)
{
this.Name = name ?? string.Empty;
this.Version = version;

return this;
}

public TracerKey Copy()
=> new(this.Name, this.Version);
}
}
26 changes: 26 additions & 0 deletions test/OpenTelemetry.Api.Tests/Trace/TracerTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,32 @@ public void CreateSpan_NotSampled()
Assert.False(span.IsRecording);
}

[Fact]
public void TracerBecomesNoopWhenParentProviderIsDisposedTest()
{
TracerProvider provider = null;
Tracer tracer = null;

using (var tracerProvider = Sdk.CreateTracerProviderBuilder()
.AddSource("mytracer")
.Build())
{
provider = tracerProvider;
tracer = tracerProvider.GetTracer("mytracer");

var span1 = tracer.StartSpan("foo");
Assert.True(span1.IsRecording);
}

var span2 = tracer.StartSpan("foo");
Assert.False(span2.IsRecording);

var tracer2 = provider.GetTracer("mytracer");

var span3 = tracer2.StartSpan("foo");
Assert.False(span3.IsRecording);
}

public void Dispose()
{
Activity.Current = null;
Expand Down
Loading