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

Updating ActivityProcessor #975

Merged
merged 19 commits into from Aug 4, 2020
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -222,7 +222,7 @@ protected async Task SendAsync(Dictionary<string, Batch> batches, CancellationTo
}
}

protected virtual void Dispose(bool isDisposing)
protected virtual void Dispose(bool disposing)
{
try
{
Expand All @@ -232,7 +232,7 @@ protected virtual void Dispose(bool isDisposing)
{
}

if (isDisposing && !this.isDisposed)
if (disposing && !this.isDisposed)
eddynaka marked this conversation as resolved.
Show resolved Hide resolved
{
this.maxFlushIntervalTimer.Dispose();
this.thriftClient.Dispose();
Expand Down
4 changes: 2 additions & 2 deletions src/OpenTelemetry.Exporter.ZPages/ZPagesProcessor.cs
Expand Up @@ -133,8 +133,8 @@ public override Task ForceFlushAsync(CancellationToken cancellationToken)
throw new NotImplementedException();
}

/// <inheritdoc />
public void Dispose()
/// <inheritdoc/>
protected override void Dispose(bool disposing)
{
throw new NotImplementedException();
}
Expand Down
6 changes: 6 additions & 0 deletions src/OpenTelemetry/CHANGELOG.md
Expand Up @@ -2,6 +2,12 @@

## Unreleased

* `ActivityProcessor` implements IDisposable.
* When `Dispose` occurs, it calls `ShutdownAsync`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to highlight that user should override their Dispose method, if they want to achieve custom behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just updated

* `BatchingActivityProcessor`/`SimpleActivityProcessor` is disposable and it
disposes the containing exporter.
* `BroadcastActivityProcessor`is disposable and it disposes the processors.

## 0.3.0-beta

Released 2020-07-23
Expand Down
24 changes: 23 additions & 1 deletion src/OpenTelemetry/Trace/ActivityProcessor.cs
Expand Up @@ -13,16 +13,19 @@
// See the License for the specific language governing permissions and
// limitations under the License.
// </copyright>

using System;
using System.Diagnostics;
using System.Threading;
using System.Threading.Tasks;
using OpenTelemetry.Internal;

namespace OpenTelemetry.Trace
{
/// <summary>
/// Activity processor base class.
/// </summary>
public abstract class ActivityProcessor
public abstract class ActivityProcessor : IDisposable
eddynaka marked this conversation as resolved.
Show resolved Hide resolved
eddynaka marked this conversation as resolved.
Show resolved Hide resolved
{
/// <summary>
/// Activity start hook.
Expand All @@ -49,5 +52,24 @@ public abstract class ActivityProcessor
/// <param name="cancellationToken">Cancellation token.</param>
/// <returns>Returns <see cref="Task"/>.</returns>
public abstract Task ForceFlushAsync(CancellationToken cancellationToken);

/// <inheritdoc/>
public void Dispose()
{
this.Dispose(true);
GC.SuppressFinalize(this);
}

protected virtual void Dispose(bool disposing)
{
try
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this block should only be called when disposing == true?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in our actual code, it's doing every time. So, I'm not sure.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think typically Close/Shutdown call goes outside of the check. Eg:

protected virtual void Dispose(bool disposing)
{
   Close();

   if (disposing)
   {
       // Free managed resources.
   }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

after I moved, i saw some problems from BroadcastActivityProcessor:

protected override void Dispose(bool disposing)
        {
            if (disposing && !this.disposed)
            {
                foreach (var processor in this.processors)
                {
                    try
                    {
                        if (processor is IDisposable disposable)
                        {
                            disposable.Dispose();
                        }
                    }
                    catch (Exception e)
                    {
                        OpenTelemetrySdkEventSource.Log.SpanProcessorException("Dispose", e);
                    }
                }

                this.disposed = true;
            }

            base.Dispose(disposing);
        }

we are disposing the processors and, then, the base.dispose will call the shutdown...

I will revert to the base calling before, and we can think in another way.

{
this.ShutdownAsync(CancellationToken.None).GetAwaiter().GetResult();
}
catch (Exception ex)
{
OpenTelemetrySdkEventSource.Log.SpanProcessorException(nameof(this.Dispose), ex);
}
}
}
}
17 changes: 2 additions & 15 deletions src/OpenTelemetry/Trace/BatchingActivityProcessor.cs
Expand Up @@ -202,26 +202,13 @@ public override async Task ForceFlushAsync(CancellationToken cancellationToken)
OpenTelemetrySdkEventSource.Log.ForceFlushCompleted(this.currentQueueSize);
}

/// <inheritdoc/>
public void Dispose()
{
this.Dispose(true);
}

/// <summary>
/// Releases the unmanaged resources used by this class and optionally releases the managed resources.
/// </summary>
/// <param name="disposing"><see langword="true"/> to release both managed and unmanaged resources; <see langword="false"/> to release only unmanaged resources.</param>
protected virtual void Dispose(bool disposing)
protected override void Dispose(bool disposing)
eddynaka marked this conversation as resolved.
Show resolved Hide resolved
{
try
{
this.ShutdownAsync(CancellationToken.None).GetAwaiter().GetResult();
}
catch (Exception ex)
{
OpenTelemetrySdkEventSource.Log.SpanProcessorException(nameof(this.Dispose), ex);
}
base.Dispose(disposing);

if (disposing && !this.isDisposed)
{
Expand Down
16 changes: 2 additions & 14 deletions src/OpenTelemetry/Trace/Internal/BroadcastActivityProcessor.cs
Expand Up @@ -96,21 +96,9 @@ public override Task ForceFlushAsync(CancellationToken cancellationToken)
return Task.WhenAll(tasks);
}

public void Dispose()
protected override void Dispose(bool disposing)
{
this.Dispose(true);
}

protected virtual void Dispose(bool disposing)
{
try
{
this.ShutdownAsync(CancellationToken.None).GetAwaiter().GetResult();
}
catch (Exception ex)
{
OpenTelemetrySdkEventSource.Log.SpanProcessorException(nameof(this.Dispose), ex);
}
base.Dispose(disposing);
eddynaka marked this conversation as resolved.
Show resolved Hide resolved

if (disposing && !this.isDisposed)
{
Expand Down
17 changes: 2 additions & 15 deletions src/OpenTelemetry/Trace/SimpleActivityProcessor.cs
Expand Up @@ -85,26 +85,13 @@ public override Task ForceFlushAsync(CancellationToken cancellationToken)
#endif
}

/// <inheritdoc/>
public void Dispose()
{
this.Dispose(true);
}

/// <summary>
/// Releases the unmanaged resources used by this class and optionally releases the managed resources.
/// </summary>
/// <param name="disposing"><see langword="true"/> to release both managed and unmanaged resources; <see langword="false"/> to release only unmanaged resources.</param>
protected virtual void Dispose(bool disposing)
protected override void Dispose(bool disposing)
{
try
{
this.ShutdownAsync(CancellationToken.None).GetAwaiter().GetResult();
}
catch (Exception ex)
{
OpenTelemetrySdkEventSource.Log.SpanProcessorException(nameof(this.Dispose), ex);
}
base.Dispose(disposing);

if (disposing)
{
Expand Down
Expand Up @@ -73,7 +73,7 @@ public override Task ForceFlushAsync(CancellationToken cancellationToken)
#endif
}

public void Dispose()
protected override void Dispose(bool disposing)
{
this.DisposedCalled = true;
}
Expand Down
Expand Up @@ -73,7 +73,7 @@ public override Task ForceFlushAsync(CancellationToken cancellationToken)
#endif
}

public void Dispose()
protected override void Dispose(bool disposing)
{
this.DisposedCalled = true;
}
Expand Down
Expand Up @@ -74,7 +74,7 @@ public override Task ForceFlushAsync(CancellationToken cancellationToken)
#endif
}

public void Dispose()
protected override void Dispose(bool disposing)
{
this.DisposedCalled = true;
}
Expand Down
Expand Up @@ -73,7 +73,7 @@ public override Task ForceFlushAsync(CancellationToken cancellationToken)
#endif
}

public void Dispose()
protected override void Dispose(bool disposing)
{
this.DisposedCalled = true;
}
Expand Down
Expand Up @@ -194,7 +194,7 @@ public void Dispose()
var currentActivity = Activity.Current;

Activity span;
Assert.Equal(2, activityProcessor.Invocations.Count); // begin and end was called
Assert.Equal(3, activityProcessor.Invocations.Count); // begin/end/dispose was called
span = (Activity)activityProcessor.Invocations[1].Arguments[0];

Assert.Equal(routeTemplate ?? HttpContext.Current.Request.Path, span.DisplayName);
Expand Down
Expand Up @@ -66,7 +66,7 @@ public void GrpcClientCallsAreCollectedSuccessfully(string baseAddress)
var rs = client.SayHello(new HelloRequest());
}

Assert.Equal(2, spanProcessor.Invocations.Count); // begin and end was called
Assert.Equal(3, spanProcessor.Invocations.Count); // start/end/dispose was called
var span = (Activity)spanProcessor.Invocations[1].Arguments[0];

Assert.Equal(parent.TraceId, span.Context.TraceId);
Expand Down Expand Up @@ -117,7 +117,7 @@ public void GrpcAndHttpClientInstrumentationIsInvoked()
var rs = client.SayHello(new HelloRequest());
}

