Skip to content

Commit de2174d

Browse files
authored
Fix Client Diagnostics following upgrade to DiagnosticSource 5.0.0 (#5326)
Relates: elastic/apm-agent-dotnet#1094 This commit fixes a rather pernicious bug with Diagnostics from the clients, following the upgrade to System.Diagnostics.DiagnosticSource 5.0.0. `Diagnostic` and `Diagnostic<TState, TStateEnd>` derived from System.Diagnostics.Activity, implementing IDisposable to make them nicer to work with. In System.Diagnostics.DiagnosticSource 5.0.0, Activity now implements IDisposable, so the impl was updated to override `Dispose(bool)`. A problem arises from now deriving from Activity in that for an activity that is not finished, `Dispose()` calls `Stop()` which in turn will notify the ActivitySource that it has stopped **and** sets `Activity.Current` to the parent Activity (null if there's no parent). Now, when calling into overridden `Dispose(bool)`, the DiagnosticSource is notified that the `Activity` has stopped, allowing for listeners to take action. If a listener is getting the Activity through `Activity.Current` as Elastic APM's integration does however, `Activity.Current` no longer represents the Activity that has stopped, but its parent, with no nice way to reference the stopped Activity. This commit removes deriving from `Activity` and introduces a private field in which to hold an Activity that is started, and ended on Dispose. Starting and stopping the activity sets the start and end times, respectively.
1 parent 4e84832 commit de2174d

File tree

1 file changed

+16
-24
lines changed

1 file changed

+16
-24
lines changed

src/Elasticsearch.Net/Diagnostics/Diagnostic.cs

Lines changed: 16 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -5,10 +5,10 @@
55
using System;
66
using System.Diagnostics;
77

8-
namespace Elasticsearch.Net.Diagnostics
8+
namespace Elasticsearch.Net.Diagnostics
99
{
1010
/// <summary>
11-
/// Internal subclass of <see cref="Activity"/> that implements <see cref="IDisposable"/> to
11+
/// Diagnostic that creates, starts and stops <see cref="Activity"/> that implements <see cref="IDisposable"/> to
1212
/// make it easier to use.
1313
/// </summary>
1414
internal class Diagnostic<TState> : Diagnostic<TState, TState>
@@ -17,48 +17,40 @@ public Diagnostic(string operationName, DiagnosticSource source, TState state)
1717
: base(operationName, source, state) =>
1818
EndState = state;
1919
}
20-
21-
internal class Diagnostic<TState, TStateEnd> : Activity
20+
21+
internal class Diagnostic<TState, TStateEnd> : IDisposable
2222
{
2323
public static Diagnostic<TState, TStateEnd> Default { get; } = new();
2424

2525
private readonly DiagnosticSource _source;
26-
private TStateEnd _endState;
2726
private readonly bool _default;
28-
private bool _disposed;
27+
private readonly Activity _activity;
28+
private TStateEnd _endState;
2929

30-
private Diagnostic() : base("__NOOP__") => _default = true;
30+
private Diagnostic() => _default = true;
3131

32-
public Diagnostic(string operationName, DiagnosticSource source, TState state) : base(operationName)
32+
public Diagnostic(string operationName, DiagnosticSource source, TState state)
3333
{
3434
_source = source;
35-
_source.StartActivity(SetStartTime(DateTime.UtcNow), state);
35+
_activity = new Activity(operationName);
36+
_source.StartActivity(_activity, state);
3637
}
3738

3839
public TStateEnd EndState
3940
{
4041
get => _endState;
4142
internal set
4243
{
43-
//do not store state on default instance
44+
//do not store state on default instance
4445
if (_default) return;
45-
_endState = value;
46+
_endState = value;
4647
}
4748
}
48-
49-
protected override void Dispose(bool disposing)
50-
{
51-
if (_disposed) return;
5249

53-
if (disposing)
54-
{
55-
//_source can be null if Default instance
56-
_source?.StopActivity(SetEndTime(DateTime.UtcNow), EndState);
57-
}
58-
59-
_disposed = true;
60-
61-
base.Dispose(disposing);
50+
public void Dispose()
51+
{
52+
_source?.StopActivity(_activity, EndState);
53+
_activity?.Dispose();
6254
}
6355
}
6456
}

0 commit comments

Comments
 (0)