From 8800b2ffdbdc013390e54a0994288111f3e95a54 Mon Sep 17 00:00:00 2001 From: Vishwesh Bankwar Date: Mon, 26 Sep 2022 12:22:41 -0700 Subject: [PATCH] Move all diagnostic source events to a single callback method `OnEventWritten` (#3691) --- .../Implementation/HttpInListener.cs | 118 ++++++++++++------ .../Implementation/HttpInMetricsListener.cs | 101 ++++++++------- .../GrpcClientDiagnosticListener.cs | 26 +++- .../HttpHandlerDiagnosticListener.cs | 35 +++++- .../HttpHandlerMetricsDiagnosticListener.cs | 40 +++--- .../SqlClientDiagnosticListener.cs | 3 +- .../DiagnosticSourceListener.cs | 17 +-- .../ListenerHandler.cs | 30 +---- .../BasicTests.cs | 105 +++++++++------- .../DiagnosticSourceListenerTest.cs | 36 ------ .../Instrumentation/TestListenerHandler.cs | 53 -------- 11 files changed, 272 insertions(+), 292 deletions(-) delete mode 100644 test/OpenTelemetry.Tests/Instrumentation/DiagnosticSourceListenerTest.cs delete mode 100644 test/OpenTelemetry.Tests/Instrumentation/TestListenerHandler.cs diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs index 1fc8e50516b..e8a214203b3 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs @@ -32,16 +32,25 @@ namespace OpenTelemetry.Instrumentation.AspNetCore.Implementation internal class HttpInListener : ListenerHandler { internal const string ActivityOperationName = "Microsoft.AspNetCore.Hosting.HttpRequestIn"; + internal const string OnStartEvent = "Microsoft.AspNetCore.Hosting.HttpRequestIn.Start"; + internal const string OnStopEvent = "Microsoft.AspNetCore.Hosting.HttpRequestIn.Stop"; + internal const string OnMvcBeforeActionEvent = "Microsoft.AspNetCore.Mvc.BeforeAction"; + internal const string OnUnhandledHostingExceptionEvent = "Microsoft.AspNetCore.Hosting.UnhandledException"; + internal const string OnUnHandledDiagnosticsExceptionEvent = "Microsoft.AspNetCore.Diagnostics.UnhandledException"; + #if NET7_0_OR_GREATER // https://github.com/dotnet/aspnetcore/blob/8d6554e655b64da75b71e0e20d6db54a3ba8d2fb/src/Hosting/Hosting/src/GenericHost/GenericWebHostBuilder.cs#L85 internal static readonly string AspNetCoreActivitySourceName = "Microsoft.AspNetCore"; #endif + internal static readonly AssemblyName AssemblyName = typeof(HttpInListener).Assembly.GetName(); internal static readonly string ActivitySourceName = AssemblyName.Name; internal static readonly Version Version = AssemblyName.Version; internal static readonly ActivitySource ActivitySource = new(ActivitySourceName, Version.ToString()); + private const string DiagnosticSourceName = "Microsoft.AspNetCore"; private const string UnknownHostName = "UNKNOWN-HOST"; + private static readonly Func> HttpRequestHeaderValuesGetter = (request, name) => request.Headers[name]; private readonly PropertyFetcher stopExceptionFetcher = new("Exception"); private readonly AspNetCoreInstrumentationOptions options; @@ -54,8 +63,40 @@ public HttpInListener(AspNetCoreInstrumentationOptions options) this.options = options; } + public override void OnEventWritten(string name, object payload) + { + switch (name) + { + case OnStartEvent: + { + this.OnStartActivity(Activity.Current, payload); + } + + break; + case OnStopEvent: + { + this.OnStopActivity(Activity.Current, payload); + } + + break; + case OnMvcBeforeActionEvent: + { + this.OnMvcBeforeAction(Activity.Current, payload); + } + + break; + case OnUnhandledHostingExceptionEvent: + case OnUnHandledDiagnosticsExceptionEvent: + { + this.OnException(Activity.Current, payload); + } + + break; + } + } + [System.Diagnostics.CodeAnalysis.SuppressMessage("Reliability", "CA2000:Dispose objects before losing scope", Justification = "The objects should not be disposed.")] - public override void OnStartActivity(Activity activity, object payload) + public void OnStartActivity(Activity activity, object payload) { // The overall flow of what AspNetCore library does is as below: // Activity.Start() @@ -179,7 +220,7 @@ public override void OnStartActivity(Activity activity, object payload) } } - public override void OnStopActivity(Activity activity, object payload) + public void OnStopActivity(Activity activity, object payload) { if (activity.IsAllDataRequested) { @@ -239,55 +280,52 @@ public override void OnStopActivity(Activity activity, object payload) } } - public override void OnCustom(string name, Activity activity, object payload) + public void OnMvcBeforeAction(Activity activity, object payload) { - if (name == "Microsoft.AspNetCore.Mvc.BeforeAction") + // We cannot rely on Activity.Current here + // There could be activities started by middleware + // after activity started by framework resulting in different Activity.Current. + // so, we need to first find the activity started by Asp.Net Core. + // For .net6.0 onwards we could use IHttpActivityFeature to get the activity created by framework + // var httpActivityFeature = context.Features.Get(); + // activity = httpActivityFeature.Activity; + // However, this will not work as in case of custom propagator + // we start a new activity during onStart event which is a sibling to the activity created by framework + // So, in that case we need to get the activity created by us here. + // we can do so only by looping through activity.Parent chain. + while (activity != null) { - // We cannot rely on Activity.Current here - // There could be activities started by middleware - // after activity started by framework resulting in different Activity.Current. - // so, we need to first find the activity started by Asp.Net Core. - // For .net6.0 onwards we could use IHttpActivityFeature to get the activity created by framework - // var httpActivityFeature = context.Features.Get(); - // activity = httpActivityFeature.Activity; - // However, this will not work as in case of custom propagator - // we start a new activity during onStart event which is a sibling to the activity created by framework - // So, in that case we need to get the activity created by us here. - // we can do so only by looping through activity.Parent chain. - while (activity != null) + if (string.Equals(activity.OperationName, ActivityOperationName, StringComparison.Ordinal)) { - if (string.Equals(activity.OperationName, ActivityOperationName, StringComparison.Ordinal)) - { - break; - } - - activity = activity.Parent; + break; } - if (activity == null) - { - return; - } + activity = activity.Parent; + } - if (activity.IsAllDataRequested) - { - var beforeActionEventData = payload as BeforeActionEventData; - var template = beforeActionEventData.ActionDescriptor?.AttributeRouteInfo?.Template; - if (!string.IsNullOrEmpty(template)) - { - // override the span name that was previously set to the path part of URL. - activity.DisplayName = template; - activity.SetTag(SemanticConventions.AttributeHttpRoute, template); - } + if (activity == null) + { + return; + } - // TODO: Should we get values from RouteData? - // private readonly PropertyFetcher beforeActionRouteDataFetcher = new PropertyFetcher("routeData"); - // var routeData = this.beforeActionRouteDataFetcher.Fetch(payload) as RouteData; + if (activity.IsAllDataRequested) + { + var beforeActionEventData = payload as BeforeActionEventData; + var template = beforeActionEventData.ActionDescriptor?.AttributeRouteInfo?.Template; + if (!string.IsNullOrEmpty(template)) + { + // override the span name that was previously set to the path part of URL. + activity.DisplayName = template; + activity.SetTag(SemanticConventions.AttributeHttpRoute, template); } + + // TODO: Should we get values from RouteData? + // private readonly PropertyFetcher beforeActionRouteDataFetcher = new PropertyFetcher("routeData"); + // var routeData = this.beforeActionRouteDataFetcher.Fetch(payload) as RouteData; } } - public override void OnException(Activity activity, object payload) + public void OnException(Activity activity, object payload) { if (activity.IsAllDataRequested) { diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs index 6c3a150e3a3..5bcae334616 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs @@ -24,6 +24,8 @@ namespace OpenTelemetry.Instrumentation.AspNetCore.Implementation { internal class HttpInMetricsListener : ListenerHandler { + private const string OnStopEvent = "Microsoft.AspNetCore.Hosting.HttpRequestIn.Stop"; + private readonly Meter meter; private readonly Histogram httpServerDuration; @@ -34,65 +36,68 @@ public HttpInMetricsListener(string name, Meter meter) this.httpServerDuration = meter.CreateHistogram("http.server.duration", "ms", "measures the duration of the inbound HTTP request"); } - public override void OnStopActivity(Activity activity, object payload) + public override void OnEventWritten(string name, object payload) { - HttpContext context = payload as HttpContext; - if (context == null) + if (name == OnStopEvent) { - AspNetCoreInstrumentationEventSource.Log.NullPayload(nameof(HttpInMetricsListener), nameof(this.OnStopActivity)); - return; - } + HttpContext context = payload as HttpContext; + if (context == null) + { + AspNetCoreInstrumentationEventSource.Log.NullPayload(nameof(HttpInMetricsListener), nameof(this.OnEventWritten)); + return; + } - // TODO: Prometheus pulls metrics by invoking the /metrics endpoint. Decide if it makes sense to suppress this. - // Below is just a temporary way of achieving this suppression for metrics (we should consider suppressing traces too). - // If we want to suppress activity from Prometheus then we should use SuppressInstrumentationScope. - if (context.Request.Path.HasValue && context.Request.Path.Value.Contains("metrics")) - { - return; - } + // TODO: Prometheus pulls metrics by invoking the /metrics endpoint. Decide if it makes sense to suppress this. + // Below is just a temporary way of achieving this suppression for metrics (we should consider suppressing traces too). + // If we want to suppress activity from Prometheus then we should use SuppressInstrumentationScope. + if (context.Request.Path.HasValue && context.Request.Path.Value.Contains("metrics")) + { + return; + } - string host; + string host; - if (context.Request.Host.Port is null or 80 or 443) - { - host = context.Request.Host.Host; - } - else - { - host = context.Request.Host.Host + ":" + context.Request.Host.Port; - } + if (context.Request.Host.Port is null or 80 or 443) + { + host = context.Request.Host.Host; + } + else + { + host = context.Request.Host.Host + ":" + context.Request.Host.Port; + } - TagList tags; + TagList tags; - var target = (context.GetEndpoint() as RouteEndpoint)?.RoutePattern.RawText; + var target = (context.GetEndpoint() as RouteEndpoint)?.RoutePattern.RawText; - // TODO: This is just a minimal set of attributes. See the spec for additional attributes: - // https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/semantic_conventions/http-metrics.md#http-server - if (!string.IsNullOrEmpty(target)) - { - tags = new TagList + // TODO: This is just a minimal set of attributes. See the spec for additional attributes: + // https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/semantic_conventions/http-metrics.md#http-server + if (!string.IsNullOrEmpty(target)) { - { SemanticConventions.AttributeHttpFlavor, HttpTagHelper.GetFlavorTagValueFromProtocol(context.Request.Protocol) }, - { SemanticConventions.AttributeHttpScheme, context.Request.Scheme }, - { SemanticConventions.AttributeHttpMethod, context.Request.Method }, - { SemanticConventions.AttributeHttpHost, host }, - { SemanticConventions.AttributeHttpTarget, target }, - { SemanticConventions.AttributeHttpStatusCode, context.Response.StatusCode.ToString() }, - }; - } - else - { - tags = new TagList + tags = new TagList + { + { SemanticConventions.AttributeHttpFlavor, HttpTagHelper.GetFlavorTagValueFromProtocol(context.Request.Protocol) }, + { SemanticConventions.AttributeHttpScheme, context.Request.Scheme }, + { SemanticConventions.AttributeHttpMethod, context.Request.Method }, + { SemanticConventions.AttributeHttpHost, host }, + { SemanticConventions.AttributeHttpTarget, target }, + { SemanticConventions.AttributeHttpStatusCode, context.Response.StatusCode.ToString() }, + }; + } + else { - { SemanticConventions.AttributeHttpFlavor, HttpTagHelper.GetFlavorTagValueFromProtocol(context.Request.Protocol) }, - { SemanticConventions.AttributeHttpScheme, context.Request.Scheme }, - { SemanticConventions.AttributeHttpMethod, context.Request.Method }, - { SemanticConventions.AttributeHttpHost, host }, - { SemanticConventions.AttributeHttpStatusCode, context.Response.StatusCode.ToString() }, - }; - } + tags = new TagList + { + { SemanticConventions.AttributeHttpFlavor, HttpTagHelper.GetFlavorTagValueFromProtocol(context.Request.Protocol) }, + { SemanticConventions.AttributeHttpScheme, context.Request.Scheme }, + { SemanticConventions.AttributeHttpMethod, context.Request.Method }, + { SemanticConventions.AttributeHttpHost, host }, + { SemanticConventions.AttributeHttpStatusCode, context.Response.StatusCode.ToString() }, + }; + } - this.httpServerDuration.Record(activity.Duration.TotalMilliseconds, tags); + this.httpServerDuration.Record(Activity.Current.Duration.TotalMilliseconds, tags); + } } } } diff --git a/src/OpenTelemetry.Instrumentation.GrpcNetClient/Implementation/GrpcClientDiagnosticListener.cs b/src/OpenTelemetry.Instrumentation.GrpcNetClient/Implementation/GrpcClientDiagnosticListener.cs index 79b7c691020..299d9ff9f8e 100644 --- a/src/OpenTelemetry.Instrumentation.GrpcNetClient/Implementation/GrpcClientDiagnosticListener.cs +++ b/src/OpenTelemetry.Instrumentation.GrpcNetClient/Implementation/GrpcClientDiagnosticListener.cs @@ -31,6 +31,9 @@ internal sealed class GrpcClientDiagnosticListener : ListenerHandler internal static readonly Version Version = AssemblyName.Version; internal static readonly ActivitySource ActivitySource = new(ActivitySourceName, Version.ToString()); + private const string OnStartEvent = "Grpc.Net.Client.GrpcOut.Start"; + private const string OnStopEvent = "Grpc.Net.Client.GrpcOut.Stop"; + private readonly GrpcClientInstrumentationOptions options; private readonly PropertyFetcher startRequestFetcher = new("Request"); private readonly PropertyFetcher stopRequestFetcher = new("Response"); @@ -41,7 +44,26 @@ public GrpcClientDiagnosticListener(GrpcClientInstrumentationOptions options) this.options = options; } - public override void OnStartActivity(Activity activity, object payload) + public override void OnEventWritten(string name, object payload) + { + switch (name) + { + case OnStartEvent: + { + this.OnStartActivity(Activity.Current, payload); + } + + break; + case OnStopEvent: + { + this.OnStopActivity(Activity.Current, payload); + } + + break; + } + } + + public void OnStartActivity(Activity activity, object payload) { // The overall flow of what GrpcClient library does is as below: // Activity.Start() @@ -137,7 +159,7 @@ public override void OnStartActivity(Activity activity, object payload) } } - public override void OnStopActivity(Activity activity, object payload) + public void OnStopActivity(Activity activity, object payload) { if (activity.IsAllDataRequested) { diff --git a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs index 6e0f982b034..2b1e741906d 100644 --- a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs +++ b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs @@ -35,6 +35,10 @@ internal sealed class HttpHandlerDiagnosticListener : ListenerHandler internal static readonly Version Version = AssemblyName.Version; internal static readonly ActivitySource ActivitySource = new(ActivitySourceName, Version.ToString()); + private const string OnStartEvent = "System.Net.Http.HttpRequestOut.Start"; + private const string OnStopEvent = "System.Net.Http.HttpRequestOut.Stop"; + private const string OnUnhandledExceptionEvent = "System.Net.Http.Exception"; + private readonly PropertyFetcher startRequestFetcher = new("Request"); private readonly PropertyFetcher stopResponseFetcher = new("Response"); private readonly PropertyFetcher stopExceptionFetcher = new("Exception"); @@ -59,7 +63,32 @@ public HttpHandlerDiagnosticListener(HttpClientInstrumentationOptions options) this.options = options; } - public override void OnStartActivity(Activity activity, object payload) + public override void OnEventWritten(string name, object payload) + { + switch (name) + { + case OnStartEvent: + { + this.OnStartActivity(Activity.Current, payload); + } + + break; + case OnStopEvent: + { + this.OnStopActivity(Activity.Current, payload); + } + + break; + case OnUnhandledExceptionEvent: + { + this.OnException(Activity.Current, payload); + } + + break; + } + } + + public void OnStartActivity(Activity activity, object payload) { // The overall flow of what HttpClient library does is as below: // Activity.Start() @@ -148,7 +177,7 @@ public override void OnStartActivity(Activity activity, object payload) } } - public override void OnStopActivity(Activity activity, object payload) + public void OnStopActivity(Activity activity, object payload) { // For .NET7.0 or higher versions, activity is created using activity source // However, the framework will fallback to creating activity if the sampler's decision is to drop and there is a active diagnostic listener. @@ -206,7 +235,7 @@ public override void OnStopActivity(Activity activity, object payload) } } - public override void OnException(Activity activity, object payload) + public void OnException(Activity activity, object payload) { // For .NET7.0 or higher versions, activity is created using activity source // However, the framework will fallback to creating activity if the sampler's decision is to drop and there is a active diagnostic listener. diff --git a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerMetricsDiagnosticListener.cs b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerMetricsDiagnosticListener.cs index 0c2d8691466..2e8234f00db 100644 --- a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerMetricsDiagnosticListener.cs +++ b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerMetricsDiagnosticListener.cs @@ -24,6 +24,8 @@ namespace OpenTelemetry.Instrumentation.Http.Implementation { internal class HttpHandlerMetricsDiagnosticListener : ListenerHandler { + internal const string OnStopEvent = "System.Net.Http.HttpRequestOut.Stop"; + private readonly PropertyFetcher stopResponseFetcher = new("Response"); private readonly Histogram httpClientDuration; @@ -33,28 +35,32 @@ public HttpHandlerMetricsDiagnosticListener(string name, Meter meter) this.httpClientDuration = meter.CreateHistogram("http.client.duration", "ms", "measures the duration of the outbound HTTP request"); } - public override void OnStopActivity(Activity activity, object payload) + public override void OnEventWritten(string name, object payload) { - if (Sdk.SuppressInstrumentation) - { - return; - } - - if (this.stopResponseFetcher.TryFetch(payload, out HttpResponseMessage response) && response != null) + if (name == OnStopEvent) { - var request = response.RequestMessage; + if (Sdk.SuppressInstrumentation) + { + return; + } - // TODO: This is just a minimal set of attributes. See the spec for additional attributes: - // https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/semantic_conventions/http-metrics.md#http-client - var tags = new KeyValuePair[] + var activity = Activity.Current; + if (this.stopResponseFetcher.TryFetch(payload, out HttpResponseMessage response) && response != null) { - new KeyValuePair(SemanticConventions.AttributeHttpMethod, HttpTagHelper.GetNameForHttpMethod(request.Method)), - new KeyValuePair(SemanticConventions.AttributeHttpScheme, request.RequestUri.Scheme), - new KeyValuePair(SemanticConventions.AttributeHttpStatusCode, (int)response.StatusCode), - new KeyValuePair(SemanticConventions.AttributeHttpFlavor, HttpTagHelper.GetFlavorTagValueFromProtocolVersion(request.Version)), - }; + var request = response.RequestMessage; + + // TODO: This is just a minimal set of attributes. See the spec for additional attributes: + // https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/semantic_conventions/http-metrics.md#http-client + var tags = new KeyValuePair[] + { + new KeyValuePair(SemanticConventions.AttributeHttpMethod, HttpTagHelper.GetNameForHttpMethod(request.Method)), + new KeyValuePair(SemanticConventions.AttributeHttpScheme, request.RequestUri.Scheme), + new KeyValuePair(SemanticConventions.AttributeHttpStatusCode, (int)response.StatusCode), + new KeyValuePair(SemanticConventions.AttributeHttpFlavor, HttpTagHelper.GetFlavorTagValueFromProtocolVersion(request.Version)), + }; - this.httpClientDuration.Record(activity.Duration.TotalMilliseconds, tags); + this.httpClientDuration.Record(activity.Duration.TotalMilliseconds, tags); + } } } } diff --git a/src/OpenTelemetry.Instrumentation.SqlClient/Implementation/SqlClientDiagnosticListener.cs b/src/OpenTelemetry.Instrumentation.SqlClient/Implementation/SqlClientDiagnosticListener.cs index 151a4080548..4f360bcbf18 100644 --- a/src/OpenTelemetry.Instrumentation.SqlClient/Implementation/SqlClientDiagnosticListener.cs +++ b/src/OpenTelemetry.Instrumentation.SqlClient/Implementation/SqlClientDiagnosticListener.cs @@ -49,8 +49,9 @@ public SqlClientDiagnosticListener(string sourceName, SqlClientInstrumentationOp public override bool SupportsNullActivity => true; - public override void OnCustom(string name, Activity activity, object payload) + public override void OnEventWritten(string name, object payload) { + var activity = Activity.Current; switch (name) { case SqlDataBeforeExecuteCommand: diff --git a/src/OpenTelemetry/DiagnosticSourceInstrumentation/DiagnosticSourceListener.cs b/src/OpenTelemetry/DiagnosticSourceInstrumentation/DiagnosticSourceListener.cs index 812fc572ced..43a78008272 100644 --- a/src/OpenTelemetry/DiagnosticSourceInstrumentation/DiagnosticSourceListener.cs +++ b/src/OpenTelemetry/DiagnosticSourceInstrumentation/DiagnosticSourceListener.cs @@ -49,22 +49,7 @@ public void OnNext(KeyValuePair value) try { - if (value.Key.EndsWith("Start", StringComparison.Ordinal)) - { - this.handler.OnStartActivity(Activity.Current, value.Value); - } - else if (value.Key.EndsWith("Stop", StringComparison.Ordinal)) - { - this.handler.OnStopActivity(Activity.Current, value.Value); - } - else if (value.Key.EndsWith("Exception", StringComparison.Ordinal)) - { - this.handler.OnException(Activity.Current, value.Value); - } - else - { - this.handler.OnCustom(value.Key, Activity.Current, value.Value); - } + this.handler.OnEventWritten(value.Key, value.Value); } catch (Exception ex) { diff --git a/src/OpenTelemetry/DiagnosticSourceInstrumentation/ListenerHandler.cs b/src/OpenTelemetry/DiagnosticSourceInstrumentation/ListenerHandler.cs index 66ae48e0cab..2c1678715b1 100644 --- a/src/OpenTelemetry/DiagnosticSourceInstrumentation/ListenerHandler.cs +++ b/src/OpenTelemetry/DiagnosticSourceInstrumentation/ListenerHandler.cs @@ -41,40 +41,12 @@ public ListenerHandler(string sourceName) /// public virtual bool SupportsNullActivity { get; } - /// - /// Method called for an event with the suffix 'Start'. - /// - /// The to be started. - /// An object that represent the value being passed as a payload for the event. - public virtual void OnStartActivity(Activity activity, object payload) - { - } - - /// - /// Method called for an event with the suffix 'Stop'. - /// - /// The to be stopped. - /// An object that represent the value being passed as a payload for the event. - public virtual void OnStopActivity(Activity activity, object payload) - { - } - - /// - /// Method called for an event with the suffix 'Exception'. - /// - /// The . - /// An object that represent the value being passed as a payload for the event. - public virtual void OnException(Activity activity, object payload) - { - } - /// /// Method called for an event which does not have 'Start', 'Stop' or 'Exception' as suffix. /// /// Custom name. - /// The to be processed. /// An object that represent the value being passed as a payload for the event. - public virtual void OnCustom(string name, Activity activity, object payload) + public virtual void OnEventWritten(string name, object payload) { } } diff --git a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/BasicTests.cs b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/BasicTests.cs index 4093da8cdc9..2ce0e7989c7 100644 --- a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/BasicTests.cs +++ b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/BasicTests.cs @@ -466,14 +466,24 @@ void ConfigureTestServices(IServiceCollection services) .AddAspNetCoreInstrumentation(new AspNetCoreInstrumentation( new TestHttpInListener(new AspNetCoreInstrumentationOptions()) { - OnStartActivityCallback = (activity, payload) => + OnEventWrittenCallback = (name, payload) => { - baggageCountAfterStart = Baggage.Current.Count; - }, - OnStopActivityCallback = (activity, payload) => - { - baggageCountAfterStop = Baggage.Current.Count; - stopSignal.Set(); + switch (name) + { + case HttpInListener.OnStartEvent: + { + baggageCountAfterStart = Baggage.Current.Count; + } + + break; + case HttpInListener.OnStopEvent: + { + baggageCountAfterStop = Baggage.Current.Count; + stopSignal.Set(); + } + + break; + } }, })) .Build(); @@ -625,7 +635,7 @@ public async Task ActivitiesStartedInMiddlewareBySettingHostActivityToNullShould Assert.Equal(activityName, middlewareActivity.OperationName); Assert.Equal(activityName, middlewareActivity.DisplayName); - // tag http.route should not be added on activity started by asp.net core as it will not be found during oncustom event + // tag http.route should not be added on activity started by asp.net core as it will not be found during OnEventWritten event Assert.DoesNotContain(aspnetcoreframeworkactivity.TagObjects, t => t.Key == SemanticConventions.AttributeHttpRoute); Assert.Equal("Microsoft.AspNetCore.Hosting.HttpRequestIn", aspnetcoreframeworkactivity.OperationName); Assert.Equal("/api/values/2", aspnetcoreframeworkactivity.DisplayName); @@ -707,10 +717,18 @@ void ConfigureTestServices(IServiceCollection services) .AddAspNetCoreInstrumentation(new AspNetCoreInstrumentation( new TestHttpInListener(new AspNetCoreInstrumentationOptions()) { - OnCustomCallback = (name, activity, payload) => + OnEventWrittenCallback = (name, payload) => { - actualCustomEventName = name; - numberOfCustomCallbacks++; + switch (name) + { + case HttpInListener.OnMvcBeforeActionEvent: + { + actualCustomEventName = name; + numberOfCustomCallbacks++; + } + + break; + } }, })) .Build(); @@ -742,9 +760,20 @@ void ConfigureTestServices(IServiceCollection services) .AddAspNetCoreInstrumentation(new AspNetCoreInstrumentation( new TestHttpInListener(new AspNetCoreInstrumentationOptions()) { - OnExceptionCallback = (activity, payload) => + OnEventWrittenCallback = (name, payload) => { - numberOfExceptionCallbacks++; + switch (name) + { + // TODO: Add test case for validating name for both the types + // of exception event. + case HttpInListener.OnUnhandledHostingExceptionEvent: + case HttpInListener.OnUnHandledDiagnosticsExceptionEvent: + { + numberOfExceptionCallbacks++; + } + + break; + } }, })) .Build(); @@ -782,9 +811,18 @@ public async Task DiagnosticSourceExceptionCallBackIsNotReceivedForExceptionsHan .AddAspNetCoreInstrumentation(new AspNetCoreInstrumentation( new TestHttpInListener(new AspNetCoreInstrumentationOptions()) { - OnExceptionCallback = (activity, payload) => + OnEventWrittenCallback = (name, payload) => { - numberOfExceptionCallbacks++; + switch (name) + { + case HttpInListener.OnUnhandledHostingExceptionEvent: + case HttpInListener.OnUnHandledDiagnosticsExceptionEvent: + { + numberOfExceptionCallbacks++; + } + + break; + } }, })) .Build(); @@ -954,45 +992,18 @@ public override SamplingResult ShouldSample(in SamplingParameters samplingParame private class TestHttpInListener : HttpInListener { - public Action OnStartActivityCallback; - - public Action OnStopActivityCallback; - - public Action OnExceptionCallback; - - public Action OnCustomCallback; + public Action OnEventWrittenCallback; public TestHttpInListener(AspNetCoreInstrumentationOptions options) : base(options) { } - public override void OnStartActivity(Activity activity, object payload) - { - base.OnStartActivity(activity, payload); - - this.OnStartActivityCallback?.Invoke(activity, payload); - } - - public override void OnStopActivity(Activity activity, object payload) - { - base.OnStopActivity(activity, payload); - - this.OnStopActivityCallback?.Invoke(activity, payload); - } - - public override void OnCustom(string name, Activity activity, object payload) - { - base.OnCustom(name, activity, payload); - - this.OnCustomCallback?.Invoke(name, activity, payload); - } - - public override void OnException(Activity activity, object payload) + public override void OnEventWritten(string name, object payload) { - base.OnException(activity, payload); + base.OnEventWritten(name, payload); - this.OnExceptionCallback?.Invoke(activity, payload); + this.OnEventWrittenCallback?.Invoke(name, payload); } } @@ -1013,7 +1024,7 @@ public override void PreProcess(HttpContext context) // Setting the host activity i.e. activity started by asp.net core // to null here will have no impact on middleware activity. // This also means that asp.net core activity will not be found - // during OnCustom event. + // during OnEventWritten event. Activity.Current = null; this.activity = this.activitySource.StartActivity(this.activityName); } diff --git a/test/OpenTelemetry.Tests/Instrumentation/DiagnosticSourceListenerTest.cs b/test/OpenTelemetry.Tests/Instrumentation/DiagnosticSourceListenerTest.cs deleted file mode 100644 index 7e9b6fc2de7..00000000000 --- a/test/OpenTelemetry.Tests/Instrumentation/DiagnosticSourceListenerTest.cs +++ /dev/null @@ -1,36 +0,0 @@ -// -// Copyright The OpenTelemetry Authors -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. -// - -using System.Diagnostics; - -namespace OpenTelemetry.Instrumentation.Tests -{ - public class DiagnosticSourceListenerTest - { - private const string TestSourceName = "TestSourceName"; - private readonly DiagnosticSource diagnosticSource; - private readonly TestListenerHandler testListenerHandler; - private readonly DiagnosticSourceSubscriber testDiagnosticSourceSubscriber; - - public DiagnosticSourceListenerTest() - { - this.diagnosticSource = new DiagnosticListener(TestSourceName); - this.testListenerHandler = new TestListenerHandler(TestSourceName); - this.testDiagnosticSourceSubscriber = new DiagnosticSourceSubscriber(this.testListenerHandler, null); - this.testDiagnosticSourceSubscriber.Subscribe(); - } - } -} diff --git a/test/OpenTelemetry.Tests/Instrumentation/TestListenerHandler.cs b/test/OpenTelemetry.Tests/Instrumentation/TestListenerHandler.cs deleted file mode 100644 index 81cb2f4b053..00000000000 --- a/test/OpenTelemetry.Tests/Instrumentation/TestListenerHandler.cs +++ /dev/null @@ -1,53 +0,0 @@ -// -// Copyright The OpenTelemetry Authors -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. -// - -using System.Diagnostics; - -namespace OpenTelemetry.Instrumentation.Tests -{ - internal class TestListenerHandler : ListenerHandler - { - public int OnStartInvokedCount = 0; - public int OnStopInvokedCount = 0; - public int OnExceptionInvokedCount = 0; - public int OnCustomInvokedCount = 0; - - public TestListenerHandler(string sourceName) - : base(sourceName) - { - } - - public override void OnStartActivity(Activity activity, object payload) - { - this.OnStartInvokedCount++; - } - - public override void OnStopActivity(Activity activity, object payload) - { - this.OnStopInvokedCount++; - } - - public override void OnException(Activity activity, object payload) - { - this.OnExceptionInvokedCount++; - } - - public override void OnCustom(string name, Activity activity, object payload) - { - this.OnCustomInvokedCount++; - } - } -}