Assert.Equal(4, spanProcessor.Invocations.Count); // begin and end was called for Grpc call and underlying Http call
Assert.Equal(5, spanProcessor.Invocations.Count); // begin and end was called for Grpc call and underlying Http call + dispose
var httpSpan = (Activity)spanProcessor.Invocations[2].Arguments[0];
var grpcSpan = (Activity)spanProcessor.Invocations[3].Arguments[0];

Expand Down
Expand Up @@ -98,7 +98,7 @@ public async Task HttpClientInstrumentationInjectsHeadersAsync()
await c.SendAsync(request);
}

Assert.Equal(2, spanProcessor.Invocations.Count); // begin and end was called
Assert.Equal(3, spanProcessor.Invocations.Count); // start/end/dispose was called
var span = (Activity)spanProcessor.Invocations[1].Arguments[0];

Assert.Equal(parent.TraceId, span.Context.TraceId);
Expand Down Expand Up @@ -151,7 +151,7 @@ public async Task HttpClientInstrumentationInjectsHeadersAsync_CustomFormat()
await c.SendAsync(request);
}

Assert.Equal(2, spanProcessor.Invocations.Count); // begin and end was called
Assert.Equal(3, spanProcessor.Invocations.Count); // start/end/dispose was called
var span = (Activity)spanProcessor.Invocations[1].Arguments[0];

