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

[Instrumentation.AspNetCore] Fix span names when http.request.method is reporter as _OTHER #5484

Merged
merged 8 commits into from
Mar 29, 2024
Merged
4 changes: 4 additions & 0 deletions src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,10 @@
the original value `Get` will be stored in `http.request.method_original`.
([#5471](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5471))

* Fixed the name of spans that have `http.request.method` attribute set to `_OTHER`.
Now, the span name have `HTTP` prefix, previously it was `_OTHER`.
Kielek marked this conversation as resolved.
Show resolved Hide resolved
([#5484](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5484))

## 1.7.1

Released 2024-Feb-09
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,7 @@ public void OnStartActivity(Activity activity, object payload)
}

var path = (request.PathBase.HasValue || request.Path.HasValue) ? (request.PathBase + request.Path).ToString() : "/";
activity.DisplayName = GetDisplayName(request.Method);
RequestMethodHelper.SetActivityDisplayName(activity, request.Method);

// see the spec https://github.com/open-telemetry/semantic-conventions/blob/v1.23.0/docs/http/http-spans.md

Expand Down Expand Up @@ -241,7 +241,7 @@ public void OnStopActivity(Activity activity, object payload)
context.GetEndpoint() as RouteEndpoint)?.RoutePattern.RawText;
if (!string.IsNullOrEmpty(routePattern))
{
activity.DisplayName = GetDisplayName(context.Request.Method, routePattern);
RequestMethodHelper.SetActivityDisplayName(activity, context.Request.Method, routePattern);
activity.SetTag(SemanticConventions.AttributeHttpRoute, routePattern);
}
#endif
Expand Down Expand Up @@ -388,13 +388,4 @@ private static void AddGrpcAttributes(Activity activity, string grpcMethod, Http
}
}
}

private static string GetDisplayName(string httpMethod, string httpRoute = null)
{
var normalizedMethod = RequestMethodHelper.GetNormalizedHttpMethod(httpMethod);

return string.IsNullOrEmpty(httpRoute)
? normalizedMethod
: $"{normalizedMethod} {httpRoute}";
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ public void OnStartActivity(Activity activity, object payload)
return;
}

RequestMethodHelper.SetHttpClientActivityDisplayName(activity, request.Method.Method);
RequestMethodHelper.SetActivityDisplayName(activity, request.Method.Method);

if (!IsNet7OrGreater)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ internal static HttpClientTraceInstrumentationOptions TracingOptions
[MethodImpl(MethodImplOptions.AggressiveInlining)]
private static void AddRequestTagsAndInstrumentRequest(HttpWebRequest request, Activity activity)
{
RequestMethodHelper.SetHttpClientActivityDisplayName(activity, request.Method);
RequestMethodHelper.SetActivityDisplayName(activity, request.Method);

if (activity.IsAllDataRequested)
{
Expand Down
10 changes: 7 additions & 3 deletions src/Shared/RequestMethodHelper.cs
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
// Copyright The OpenTelemetry Authors
// SPDX-License-Identifier: Apache-2.0

#nullable enable

#if NET8_0_OR_GREATER
using System.Collections.Frozen;
#endif
Expand Down Expand Up @@ -62,9 +64,11 @@ public static void SetHttpMethodTag(Activity activity, string originalHttpMethod
}
}

public static void SetHttpClientActivityDisplayName(Activity activity, string method)
public static void SetActivityDisplayName(Activity activity, string method, string? httpRoute = null)
{
// https://github.com/open-telemetry/semantic-conventions/blob/v1.23.0/docs/http/http-spans.md#name
activity.DisplayName = KnownMethods.TryGetValue(method, out var httpMethod) ? httpMethod : "HTTP";
// https://github.com/open-telemetry/semantic-conventions/blob/v1.24.0/docs/http/http-spans.md#name

var namePrefix = KnownMethods.TryGetValue(method, out var httpMethod) ? httpMethod : "HTTP";
activity.DisplayName = string.IsNullOrEmpty(httpRoute) ? namePrefix : $"{namePrefix} {httpRoute}";
Kielek marked this conversation as resolved.
Show resolved Hide resolved
}
}
25 changes: 13 additions & 12 deletions test/OpenTelemetry.Instrumentation.AspNetCore.Tests/BasicTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -629,18 +629,18 @@ void ConfigureTestServices(IServiceCollection services)
}

[Theory]
[InlineData("CONNECT", "CONNECT", null)]
[InlineData("DELETE", "DELETE", null)]
[InlineData("GET", "GET", null)]
[InlineData("PUT", "PUT", null)]
[InlineData("HEAD", "HEAD", null)]
[InlineData("OPTIONS", "OPTIONS", null)]
[InlineData("PATCH", "PATCH", null)]
[InlineData("Get", "GET", "Get")]
[InlineData("POST", "POST", null)]
[InlineData("TRACE", "TRACE", null)]
[InlineData("CUSTOM", "_OTHER", "CUSTOM")]
public async Task HttpRequestMethodIsSetAsPerSpec(string originalMethod, string expectedMethod, string expectedOriginalMethod)
[InlineData("CONNECT", "CONNECT", null, "CONNECT")]
[InlineData("DELETE", "DELETE", null, "DELETE")]
[InlineData("GET", "GET", null, "GET")]
[InlineData("PUT", "PUT", null, "PUT")]
[InlineData("HEAD", "HEAD", null, "HEAD")]
[InlineData("OPTIONS", "OPTIONS", null, "OPTIONS")]
[InlineData("PATCH", "PATCH", null, "PATCH")]
[InlineData("Get", "GET", "Get", "GET")]
[InlineData("POST", "POST", null, "POST")]
[InlineData("TRACE", "TRACE", null, "TRACE")]
[InlineData("CUSTOM", "_OTHER", "CUSTOM", "HTTP")]
public async Task HttpRequestMethodIsSetAsPerSpec(string originalMethod, string expectedMethod, string expectedOriginalMethod, string expectedDisplayName)
Kielek marked this conversation as resolved.
Show resolved Hide resolved
{
var exportedItems = new List<Activity>();

Expand Down Expand Up @@ -683,6 +683,7 @@ void ConfigureTestServices(IServiceCollection services)

Assert.Equal(expectedMethod, activity.GetTagValue(SemanticConventions.AttributeHttpRequestMethod));
Assert.Equal(expectedOriginalMethod, activity.GetTagValue(SemanticConventions.AttributeHttpRequestMethodOriginal));
Assert.Equal(expectedDisplayName, activity.DisplayName);
}

[Fact]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,6 @@ namespace RouteTests;

public class RoutingTests : IClassFixture<RoutingTestFixture>
{
private const string OldHttpStatusCode = "http.status_code";
private const string OldHttpMethod = "http.method";
private const string HttpStatusCode = "http.response.status_code";
private const string HttpMethod = "http.request.method";
private const string HttpRoute = "http.route";
Expand Down