Skip to content

Commit

Permalink
Remove subscription to MVC BeforeActionEvent
Browse files Browse the repository at this point in the history
  • Loading branch information
@alanwest
alanwest committed Nov 14, 2023
1 parent 9df1286 commit 2ce8425
Show file tree
Hide file tree
Showing 10 changed files with 213 additions and 407 deletions.
85 changes: 4 additions & 81 deletions src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,6 @@
#endif
using Microsoft.AspNetCore.Http;
#if !NETSTANDARD
using Microsoft.AspNetCore.Mvc.Diagnostics;
using Microsoft.AspNetCore.Mvc.RazorPages;
using Microsoft.AspNetCore.Routing;
#endif
using OpenTelemetry.Context.Propagation;
Expand All @@ -43,7 +41,6 @@ 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";

Expand Down Expand Up @@ -99,12 +96,6 @@ public override void OnEventWritten(string name, object payload)
this.OnStopActivity(Activity.Current, payload);
}

break;
case OnMvcBeforeActionEvent:
{
this.OnMvcBeforeAction(Activity.Current, payload);
}

break;
case OnUnhandledHostingExceptionEvent:
case OnUnHandledDiagnosticsExceptionEvent:
Expand Down Expand Up @@ -295,15 +286,11 @@ public void OnStopActivity(Activity activity, object payload)
var response = context.Response;

#if !NETSTANDARD
var httpRoute = activity.GetTagValue(SemanticConventions.AttributeHttpRoute) as string;
if (string.IsNullOrEmpty(httpRoute))
var routePattern = (context.GetEndpoint() as RouteEndpoint)?.RoutePattern.RawText;
if (!string.IsNullOrEmpty(routePattern))
{
var routePattern = (context.GetEndpoint() as RouteEndpoint)?.RoutePattern.RawText;
if (!string.IsNullOrEmpty(routePattern))
{
activity.DisplayName = this.GetDisplayName(context.Request.Method, routePattern);
activity.SetTag(SemanticConventions.AttributeHttpRoute, routePattern);
}
activity.DisplayName = this.GetDisplayName(context.Request.Method, routePattern);
activity.SetTag(SemanticConventions.AttributeHttpRoute, routePattern);
}
#endif

Expand Down Expand Up @@ -368,70 +355,6 @@ public void OnStopActivity(Activity activity, object payload)
}
}

public void OnMvcBeforeAction(Activity activity, object payload)
{
// 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<IHttpActivityFeature>();
// 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))
{
break;
}

activity = activity.Parent;
}

if (activity == null)
{
return;
}

if (activity.IsAllDataRequested)
{
#if NETSTANDARD
_ = this.beforeActionActionDescriptorFetcher.TryFetch(payload, out var actionDescriptor);
_ = this.beforeActionAttributeRouteInfoFetcher.TryFetch(actionDescriptor, out var attributeRouteInfo);
_ = this.beforeActionTemplateFetcher.TryFetch(attributeRouteInfo, out var 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;
#else
var beforeActionEventData = payload as BeforeActionEventData;
var httpContext = beforeActionEventData.HttpContext;
var actionDescriptor = beforeActionEventData.ActionDescriptor;
var template = actionDescriptor?.AttributeRouteInfo?.Template;

// The template will be null when application does not use attribute routing
// For attribute routing, the DisplayName and http.route will be set when
// the activity ends.
if (actionDescriptor != null && (string.IsNullOrEmpty(template) || actionDescriptor is PageActionDescriptor))
{
var httpRoute = HttpTagHelper.GetHttpRouteFromActionDescriptor(httpContext, actionDescriptor);
activity.DisplayName = this.GetDisplayName(httpContext.Request.Method, httpRoute);
activity.SetTag(SemanticConventions.AttributeHttpRoute, httpRoute);
}
#endif
}
}

public void OnException(Activity activity, object payload)
{
if (activity.IsAllDataRequested)
Expand Down
31 changes: 0 additions & 31 deletions src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpTagHelper.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,44 +14,13 @@
// limitations under the License.
// </copyright>

using System.Text.RegularExpressions;
#if !NETSTANDARD
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Mvc.Abstractions;
using Microsoft.AspNetCore.Mvc.Controllers;
using Microsoft.AspNetCore.Mvc.RazorPages;
using Microsoft.AspNetCore.Routing;
#endif

namespace OpenTelemetry.Instrumentation.AspNetCore.Implementation;

/// <summary>
/// A collection of helper methods to be used when building Http activities.
/// </summary>
internal static class HttpTagHelper
{
private static readonly Regex ControllerNameRegex = new(@"\{controller=.*?\}+?", RegexOptions.Compiled);
private static readonly Regex ActionNameRegex = new(@"\{action=.*?\}+?", RegexOptions.Compiled);

#if !NETSTANDARD
public static string GetHttpRouteFromActionDescriptor(HttpContext httpContext, ActionDescriptor actionDescriptor)
{
var result = (httpContext.GetEndpoint() as RouteEndpoint)?.RoutePattern.RawText;

if (actionDescriptor is ControllerActionDescriptor cad)
{
result = ControllerNameRegex.Replace(result, cad.ControllerName);
result = ActionNameRegex.Replace(result, cad.ActionName);
}
else if (actionDescriptor is PageActionDescriptor pad)
{
result = pad.ViewEnginePath;
}

return result;
}
#endif

/// <summary>
/// Gets the OpenTelemetry standard version tag value for a span based on its protocol/>.
/// </summary>
Expand Down
20 changes: 4 additions & 16 deletions test/OpenTelemetry.Instrumentation.AspNetCore.Tests/BasicTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -861,12 +861,6 @@ void ConfigureTestServices(IServiceCollection services)
numberofSubscribedEvents++;
}

break;
case HttpInListener.OnMvcBeforeActionEvent:
{
numberofSubscribedEvents++;
}

break;
default:
{
Expand Down Expand Up @@ -895,8 +889,8 @@ void ConfigureTestServices(IServiceCollection services)
using var response = await client.SendAsync(request).ConfigureAwait(false);
}

Assert.Equal(0, numberOfUnSubscribedEvents);
Assert.Equal(3, numberofSubscribedEvents);
Assert.Equal(1, numberOfUnSubscribedEvents);
Assert.Equal(2, numberofSubscribedEvents);
}

[Fact]
Expand Down Expand Up @@ -926,12 +920,6 @@ void ConfigureTestServices(IServiceCollection services)
numberofSubscribedEvents++;
}

break;
case HttpInListener.OnMvcBeforeActionEvent:
{
numberofSubscribedEvents++;
}

break;

// TODO: Add test case for validating name for both the types
Expand Down Expand Up @@ -979,8 +967,8 @@ void ConfigureTestServices(IServiceCollection services)
}

Assert.Equal(1, numberOfExceptionCallbacks);
Assert.Equal(0, numberOfUnSubscribedEvents);
Assert.Equal(4, numberofSubscribedEvents);
Assert.Equal(1, numberOfUnSubscribedEvents);
Assert.Equal(3, numberofSubscribedEvents);
}

[Fact(Skip = "https://github.com/open-telemetry/opentelemetry-dotnet/issues/4884")]
Expand Down
Loading

0 comments on commit 2ce8425

Please sign in to comment.