Assert.Equal(parent.TraceId, span.Context.TraceId);
Expand Down Expand Up @@ -225,7 +225,7 @@ public async Task HttpClientInstrumentationBacksOffIfAlreadyInstrumented()
await c.SendAsync(request);
}

Assert.Equal(0, spanProcessor.Invocations.Count);
Assert.Equal(1, spanProcessor.Invocations.Count); // dispose
}

[Fact]
Expand All @@ -243,7 +243,7 @@ public async void HttpClientInstrumentationFiltersOutRequests()
await c.GetAsync(this.url);
}

Assert.Equal(0, spanProcessor.Invocations.Count);
Assert.Equal(1, spanProcessor.Invocations.Count); // dispose
}

[Fact]
Expand All @@ -268,7 +268,7 @@ public async Task HttpClientInstrumentationFiltersOutRequestsToExporterEndpoints
}
}

Assert.Equal(0, spanProcessor.Invocations.Count);
Assert.Equal(1, spanProcessor.Invocations.Count); // dispose
}

public void Dispose()
Expand Down
Expand Up @@ -85,7 +85,7 @@ public async Task HttpOutCallsAreCollectedSuccessfullyAsync(HttpTestData.HttpOut
}
}

Assert.Equal(2, spanProcessor.Invocations.Count); // begin and end was called
Assert.Equal(3, spanProcessor.Invocations.Count); // start/end/dispose was called
var span = (Activity)spanProcessor.Invocations[1].Arguments[0];

