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://github.com/open-telemetry/opentelemetry-dotnet/pull/4890))

* Added a `Tracer` cache inside of `TracerProvider` to prevent repeated calls to
`GetTracer` from leaking memory.
([#4906](https://github.com/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
72 changes: 70 additions & 2 deletions src/OpenTelemetry.Api/Trace/TracerProvider.cs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@

#nullable enable

using System.Diagnostics;
using System.Collections;

namespace OpenTelemetry.Trace;

Expand All @@ -25,6 +25,8 @@ namespace OpenTelemetry.Trace;
/// </summary>
public class TracerProvider : BaseProvider
{
private Hashtable? tracers = new();

/// <summary>
/// Initializes a new instance of the <see cref="TracerProvider"/> class.
/// </summary>
Expand All @@ -44,5 +46,71 @@ 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 = new TracerKey(name, version);
CodeBlanch marked this conversation as resolved.
Show resolved Hide resolved

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));
tracers[key] = 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.Name = name ?? string.Empty;
this.Version = version;
}

public string Name { get; }

public string? Version { get; }
}
}
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 Tracer_Disposed_Becomes_Noop()
CodeBlanch marked this conversation as resolved.
Show resolved Hide resolved
{
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