Assert.Equal(tc.SpanName, span.DisplayName);
Expand Down
Expand Up @@ -173,7 +173,7 @@ public void SqlClient_BadArgs()
afterExecuteEventData);
}

Assert.Equal(2, spanProcessor.Invocations.Count); // begin and end was called
Assert.Equal(3, spanProcessor.Invocations.Count); // start/end/dispose was called

VerifyActivityData(sqlCommand.CommandType, sqlCommand.CommandText, captureStoredProcedureCommandName, captureTextCommandContent, false, sqlConnection.DataSource, (Activity)spanProcessor.Invocations[1].Arguments[0]);
}
Expand Down Expand Up @@ -219,7 +219,7 @@ public void SqlClientErrorsAreCollectedSuccessfully(string beforeCommand, string
commandErrorEventData);
}

Assert.Equal(2, spanProcessor.Invocations.Count); // begin and end was called
Assert.Equal(3, spanProcessor.Invocations.Count); // begin and end was called

VerifyActivityData(sqlCommand.CommandType, sqlCommand.CommandText, true, false, true, sqlConnection.DataSource, (Activity)spanProcessor.Invocations[1].Arguments[0]);
}
Expand Down
Expand Up @@ -82,7 +82,7 @@ public async Task SuccessfulCommandTest(CommandType commandType, string commandT
{
}

Assert.Equal(2, activityProcessor.Invocations.Count);
Assert.Equal(3, activityProcessor.Invocations.Count);

var activity = (Activity)activityProcessor.Invocations[1].Arguments[0];

Expand Down Expand Up @@ -131,8 +131,8 @@ public async Task SuccessfulCommandTest(CommandType commandType, string commandT
int compositeState = successFlag | isSqlExceptionFlag | synchronousFlag;

fakeSqlEventSource.WriteEndExecuteEvent(objectId, compositeState, sqlExceptionNumber);

Assert.Equal(2, activityProcessor.Invocations.Count);
shutdownSignal.Dispose();
Assert.Equal(3, activityProcessor.Invocations.Count);

var activity = (Activity)activityProcessor.Invocations[1].Arguments[0];

Expand All @@ -153,7 +153,9 @@ public void EventSourceFakeUnknownEventWithNullPayloadTest()

fakeSqlEventSource.WriteUnknownEventWithNullPayload();

Assert.Equal(0, activityProcessor.Invocations.Count);
shutdownSignal.Dispose();

Assert.Single(activityProcessor.Invocations);
}

[Fact]
Expand All @@ -172,7 +174,7 @@ public void EventSourceFakeInvalidPayloadTest()

fakeSqlEventSource.WriteEndExecuteEvent("arg1", "arg2", "arg3", "arg4");

Assert.Equal(0, activityProcessor.Invocations.Count);
Assert.InRange(activityProcessor.Invocations.Count, 0, 1);
}

private static void VerifyActivityData(
Expand Down
Expand Up @@ -75,7 +75,7 @@ public void SuccessfulCommandTest(string value)

// Disposing SDK should flush the Redis profiling session immediately.

Assert.Equal(4, activityProcessor.Invocations.Count);
Assert.Equal(5, activityProcessor.Invocations.Count);

VerifyActivityData((Activity)activityProcessor.Invocations[1].Arguments[0], true, connection.GetEndPoints()[0]);
VerifyActivityData((Activity)activityProcessor.Invocations[3].Arguments[0], false, connection.GetEndPoints()[0]);
Expand Down
Expand Up @@ -73,7 +73,7 @@ public override Task ForceFlushAsync(CancellationToken cancellationToken)
#endif
}

public void Dispose()
protected override void Dispose(bool disposing)
{
this.DisposedCalled = true;
}
Expand Down