From c60216fdb83295396945a0c789f247a4631bcb16 Mon Sep 17 00:00:00 2001 From: TimothyMothra Date: Thu, 1 Jun 2023 14:40:51 -0700 Subject: [PATCH 01/15] update http semantic conventions for aspnetcore --- .../Internal/SemanticConventions.cs | 13 +++ .../Implementation/HttpInListener.cs | 79 +++++++++++++++---- .../Implementation/HttpInMetricsListener.cs | 38 +++++++-- 3 files changed, 105 insertions(+), 25 deletions(-) diff --git a/src/OpenTelemetry.Api/Internal/SemanticConventions.cs b/src/OpenTelemetry.Api/Internal/SemanticConventions.cs index a9c760378c3..56489c83fb2 100644 --- a/src/OpenTelemetry.Api/Internal/SemanticConventions.cs +++ b/src/OpenTelemetry.Api/Internal/SemanticConventions.cs @@ -110,5 +110,18 @@ internal static class SemanticConventions public const string AttributeExceptionType = "exception.type"; public const string AttributeExceptionMessage = "exception.message"; public const string AttributeExceptionStacktrace = "exception.stacktrace"; + + // NEW v1.21.0 Http Semantic Conventions + public const string AttributeClientSocketPort = "client.socket.port"; + public const string AttributeHttpRequestMethod = "http.request.method"; + public const string AttributeHttpResponseStatusCode = "http.response.status_code"; + public const string AttributeNetworkProtocolVersion = "network.protocol.version"; + public const string AttributeServerAddress = "server.address"; + public const string AttributeServerName = "server.name"; + public const string AttributeServerPort = "server.port"; + public const string AttributeUrlFull = "url.full"; + public const string AttributeUrlPath = "url.path"; + public const string AttributeUrlScheme = "url.scheme"; + public const string AttributeUserAgentOriginal = "user_agent.original"; } } diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs index 537ff5ea42e..5da3633e9cd 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs @@ -197,27 +197,55 @@ public void OnStartActivity(Activity activity, object payload) var path = (request.PathBase.HasValue || request.Path.HasValue) ? (request.PathBase + request.Path).ToString() : "/"; activity.DisplayName = path; - // see the spec https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/http.md - if (request.Host.HasValue) + // see the spec https://github.com/open-telemetry/semantic-conventions/blob/main/specification/trace/semantic_conventions/http.md + if (this.httpSemanticConvention.HasFlag(HttpSemanticConvention.Old)) { - activity.SetTag(SemanticConventions.AttributeNetHostName, request.Host.Host); + if (request.Host.HasValue) + { + activity.SetTag(SemanticConventions.AttributeNetHostName, request.Host.Host); + + if (request.Host.Port is not null && request.Host.Port != 80 && request.Host.Port != 443) + { + activity.SetTag(SemanticConventions.AttributeNetHostPort, request.Host.Port); + } + } - if (request.Host.Port is not null && request.Host.Port != 80 && request.Host.Port != 443) + activity.SetTag(SemanticConventions.AttributeHttpMethod, request.Method); + activity.SetTag(SemanticConventions.AttributeHttpScheme, request.Scheme); + activity.SetTag(SemanticConventions.AttributeHttpTarget, path); + activity.SetTag(SemanticConventions.AttributeHttpUrl, GetUri(request)); + activity.SetTag(SemanticConventions.AttributeHttpFlavor, HttpTagHelper.GetFlavorTagValueFromProtocol(request.Protocol)); + + var userAgent = request.Headers["User-Agent"].FirstOrDefault(); + if (!string.IsNullOrEmpty(userAgent)) { - activity.SetTag(SemanticConventions.AttributeNetHostPort, request.Host.Port); + activity.SetTag(SemanticConventions.AttributeHttpUserAgent, userAgent); } } - activity.SetTag(SemanticConventions.AttributeHttpMethod, request.Method); - activity.SetTag(SemanticConventions.AttributeHttpScheme, request.Scheme); - activity.SetTag(SemanticConventions.AttributeHttpTarget, path); - activity.SetTag(SemanticConventions.AttributeHttpUrl, GetUri(request)); - activity.SetTag(SemanticConventions.AttributeHttpFlavor, HttpTagHelper.GetFlavorTagValueFromProtocol(request.Protocol)); - - var userAgent = request.Headers["User-Agent"].FirstOrDefault(); - if (!string.IsNullOrEmpty(userAgent)) + if (this.httpSemanticConvention.HasFlag(HttpSemanticConvention.New)) { - activity.SetTag(SemanticConventions.AttributeHttpUserAgent, userAgent); + if (request.Host.HasValue) + { + activity.SetTag(SemanticConventions.AttributeServerAddress, request.Host.Host); + + if (request.Host.Port is not null && request.Host.Port != 80 && request.Host.Port != 443) + { + activity.SetTag(SemanticConventions.AttributeServerPort, request.Host.Port); + } + } + + activity.SetTag(SemanticConventions.AttributeHttpRequestMethod, request.Method); + activity.SetTag(SemanticConventions.AttributeUrlScheme, request.Scheme); + activity.SetTag(SemanticConventions.AttributeUrlPath, path); + activity.SetTag(SemanticConventions.AttributeUrlFull, GetUri(request)); + activity.SetTag(SemanticConventions.AttributeNetworkProtocolVersion, HttpTagHelper.GetFlavorTagValueFromProtocol(request.Protocol)); + + var userAgent = request.Headers["User-Agent"].FirstOrDefault(); + if (!string.IsNullOrEmpty(userAgent)) + { + activity.SetTag(SemanticConventions.AttributeUserAgentOriginal, userAgent); + } } try @@ -244,7 +272,15 @@ public void OnStopActivity(Activity activity, object payload) var response = context.Response; - activity.SetTag(SemanticConventions.AttributeHttpStatusCode, TelemetryHelper.GetBoxedStatusCode(response.StatusCode)); + if (this.httpSemanticConvention.HasFlag(HttpSemanticConvention.Old)) + { + activity.SetTag(SemanticConventions.AttributeHttpStatusCode, TelemetryHelper.GetBoxedStatusCode(response.StatusCode)); + } + + if (this.httpSemanticConvention.HasFlag(HttpSemanticConvention.New)) + { + activity.SetTag(SemanticConventions.AttributeHttpResponseStatusCode, TelemetryHelper.GetBoxedStatusCode(response.StatusCode)); + } #if !NETSTANDARD2_0 if (this.options.EnableGrpcAspNetCoreSupport && TryGetGrpcMethod(activity, out var grpcMethod)) @@ -426,7 +462,7 @@ private static bool TryGetGrpcMethod(Activity activity, out string grpcMethod) } [MethodImpl(MethodImplOptions.AggressiveInlining)] - private static void AddGrpcAttributes(Activity activity, string grpcMethod, HttpContext context) + private void AddGrpcAttributes(Activity activity, string grpcMethod, HttpContext context) { // The RPC semantic conventions indicate the span name // should not have a leading forward slash. @@ -436,10 +472,19 @@ private static void AddGrpcAttributes(Activity activity, string grpcMethod, Http activity.SetTag(SemanticConventions.AttributeRpcSystem, GrpcTagHelper.RpcSystemGrpc); if (context.Connection.RemoteIpAddress != null) { + // TODO: This attribute was changed in v1.13.0 https://github.com/open-telemetry/opentelemetry-specification/pull/2614 activity.SetTag(SemanticConventions.AttributeNetPeerIp, context.Connection.RemoteIpAddress.ToString()); } - activity.SetTag(SemanticConventions.AttributeNetPeerPort, context.Connection.RemotePort); + if (this.httpSemanticConvention.HasFlag(HttpSemanticConvention.Old)) + { + activity.SetTag(SemanticConventions.AttributeNetPeerPort, context.Connection.RemotePort); + } + + if (this.httpSemanticConvention.HasFlag(HttpSemanticConvention.New)) + { + activity.SetTag(SemanticConventions.AttributeClientSocketPort, context.Connection.RemotePort); + } bool validConversion = GrpcTagHelper.TryGetGrpcStatusCodeFromActivity(activity, out int status); if (validConversion) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs index 810225daa25..03f00864aaa 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs @@ -82,20 +82,42 @@ public override void OnEventWritten(string name, object payload) TagList tags = default; - tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpFlavor, HttpTagHelper.GetFlavorTagValueFromProtocol(context.Request.Protocol))); - tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpScheme, context.Request.Scheme)); - tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpMethod, context.Request.Method)); - tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpStatusCode, TelemetryHelper.GetBoxedStatusCode(context.Response.StatusCode))); + if (this.httpSemanticConvention.HasFlag(HttpSemanticConvention.Old)) + { + tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpFlavor, HttpTagHelper.GetFlavorTagValueFromProtocol(context.Request.Protocol))); + tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpScheme, context.Request.Scheme)); + tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpMethod, context.Request.Method)); + tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpStatusCode, TelemetryHelper.GetBoxedStatusCode(context.Response.StatusCode))); + + if (context.Request.Host.HasValue) + { + tags.Add(new KeyValuePair(SemanticConventions.AttributeNetHostName, context.Request.Host.Host)); + + if (context.Request.Host.Port is not null && context.Request.Host.Port != 80 && context.Request.Host.Port != 443) + { + tags.Add(new KeyValuePair(SemanticConventions.AttributeNetHostPort, context.Request.Host.Port)); + } + } + } - if (context.Request.Host.HasValue) + if (this.httpSemanticConvention.HasFlag(HttpSemanticConvention.New)) { - tags.Add(new KeyValuePair(SemanticConventions.AttributeNetHostName, context.Request.Host.Host)); + tags.Add(new KeyValuePair(SemanticConventions.AttributeNetworkProtocolVersion, HttpTagHelper.GetFlavorTagValueFromProtocol(context.Request.Protocol))); + tags.Add(new KeyValuePair(SemanticConventions.AttributeUrlScheme, context.Request.Scheme)); + tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpRequestMethod, context.Request.Method)); + tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpResponseStatusCode, TelemetryHelper.GetBoxedStatusCode(context.Response.StatusCode))); - if (context.Request.Host.Port is not null && context.Request.Host.Port != 80 && context.Request.Host.Port != 443) + if (context.Request.Host.HasValue) { - tags.Add(new KeyValuePair(SemanticConventions.AttributeNetHostPort, context.Request.Host.Port)); + tags.Add(new KeyValuePair(SemanticConventions.AttributeServerName, context.Request.Host.Host)); + + if (context.Request.Host.Port is not null && context.Request.Host.Port != 80 && context.Request.Host.Port != 443) + { + tags.Add(new KeyValuePair(SemanticConventions.AttributeServerPort, context.Request.Host.Port)); + } } } + #if NET6_0_OR_GREATER var route = (context.GetEndpoint() as RouteEndpoint)?.RoutePattern.RawText; if (!string.IsNullOrEmpty(route)) From 9b5c80030736b2e4514a4ccf09b7e59651a7ad84 Mon Sep 17 00:00:00 2001 From: TimothyMothra Date: Thu, 1 Jun 2023 14:49:40 -0700 Subject: [PATCH 02/15] prefix local calls with this --- .../Implementation/HttpInListener.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs index 5da3633e9cd..1c1f74f1559 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs @@ -285,7 +285,7 @@ public void OnStopActivity(Activity activity, object payload) #if !NETSTANDARD2_0 if (this.options.EnableGrpcAspNetCoreSupport && TryGetGrpcMethod(activity, out var grpcMethod)) { - AddGrpcAttributes(activity, grpcMethod, context); + this.AddGrpcAttributes(activity, grpcMethod, context); } else if (activity.Status == ActivityStatusCode.Unset) { From 9215a07736ed234e6c23496bb7620e20e2197e82 Mon Sep 17 00:00:00 2001 From: Timothy Mothra Lee Date: Tue, 6 Jun 2023 14:16:05 -0700 Subject: [PATCH 03/15] pr feedback --- .../Internal/SemanticConventions.cs | 1 + .../Implementation/HttpInListener.cs | 9 +- ...stsCollectionsIsAccordingToTheSpecTests.cs | 265 +++++++++++++----- 3 files changed, 204 insertions(+), 71 deletions(-) diff --git a/src/OpenTelemetry.Api/Internal/SemanticConventions.cs b/src/OpenTelemetry.Api/Internal/SemanticConventions.cs index 56489c83fb2..52a08634606 100644 --- a/src/OpenTelemetry.Api/Internal/SemanticConventions.cs +++ b/src/OpenTelemetry.Api/Internal/SemanticConventions.cs @@ -122,6 +122,7 @@ internal static class SemanticConventions public const string AttributeUrlFull = "url.full"; public const string AttributeUrlPath = "url.path"; public const string AttributeUrlScheme = "url.scheme"; + public const string AttributeUrlQuery = "url.query"; public const string AttributeUserAgentOriginal = "user_agent.original"; } } diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs index 1c1f74f1559..b7a6dbe891f 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs @@ -197,7 +197,7 @@ public void OnStartActivity(Activity activity, object payload) var path = (request.PathBase.HasValue || request.Path.HasValue) ? (request.PathBase + request.Path).ToString() : "/"; activity.DisplayName = path; - // see the spec https://github.com/open-telemetry/semantic-conventions/blob/main/specification/trace/semantic_conventions/http.md + // see the spec https://github.com/open-telemetry/opentelemetry-specification/blob/v1.20.0/specification/trace/semantic_conventions/http.md if (this.httpSemanticConvention.HasFlag(HttpSemanticConvention.Old)) { if (request.Host.HasValue) @@ -223,6 +223,7 @@ public void OnStartActivity(Activity activity, object payload) } } + // see the spec https://github.com/open-telemetry/semantic-conventions/blob/main/specification/trace/semantic_conventions/http.md if (this.httpSemanticConvention.HasFlag(HttpSemanticConvention.New)) { if (request.Host.HasValue) @@ -235,10 +236,14 @@ public void OnStartActivity(Activity activity, object payload) } } + if (request.QueryString.HasValue) + { + activity.SetTag(SemanticConventions.AttributeUrlQuery, request.QueryString.Value); + } + activity.SetTag(SemanticConventions.AttributeHttpRequestMethod, request.Method); activity.SetTag(SemanticConventions.AttributeUrlScheme, request.Scheme); activity.SetTag(SemanticConventions.AttributeUrlPath, path); - activity.SetTag(SemanticConventions.AttributeUrlFull, GetUri(request)); activity.SetTag(SemanticConventions.AttributeNetworkProtocolVersion, HttpTagHelper.GetFlavorTagValueFromProtocol(request.Protocol)); var userAgent = request.Headers["User-Agent"].FirstOrDefault(); diff --git a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/IncomingRequestsCollectionsIsAccordingToTheSpecTests.cs b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/IncomingRequestsCollectionsIsAccordingToTheSpecTests.cs index 8500ac00a22..b1adfd007f3 100644 --- a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/IncomingRequestsCollectionsIsAccordingToTheSpecTests.cs +++ b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/IncomingRequestsCollectionsIsAccordingToTheSpecTests.cs @@ -43,7 +43,7 @@ public IncomingRequestsCollectionsIsAccordingToTheSpecTests(WebApplicationFactor [InlineData("/api/values", "?query=1", null, 503, null)] [InlineData("/api/exception", null, null, 503, null)] [InlineData("/api/exception", null, null, 503, null, true)] - public async Task SuccessfulTemplateControllerCallGeneratesASpan( + public async Task SuccessfulTemplateControllerCallGeneratesASpan_Old( string urlPath, string query, string userAgent, @@ -51,100 +51,227 @@ public async Task SuccessfulTemplateControllerCallGeneratesASpan( string reasonPhrase, bool recordException = false) { - var exportedItems = new List(); + try + { + Environment.SetEnvironmentVariable("OTEL_SEMCONV_STABILITY_OPT_IN", "none"); - // Arrange - using (var client = this.factory - .WithWebHostBuilder(builder => - { - builder.ConfigureTestServices((IServiceCollection services) => + var exportedItems = new List(); + + // Arrange + using (var client = this.factory + .WithWebHostBuilder(builder => { - services.AddSingleton(new TestCallbackMiddlewareImpl(statusCode, reasonPhrase)); - services.AddOpenTelemetry() - .WithTracing(builder => builder - .AddAspNetCoreInstrumentation(options => options.RecordException = recordException) - .AddInMemoryExporter(exportedItems)); - }); - builder.ConfigureLogging(loggingBuilder => loggingBuilder.ClearProviders()); - }) - .CreateClient()) - { - try + builder.ConfigureTestServices((IServiceCollection services) => + { + services.AddSingleton(new TestCallbackMiddlewareImpl(statusCode, reasonPhrase)); + services.AddOpenTelemetry() + .WithTracing(builder => builder + .AddAspNetCoreInstrumentation(options => options.RecordException = recordException) + .AddInMemoryExporter(exportedItems)); + }); + builder.ConfigureLogging(loggingBuilder => loggingBuilder.ClearProviders()); + }) + .CreateClient()) { - if (!string.IsNullOrEmpty(userAgent)) + try + { + if (!string.IsNullOrEmpty(userAgent)) + { + client.DefaultRequestHeaders.Add("User-Agent", userAgent); + } + + // Act + var path = urlPath; + if (query != null) + { + path += query; + } + + using var response = await client.GetAsync(path).ConfigureAwait(false); + } + catch (Exception) { - client.DefaultRequestHeaders.Add("User-Agent", userAgent); + // ignore errors } - // Act - var path = urlPath; - if (query != null) + for (var i = 0; i < 10; i++) { - path += query; + if (exportedItems.Count == 1) + { + break; + } + + // We need to let End callback execute as it is executed AFTER response was returned. + // In unit tests environment there may be a lot of parallel unit tests executed, so + // giving some breezing room for the End callback to complete + await Task.Delay(TimeSpan.FromSeconds(1)).ConfigureAwait(false); } + } - using var response = await client.GetAsync(path).ConfigureAwait(false); + Assert.Single(exportedItems); + var activity = exportedItems[0]; + + Assert.Equal(ActivityKind.Server, activity.Kind); + Assert.Equal("localhost", activity.GetTagValue(SemanticConventions.AttributeNetHostName)); + Assert.Equal("GET", activity.GetTagValue(SemanticConventions.AttributeHttpMethod)); + Assert.Equal("1.1", activity.GetTagValue(SemanticConventions.AttributeHttpFlavor)); + Assert.Equal("http", activity.GetTagValue(SemanticConventions.AttributeHttpScheme)); + Assert.Equal(urlPath, activity.GetTagValue(SemanticConventions.AttributeHttpTarget)); + Assert.Equal($"http://localhost{urlPath}{query}", activity.GetTagValue(SemanticConventions.AttributeHttpUrl)); + Assert.Equal(statusCode, activity.GetTagValue(SemanticConventions.AttributeHttpStatusCode)); + + if (statusCode == 503) + { + Assert.Equal(ActivityStatusCode.Error, activity.Status); } - catch (Exception) + else { - // ignore errors + Assert.Equal(ActivityStatusCode.Unset, activity.Status); } - for (var i = 0; i < 10; i++) + // Instrumentation is not expected to set status description + // as the reason can be inferred from SemanticConventions.AttributeHttpStatusCode + if (!urlPath.EndsWith("exception")) { - if (exportedItems.Count == 1) - { - break; - } - - // We need to let End callback execute as it is executed AFTER response was returned. - // In unit tests environment there may be a lot of parallel unit tests executed, so - // giving some breezing room for the End callback to complete - await Task.Delay(TimeSpan.FromSeconds(1)).ConfigureAwait(false); + Assert.True(string.IsNullOrEmpty(activity.StatusDescription)); + } + else + { + Assert.Equal("exception description", activity.StatusDescription); } - } - Assert.Single(exportedItems); - var activity = exportedItems[0]; + if (recordException) + { + Assert.Single(activity.Events); + Assert.Equal("exception", activity.Events.First().Name); + } - Assert.Equal(ActivityKind.Server, activity.Kind); - Assert.Equal("localhost", activity.GetTagValue(SemanticConventions.AttributeNetHostName)); - Assert.Equal("GET", activity.GetTagValue(SemanticConventions.AttributeHttpMethod)); - Assert.Equal("1.1", activity.GetTagValue(SemanticConventions.AttributeHttpFlavor)); - Assert.Equal("http", activity.GetTagValue(SemanticConventions.AttributeHttpScheme)); - Assert.Equal(urlPath, activity.GetTagValue(SemanticConventions.AttributeHttpTarget)); - Assert.Equal($"http://localhost{urlPath}{query}", activity.GetTagValue(SemanticConventions.AttributeHttpUrl)); - Assert.Equal(statusCode, activity.GetTagValue(SemanticConventions.AttributeHttpStatusCode)); + ValidateTagValue(activity, SemanticConventions.AttributeHttpUserAgent, userAgent); - if (statusCode == 503) - { - Assert.Equal(ActivityStatusCode.Error, activity.Status); + activity.Dispose(); } - else + finally { - Assert.Equal(ActivityStatusCode.Unset, activity.Status); + Environment.SetEnvironmentVariable("OTEL_SEMCONV_STABILITY_OPT_IN", null); } + } - // Instrumentation is not expected to set status description - // as the reason can be inferred from SemanticConventions.AttributeHttpStatusCode - if (!urlPath.EndsWith("exception")) - { - Assert.True(string.IsNullOrEmpty(activity.StatusDescription)); - } - else + [Theory] + [InlineData("/api/values", null, "user-agent", 503, "503")] + [InlineData("/api/values", "?query=1", null, 503, null)] + [InlineData("/api/exception", null, null, 503, null)] + [InlineData("/api/exception", null, null, 503, null, true)] + public async Task SuccessfulTemplateControllerCallGeneratesASpan_New( + string urlPath, + string query, + string userAgent, + int statusCode, + string reasonPhrase, + bool recordException = false) + { + try { - Assert.Equal("exception description", activity.StatusDescription); - } + Environment.SetEnvironmentVariable("OTEL_SEMCONV_STABILITY_OPT_IN", "http"); - if (recordException) - { - Assert.Single(activity.Events); - Assert.Equal("exception", activity.Events.First().Name); - } + var exportedItems = new List(); + + // Arrange + using (var client = this.factory + .WithWebHostBuilder(builder => + { + builder.ConfigureTestServices((IServiceCollection services) => + { + services.AddSingleton(new TestCallbackMiddlewareImpl(statusCode, reasonPhrase)); + services.AddOpenTelemetry() + .WithTracing(builder => builder + .AddAspNetCoreInstrumentation(options => options.RecordException = recordException) + .AddInMemoryExporter(exportedItems)); + }); + builder.ConfigureLogging(loggingBuilder => loggingBuilder.ClearProviders()); + }) + .CreateClient()) + { + try + { + if (!string.IsNullOrEmpty(userAgent)) + { + client.DefaultRequestHeaders.Add("User-Agent", userAgent); + } + + // Act + var path = urlPath; + if (query != null) + { + path += query; + } + + using var response = await client.GetAsync(path).ConfigureAwait(false); + } + catch (Exception) + { + // ignore errors + } + + for (var i = 0; i < 10; i++) + { + if (exportedItems.Count == 1) + { + break; + } + + // We need to let End callback execute as it is executed AFTER response was returned. + // In unit tests environment there may be a lot of parallel unit tests executed, so + // giving some breezing room for the End callback to complete + await Task.Delay(TimeSpan.FromSeconds(1)).ConfigureAwait(false); + } + } - ValidateTagValue(activity, SemanticConventions.AttributeHttpUserAgent, userAgent); + Assert.Single(exportedItems); + var activity = exportedItems[0]; - activity.Dispose(); + Assert.Equal(ActivityKind.Server, activity.Kind); + Assert.Equal("localhost", activity.GetTagValue(SemanticConventions.AttributeServerAddress)); + Assert.Equal("GET", activity.GetTagValue(SemanticConventions.AttributeHttpRequestMethod)); + Assert.Equal("1.1", activity.GetTagValue(SemanticConventions.AttributeNetworkProtocolVersion)); + Assert.Equal("http", activity.GetTagValue(SemanticConventions.AttributeUrlScheme)); + Assert.Equal(urlPath, activity.GetTagValue(SemanticConventions.AttributeUrlPath)); + Assert.Equal(query, activity.GetTagValue(SemanticConventions.AttributeUrlQuery)); + Assert.Equal(statusCode, activity.GetTagValue(SemanticConventions.AttributeHttpResponseStatusCode)); + + if (statusCode == 503) + { + Assert.Equal(ActivityStatusCode.Error, activity.Status); + } + else + { + Assert.Equal(ActivityStatusCode.Unset, activity.Status); + } + + // Instrumentation is not expected to set status description + // as the reason can be inferred from SemanticConventions.AttributeHttpStatusCode + if (!urlPath.EndsWith("exception")) + { + Assert.True(string.IsNullOrEmpty(activity.StatusDescription)); + } + else + { + Assert.Equal("exception description", activity.StatusDescription); + } + + if (recordException) + { + Assert.Single(activity.Events); + Assert.Equal("exception", activity.Events.First().Name); + } + + ValidateTagValue(activity, SemanticConventions.AttributeUserAgentOriginal, userAgent); + + activity.Dispose(); + } + finally + { + Environment.SetEnvironmentVariable("OTEL_SEMCONV_STABILITY_OPT_IN", null); + } } private static void ValidateTagValue(Activity activity, string attribute, string expectedValue) From f78aedf7ac91d77f5d7fdcda54fc1499a8fe98f5 Mon Sep 17 00:00:00 2001 From: Timothy Mothra Lee Date: Wed, 7 Jun 2023 13:31:54 -0700 Subject: [PATCH 04/15] comment --- .../Implementation/HttpInMetricsListener.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs index 03f00864aaa..dc32c16f68b 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs @@ -82,6 +82,7 @@ public override void OnEventWritten(string name, object payload) TagList tags = default; + // see the spec https://github.com/open-telemetry/opentelemetry-specification/blob/v1.20.0/specification/trace/semantic_conventions/http.md if (this.httpSemanticConvention.HasFlag(HttpSemanticConvention.Old)) { tags.Add(new KeyValuePair(SemanticConventions.AttributeHttpFlavor, HttpTagHelper.GetFlavorTagValueFromProtocol(context.Request.Protocol))); @@ -100,6 +101,7 @@ public override void OnEventWritten(string name, object payload) } } + // see the spec https://github.com/open-telemetry/semantic-conventions/blob/main/specification/trace/semantic_conventions/http.md if (this.httpSemanticConvention.HasFlag(HttpSemanticConvention.New)) { tags.Add(new KeyValuePair(SemanticConventions.AttributeNetworkProtocolVersion, HttpTagHelper.GetFlavorTagValueFromProtocol(context.Request.Protocol))); From b1fc737c678354136bfa53d949579bb4c5198831 Mon Sep 17 00:00:00 2001 From: Timothy Mothra Lee Date: Wed, 7 Jun 2023 14:36:50 -0700 Subject: [PATCH 05/15] update --- .../Implementation/HttpInListener.cs | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs index b7a6dbe891f..69ba130ade6 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs @@ -216,10 +216,13 @@ public void OnStartActivity(Activity activity, object payload) activity.SetTag(SemanticConventions.AttributeHttpUrl, GetUri(request)); activity.SetTag(SemanticConventions.AttributeHttpFlavor, HttpTagHelper.GetFlavorTagValueFromProtocol(request.Protocol)); - var userAgent = request.Headers["User-Agent"].FirstOrDefault(); - if (!string.IsNullOrEmpty(userAgent)) + if (request.Headers.TryGetValue("User-Agent", out var values)) { - activity.SetTag(SemanticConventions.AttributeHttpUserAgent, userAgent); + var userAgent = values.Count > 0 ? values[0] : null; + if (!string.IsNullOrEmpty(userAgent)) + { + activity.SetTag(SemanticConventions.AttributeHttpUserAgent, userAgent); + } } } @@ -246,10 +249,13 @@ public void OnStartActivity(Activity activity, object payload) activity.SetTag(SemanticConventions.AttributeUrlPath, path); activity.SetTag(SemanticConventions.AttributeNetworkProtocolVersion, HttpTagHelper.GetFlavorTagValueFromProtocol(request.Protocol)); - var userAgent = request.Headers["User-Agent"].FirstOrDefault(); - if (!string.IsNullOrEmpty(userAgent)) + if (request.Headers.TryGetValue("User-Agent", out var values)) { - activity.SetTag(SemanticConventions.AttributeUserAgentOriginal, userAgent); + var userAgent = values.Count > 0 ? values[0] : null; + if (!string.IsNullOrEmpty(userAgent)) + { + activity.SetTag(SemanticConventions.AttributeUserAgentOriginal, userAgent); + } } } From f2203e34778015ca519d14a939ec04539ea219fe Mon Sep 17 00:00:00 2001 From: Timothy Mothra Lee Date: Wed, 7 Jun 2023 15:29:06 -0700 Subject: [PATCH 06/15] split tests into two files to solve concurrency failures --- ...stsCollectionsIsAccordingToTheSpecTests.cs | 118 ----------- ...ollectionsIsAccordingToTheSpecTests_New.cs | 197 ++++++++++++++++++ 2 files changed, 197 insertions(+), 118 deletions(-) create mode 100644 test/OpenTelemetry.Instrumentation.AspNetCore.Tests/IncomingRequestsCollectionsIsAccordingToTheSpecTests_New.cs diff --git a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/IncomingRequestsCollectionsIsAccordingToTheSpecTests.cs b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/IncomingRequestsCollectionsIsAccordingToTheSpecTests.cs index b1adfd007f3..af9e63e2677 100644 --- a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/IncomingRequestsCollectionsIsAccordingToTheSpecTests.cs +++ b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/IncomingRequestsCollectionsIsAccordingToTheSpecTests.cs @@ -156,124 +156,6 @@ public async Task SuccessfulTemplateControllerCallGeneratesASpan_Old( } } - [Theory] - [InlineData("/api/values", null, "user-agent", 503, "503")] - [InlineData("/api/values", "?query=1", null, 503, null)] - [InlineData("/api/exception", null, null, 503, null)] - [InlineData("/api/exception", null, null, 503, null, true)] - public async Task SuccessfulTemplateControllerCallGeneratesASpan_New( - string urlPath, - string query, - string userAgent, - int statusCode, - string reasonPhrase, - bool recordException = false) - { - try - { - Environment.SetEnvironmentVariable("OTEL_SEMCONV_STABILITY_OPT_IN", "http"); - - var exportedItems = new List(); - - // Arrange - using (var client = this.factory - .WithWebHostBuilder(builder => - { - builder.ConfigureTestServices((IServiceCollection services) => - { - services.AddSingleton(new TestCallbackMiddlewareImpl(statusCode, reasonPhrase)); - services.AddOpenTelemetry() - .WithTracing(builder => builder - .AddAspNetCoreInstrumentation(options => options.RecordException = recordException) - .AddInMemoryExporter(exportedItems)); - }); - builder.ConfigureLogging(loggingBuilder => loggingBuilder.ClearProviders()); - }) - .CreateClient()) - { - try - { - if (!string.IsNullOrEmpty(userAgent)) - { - client.DefaultRequestHeaders.Add("User-Agent", userAgent); - } - - // Act - var path = urlPath; - if (query != null) - { - path += query; - } - - using var response = await client.GetAsync(path).ConfigureAwait(false); - } - catch (Exception) - { - // ignore errors - } - - for (var i = 0; i < 10; i++) - { - if (exportedItems.Count == 1) - { - break; - } - - // We need to let End callback execute as it is executed AFTER response was returned. - // In unit tests environment there may be a lot of parallel unit tests executed, so - // giving some breezing room for the End callback to complete - await Task.Delay(TimeSpan.FromSeconds(1)).ConfigureAwait(false); - } - } - - Assert.Single(exportedItems); - var activity = exportedItems[0]; - - Assert.Equal(ActivityKind.Server, activity.Kind); - Assert.Equal("localhost", activity.GetTagValue(SemanticConventions.AttributeServerAddress)); - Assert.Equal("GET", activity.GetTagValue(SemanticConventions.AttributeHttpRequestMethod)); - Assert.Equal("1.1", activity.GetTagValue(SemanticConventions.AttributeNetworkProtocolVersion)); - Assert.Equal("http", activity.GetTagValue(SemanticConventions.AttributeUrlScheme)); - Assert.Equal(urlPath, activity.GetTagValue(SemanticConventions.AttributeUrlPath)); - Assert.Equal(query, activity.GetTagValue(SemanticConventions.AttributeUrlQuery)); - Assert.Equal(statusCode, activity.GetTagValue(SemanticConventions.AttributeHttpResponseStatusCode)); - - if (statusCode == 503) - { - Assert.Equal(ActivityStatusCode.Error, activity.Status); - } - else - { - Assert.Equal(ActivityStatusCode.Unset, activity.Status); - } - - // Instrumentation is not expected to set status description - // as the reason can be inferred from SemanticConventions.AttributeHttpStatusCode - if (!urlPath.EndsWith("exception")) - { - Assert.True(string.IsNullOrEmpty(activity.StatusDescription)); - } - else - { - Assert.Equal("exception description", activity.StatusDescription); - } - - if (recordException) - { - Assert.Single(activity.Events); - Assert.Equal("exception", activity.Events.First().Name); - } - - ValidateTagValue(activity, SemanticConventions.AttributeUserAgentOriginal, userAgent); - - activity.Dispose(); - } - finally - { - Environment.SetEnvironmentVariable("OTEL_SEMCONV_STABILITY_OPT_IN", null); - } - } - private static void ValidateTagValue(Activity activity, string attribute, string expectedValue) { if (string.IsNullOrEmpty(expectedValue)) diff --git a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/IncomingRequestsCollectionsIsAccordingToTheSpecTests_New.cs b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/IncomingRequestsCollectionsIsAccordingToTheSpecTests_New.cs new file mode 100644 index 00000000000..47895a14754 --- /dev/null +++ b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/IncomingRequestsCollectionsIsAccordingToTheSpecTests_New.cs @@ -0,0 +1,197 @@ +// +// 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; +using Microsoft.AspNetCore.Hosting; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Http.Features; +using Microsoft.AspNetCore.Mvc.Testing; +using Microsoft.AspNetCore.TestHost; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging; +using OpenTelemetry.Trace; +using TestApp.AspNetCore; +using Xunit; + +namespace OpenTelemetry.Instrumentation.AspNetCore.Tests +{ + public class IncomingRequestsCollectionsIsAccordingToTheSpecTests_New + : IClassFixture> + { + private readonly WebApplicationFactory factory; + + public IncomingRequestsCollectionsIsAccordingToTheSpecTests_New(WebApplicationFactory factory) + { + this.factory = factory; + } + + [Theory] + [InlineData("/api/values", null, "user-agent", 503, "503")] + [InlineData("/api/values", "?query=1", null, 503, null)] + [InlineData("/api/exception", null, null, 503, null)] + [InlineData("/api/exception", null, null, 503, null, true)] + public async Task SuccessfulTemplateControllerCallGeneratesASpan_New( + string urlPath, + string query, + string userAgent, + int statusCode, + string reasonPhrase, + bool recordException = false) + { + try + { + Environment.SetEnvironmentVariable("OTEL_SEMCONV_STABILITY_OPT_IN", "http"); + + var exportedItems = new List(); + + // Arrange + using (var client = this.factory + .WithWebHostBuilder(builder => + { + builder.ConfigureTestServices((IServiceCollection services) => + { + services.AddSingleton(new TestCallbackMiddlewareImpl(statusCode, reasonPhrase)); + services.AddOpenTelemetry() + .WithTracing(builder => builder + .AddAspNetCoreInstrumentation(options => options.RecordException = recordException) + .AddInMemoryExporter(exportedItems)); + }); + builder.ConfigureLogging(loggingBuilder => loggingBuilder.ClearProviders()); + }) + .CreateClient()) + { + try + { + if (!string.IsNullOrEmpty(userAgent)) + { + client.DefaultRequestHeaders.Add("User-Agent", userAgent); + } + + // Act + var path = urlPath; + if (query != null) + { + path += query; + } + + using var response = await client.GetAsync(path).ConfigureAwait(false); + } + catch (Exception) + { + // ignore errors + } + + for (var i = 0; i < 10; i++) + { + if (exportedItems.Count == 1) + { + break; + } + + // We need to let End callback execute as it is executed AFTER response was returned. + // In unit tests environment there may be a lot of parallel unit tests executed, so + // giving some breezing room for the End callback to complete + await Task.Delay(TimeSpan.FromSeconds(1)).ConfigureAwait(false); + } + } + + Assert.Single(exportedItems); + var activity = exportedItems[0]; + + Assert.Equal(ActivityKind.Server, activity.Kind); + Assert.Equal("localhost", activity.GetTagValue(SemanticConventions.AttributeServerAddress)); + Assert.Equal("GET", activity.GetTagValue(SemanticConventions.AttributeHttpRequestMethod)); + Assert.Equal("1.1", activity.GetTagValue(SemanticConventions.AttributeNetworkProtocolVersion)); + Assert.Equal("http", activity.GetTagValue(SemanticConventions.AttributeUrlScheme)); + Assert.Equal(urlPath, activity.GetTagValue(SemanticConventions.AttributeUrlPath)); + Assert.Equal(query, activity.GetTagValue(SemanticConventions.AttributeUrlQuery)); + Assert.Equal(statusCode, activity.GetTagValue(SemanticConventions.AttributeHttpResponseStatusCode)); + + if (statusCode == 503) + { + Assert.Equal(ActivityStatusCode.Error, activity.Status); + } + else + { + Assert.Equal(ActivityStatusCode.Unset, activity.Status); + } + + // Instrumentation is not expected to set status description + // as the reason can be inferred from SemanticConventions.AttributeHttpStatusCode + if (!urlPath.EndsWith("exception")) + { + Assert.True(string.IsNullOrEmpty(activity.StatusDescription)); + } + else + { + Assert.Equal("exception description", activity.StatusDescription); + } + + if (recordException) + { + Assert.Single(activity.Events); + Assert.Equal("exception", activity.Events.First().Name); + } + + ValidateTagValue(activity, SemanticConventions.AttributeUserAgentOriginal, userAgent); + + activity.Dispose(); + } + finally + { + Environment.SetEnvironmentVariable("OTEL_SEMCONV_STABILITY_OPT_IN", null); + } + } + + private static void ValidateTagValue(Activity activity, string attribute, string expectedValue) + { + if (string.IsNullOrEmpty(expectedValue)) + { + Assert.Null(activity.GetTagValue(attribute)); + } + else + { + Assert.Equal(expectedValue, activity.GetTagValue(attribute)); + } + } + + public class TestCallbackMiddlewareImpl : CallbackMiddleware.CallbackMiddlewareImpl + { + private readonly int statusCode; + private readonly string reasonPhrase; + + public TestCallbackMiddlewareImpl(int statusCode, string reasonPhrase) + { + this.statusCode = statusCode; + this.reasonPhrase = reasonPhrase; + } + + public override async Task ProcessAsync(HttpContext context) + { + context.Response.StatusCode = this.statusCode; + context.Response.HttpContext.Features.Get().ReasonPhrase = this.reasonPhrase; + await context.Response.WriteAsync("empty").ConfigureAwait(false); + + if (context.Request.Path.Value.EndsWith("exception")) + { + throw new Exception("exception description"); + } + + return false; + } + } + } +} From 14250da3810acd8b697421f77e17a04ad281e5b4 Mon Sep 17 00:00:00 2001 From: Timothy Mothra Lee Date: Fri, 9 Jun 2023 10:00:26 -0700 Subject: [PATCH 07/15] fix attribute name --- src/OpenTelemetry.Api/Internal/SemanticConventions.cs | 1 - .../Implementation/HttpInMetricsListener.cs | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/src/OpenTelemetry.Api/Internal/SemanticConventions.cs b/src/OpenTelemetry.Api/Internal/SemanticConventions.cs index 52a08634606..8a010f73704 100644 --- a/src/OpenTelemetry.Api/Internal/SemanticConventions.cs +++ b/src/OpenTelemetry.Api/Internal/SemanticConventions.cs @@ -117,7 +117,6 @@ internal static class SemanticConventions public const string AttributeHttpResponseStatusCode = "http.response.status_code"; public const string AttributeNetworkProtocolVersion = "network.protocol.version"; public const string AttributeServerAddress = "server.address"; - public const string AttributeServerName = "server.name"; public const string AttributeServerPort = "server.port"; public const string AttributeUrlFull = "url.full"; public const string AttributeUrlPath = "url.path"; diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs index dc32c16f68b..632056fca86 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs @@ -111,7 +111,7 @@ public override void OnEventWritten(string name, object payload) if (context.Request.Host.HasValue) { - tags.Add(new KeyValuePair(SemanticConventions.AttributeServerName, context.Request.Host.Host)); + tags.Add(new KeyValuePair(SemanticConventions.AttributeServerAddress, context.Request.Host.Host)); if (context.Request.Host.Port is not null && context.Request.Host.Port != 80 && context.Request.Host.Port != 443) { From cbd15c7d8a28f467e2b9a2e472b5de75c03b46e6 Mon Sep 17 00:00:00 2001 From: Timothy Mothra Lee Date: Fri, 9 Jun 2023 10:01:36 -0700 Subject: [PATCH 08/15] comment --- .../Implementation/HttpInListener.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs index 69ba130ade6..a6251a837d2 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs @@ -241,6 +241,7 @@ public void OnStartActivity(Activity activity, object payload) if (request.QueryString.HasValue) { + // QueryString should be sanitized. see: https://github.com/open-telemetry/opentelemetry-dotnet/issues/4571 activity.SetTag(SemanticConventions.AttributeUrlQuery, request.QueryString.Value); } From f2716a2fc069b621f0253fdc7dffbd8621e99d88 Mon Sep 17 00:00:00 2001 From: Timothy Mothra Lee Date: Fri, 9 Jun 2023 10:35:18 -0700 Subject: [PATCH 09/15] remove unused attribute --- src/OpenTelemetry.Api/Internal/SemanticConventions.cs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/OpenTelemetry.Api/Internal/SemanticConventions.cs b/src/OpenTelemetry.Api/Internal/SemanticConventions.cs index 8a010f73704..da13b7bf9e6 100644 --- a/src/OpenTelemetry.Api/Internal/SemanticConventions.cs +++ b/src/OpenTelemetry.Api/Internal/SemanticConventions.cs @@ -111,14 +111,13 @@ internal static class SemanticConventions public const string AttributeExceptionMessage = "exception.message"; public const string AttributeExceptionStacktrace = "exception.stacktrace"; - // NEW v1.21.0 Http Semantic Conventions + // v1.21.0 Http Semantic Conventions public const string AttributeClientSocketPort = "client.socket.port"; public const string AttributeHttpRequestMethod = "http.request.method"; public const string AttributeHttpResponseStatusCode = "http.response.status_code"; public const string AttributeNetworkProtocolVersion = "network.protocol.version"; public const string AttributeServerAddress = "server.address"; public const string AttributeServerPort = "server.port"; - public const string AttributeUrlFull = "url.full"; public const string AttributeUrlPath = "url.path"; public const string AttributeUrlScheme = "url.scheme"; public const string AttributeUrlQuery = "url.query"; From 85a91206324fcc3498542a3813b637e977d34ae4 Mon Sep 17 00:00:00 2001 From: Timothy Mothra Lee Date: Mon, 12 Jun 2023 17:17:32 -0700 Subject: [PATCH 10/15] added another test for the dup scenario --- ...llectionsIsAccordingToTheSpecTests_Dupe.cs | 204 ++++++++++++++++++ 1 file changed, 204 insertions(+) create mode 100644 test/OpenTelemetry.Instrumentation.AspNetCore.Tests/IncomingRequestsCollectionsIsAccordingToTheSpecTests_Dupe.cs diff --git a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/IncomingRequestsCollectionsIsAccordingToTheSpecTests_Dupe.cs b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/IncomingRequestsCollectionsIsAccordingToTheSpecTests_Dupe.cs new file mode 100644 index 00000000000..01e3cb8ce93 --- /dev/null +++ b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/IncomingRequestsCollectionsIsAccordingToTheSpecTests_Dupe.cs @@ -0,0 +1,204 @@ +// +// 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; +using Microsoft.AspNetCore.Hosting; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Http.Features; +using Microsoft.AspNetCore.Mvc.Testing; +using Microsoft.AspNetCore.TestHost; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging; +using OpenTelemetry.Trace; +using TestApp.AspNetCore; +using Xunit; + +namespace OpenTelemetry.Instrumentation.AspNetCore.Tests +{ + public class IncomingRequestsCollectionsIsAccordingToTheSpecTests_Dupe + : IClassFixture> + { + private readonly WebApplicationFactory factory; + + public IncomingRequestsCollectionsIsAccordingToTheSpecTests_Dupe(WebApplicationFactory factory) + { + this.factory = factory; + } + + [Theory] + [InlineData("/api/values", null, "user-agent", 503, "503")] + [InlineData("/api/values", "?query=1", null, 503, null)] + [InlineData("/api/exception", null, null, 503, null)] + [InlineData("/api/exception", null, null, 503, null, true)] + public async Task SuccessfulTemplateControllerCallGeneratesASpan_Dupe( + string urlPath, + string query, + string userAgent, + int statusCode, + string reasonPhrase, + bool recordException = false) + { + try + { + Environment.SetEnvironmentVariable("OTEL_SEMCONV_STABILITY_OPT_IN", "http/dup"); + + var exportedItems = new List(); + + // Arrange + using (var client = this.factory + .WithWebHostBuilder(builder => + { + builder.ConfigureTestServices((IServiceCollection services) => + { + services.AddSingleton(new TestCallbackMiddlewareImpl(statusCode, reasonPhrase)); + services.AddOpenTelemetry() + .WithTracing(builder => builder + .AddAspNetCoreInstrumentation(options => options.RecordException = recordException) + .AddInMemoryExporter(exportedItems)); + }); + builder.ConfigureLogging(loggingBuilder => loggingBuilder.ClearProviders()); + }) + .CreateClient()) + { + try + { + if (!string.IsNullOrEmpty(userAgent)) + { + client.DefaultRequestHeaders.Add("User-Agent", userAgent); + } + + // Act + var path = urlPath; + if (query != null) + { + path += query; + } + + using var response = await client.GetAsync(path).ConfigureAwait(false); + } + catch (Exception) + { + // ignore errors + } + + for (var i = 0; i < 10; i++) + { + if (exportedItems.Count == 1) + { + break; + } + + // We need to let End callback execute as it is executed AFTER response was returned. + // In unit tests environment there may be a lot of parallel unit tests executed, so + // giving some breezing room for the End callback to complete + await Task.Delay(TimeSpan.FromSeconds(1)).ConfigureAwait(false); + } + } + + Assert.Single(exportedItems); + var activity = exportedItems[0]; + + Assert.Equal(ActivityKind.Server, activity.Kind); + Assert.Equal("localhost", activity.GetTagValue(SemanticConventions.AttributeServerAddress)); + Assert.Equal("localhost", activity.GetTagValue(SemanticConventions.AttributeNetHostName)); + Assert.Equal("GET", activity.GetTagValue(SemanticConventions.AttributeHttpRequestMethod)); + Assert.Equal("GET", activity.GetTagValue(SemanticConventions.AttributeHttpMethod)); + Assert.Equal("1.1", activity.GetTagValue(SemanticConventions.AttributeNetworkProtocolVersion)); + Assert.Equal("1.1", activity.GetTagValue(SemanticConventions.AttributeHttpFlavor)); + Assert.Equal("http", activity.GetTagValue(SemanticConventions.AttributeUrlScheme)); + Assert.Equal("http", activity.GetTagValue(SemanticConventions.AttributeHttpScheme)); + Assert.Equal(urlPath, activity.GetTagValue(SemanticConventions.AttributeUrlPath)); + Assert.Equal(urlPath, activity.GetTagValue(SemanticConventions.AttributeHttpTarget)); + Assert.Equal($"http://localhost{urlPath}{query}", activity.GetTagValue(SemanticConventions.AttributeHttpUrl)); + Assert.Equal(query, activity.GetTagValue(SemanticConventions.AttributeUrlQuery)); + Assert.Equal(statusCode, activity.GetTagValue(SemanticConventions.AttributeHttpResponseStatusCode)); + Assert.Equal(statusCode, activity.GetTagValue(SemanticConventions.AttributeHttpStatusCode)); + + if (statusCode == 503) + { + Assert.Equal(ActivityStatusCode.Error, activity.Status); + } + else + { + Assert.Equal(ActivityStatusCode.Unset, activity.Status); + } + + // Instrumentation is not expected to set status description + // as the reason can be inferred from SemanticConventions.AttributeHttpStatusCode + if (!urlPath.EndsWith("exception")) + { + Assert.True(string.IsNullOrEmpty(activity.StatusDescription)); + } + else + { + Assert.Equal("exception description", activity.StatusDescription); + } + + if (recordException) + { + Assert.Single(activity.Events); + Assert.Equal("exception", activity.Events.First().Name); + } + + ValidateTagValue(activity, SemanticConventions.AttributeUserAgentOriginal, userAgent); + + activity.Dispose(); + } + finally + { + Environment.SetEnvironmentVariable("OTEL_SEMCONV_STABILITY_OPT_IN", null); + } + } + + private static void ValidateTagValue(Activity activity, string attribute, string expectedValue) + { + if (string.IsNullOrEmpty(expectedValue)) + { + Assert.Null(activity.GetTagValue(attribute)); + } + else + { + Assert.Equal(expectedValue, activity.GetTagValue(attribute)); + } + } + + public class TestCallbackMiddlewareImpl : CallbackMiddleware.CallbackMiddlewareImpl + { + private readonly int statusCode; + private readonly string reasonPhrase; + + public TestCallbackMiddlewareImpl(int statusCode, string reasonPhrase) + { + this.statusCode = statusCode; + this.reasonPhrase = reasonPhrase; + } + + public override async Task ProcessAsync(HttpContext context) + { + context.Response.StatusCode = this.statusCode; + context.Response.HttpContext.Features.Get().ReasonPhrase = this.reasonPhrase; + await context.Response.WriteAsync("empty").ConfigureAwait(false); + + if (context.Request.Path.Value.EndsWith("exception")) + { + throw new Exception("exception description"); + } + + return false; + } + } + } +} From 808646bd91d5d337777f0ff16e37a51c5cd3ccfa Mon Sep 17 00:00:00 2001 From: Timothy Mothra Lee Date: Wed, 14 Jun 2023 15:02:37 -0700 Subject: [PATCH 11/15] changelog --- src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md b/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md index c40c65cb89f..2fc112c078c 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md @@ -2,6 +2,11 @@ ## Unreleased +* Updated [Http Semantic Conventions](https://github.com/open-telemetry/semantic-conventions/blob/main/specification/trace/semantic_conventions/http.md). + * This library can emit either old, new, or both attributes. Users can control + which attributes are emitted by setting the environment variable + `OTEL_SEMCONV_STABILITY_OPT_IN`. + ## 1.5.0-beta.1 Released 2023-Jun-05 From 99eabb88d3abcad9f44397f988d113ffce40355c Mon Sep 17 00:00:00 2001 From: Timothy Mothra Lee Date: Thu, 15 Jun 2023 17:25:33 -0700 Subject: [PATCH 12/15] comments --- .../Internal/SemanticConventions.cs | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/OpenTelemetry.Api/Internal/SemanticConventions.cs b/src/OpenTelemetry.Api/Internal/SemanticConventions.cs index da13b7bf9e6..b34fb0a006a 100644 --- a/src/OpenTelemetry.Api/Internal/SemanticConventions.cs +++ b/src/OpenTelemetry.Api/Internal/SemanticConventions.cs @@ -112,15 +112,15 @@ internal static class SemanticConventions public const string AttributeExceptionStacktrace = "exception.stacktrace"; // v1.21.0 Http Semantic Conventions - public const string AttributeClientSocketPort = "client.socket.port"; - public const string AttributeHttpRequestMethod = "http.request.method"; - public const string AttributeHttpResponseStatusCode = "http.response.status_code"; - public const string AttributeNetworkProtocolVersion = "network.protocol.version"; - public const string AttributeServerAddress = "server.address"; - public const string AttributeServerPort = "server.port"; - public const string AttributeUrlPath = "url.path"; - public const string AttributeUrlScheme = "url.scheme"; + public const string AttributeClientSocketPort = "client.socket.port"; // replaces: "net.peer.port" (AttributeNetPeerPort) + public const string AttributeHttpRequestMethod = "http.request.method"; // replaces: "http.method" (AttributeHttpMethod) + public const string AttributeHttpResponseStatusCode = "http.response.status_code"; // replaces: "http.status_code" (AttributeHttpStatusCode) + public const string AttributeNetworkProtocolVersion = "network.protocol.version"; // replaces: "http.flavor" (AttributeHttpFlavor) + public const string AttributeServerAddress = "server.address"; // replaces: "net.host.name" (AttributeNetHostName) + public const string AttributeServerPort = "server.port"; // replaces: "net.host.port" (AttributeNetHostPort) + public const string AttributeUrlPath = "url.path"; // replaces: "http.target" (AttributeHttpTarget) + public const string AttributeUrlScheme = "url.scheme"; // replaces: "http.scheme" (AttributeHttpScheme) public const string AttributeUrlQuery = "url.query"; - public const string AttributeUserAgentOriginal = "user_agent.original"; + public const string AttributeUserAgentOriginal = "user_agent.original"; // replaces: "http.user_agent" (AttributeHttpUserAgent) } } From 0933da2dc1fc51892b3d47e6ee03435f73d71a4d Mon Sep 17 00:00:00 2001 From: Timothy Mothra Lee Date: Thu, 15 Jun 2023 17:27:54 -0700 Subject: [PATCH 13/15] update url --- src/OpenTelemetry.Api/Internal/SemanticConventions.cs | 2 +- .../Implementation/HttpInListener.cs | 2 +- .../Implementation/HttpInMetricsListener.cs | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/OpenTelemetry.Api/Internal/SemanticConventions.cs b/src/OpenTelemetry.Api/Internal/SemanticConventions.cs index b34fb0a006a..5405a5008f8 100644 --- a/src/OpenTelemetry.Api/Internal/SemanticConventions.cs +++ b/src/OpenTelemetry.Api/Internal/SemanticConventions.cs @@ -111,7 +111,7 @@ internal static class SemanticConventions public const string AttributeExceptionMessage = "exception.message"; public const string AttributeExceptionStacktrace = "exception.stacktrace"; - // v1.21.0 Http Semantic Conventions + // v1.21.0 Http Semantic Conventions https://github.com/open-telemetry/opentelemetry-specification/blob/v1.21.0/specification/trace/semantic_conventions/http.md public const string AttributeClientSocketPort = "client.socket.port"; // replaces: "net.peer.port" (AttributeNetPeerPort) public const string AttributeHttpRequestMethod = "http.request.method"; // replaces: "http.method" (AttributeHttpMethod) public const string AttributeHttpResponseStatusCode = "http.response.status_code"; // replaces: "http.status_code" (AttributeHttpStatusCode) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs index a6251a837d2..dbe747aaf85 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs @@ -226,7 +226,7 @@ public void OnStartActivity(Activity activity, object payload) } } - // see the spec https://github.com/open-telemetry/semantic-conventions/blob/main/specification/trace/semantic_conventions/http.md + // see the spec https://github.com/open-telemetry/opentelemetry-specification/blob/v1.21.0/specification/trace/semantic_conventions/http.md if (this.httpSemanticConvention.HasFlag(HttpSemanticConvention.New)) { if (request.Host.HasValue) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs index 632056fca86..17d9e77738e 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs @@ -101,7 +101,7 @@ public override void OnEventWritten(string name, object payload) } } - // see the spec https://github.com/open-telemetry/semantic-conventions/blob/main/specification/trace/semantic_conventions/http.md + // see the spec https://github.com/open-telemetry/opentelemetry-specification/blob/v1.21.0/specification/trace/semantic_conventions/http.md if (this.httpSemanticConvention.HasFlag(HttpSemanticConvention.New)) { tags.Add(new KeyValuePair(SemanticConventions.AttributeNetworkProtocolVersion, HttpTagHelper.GetFlavorTagValueFromProtocol(context.Request.Protocol))); From 2a6d88c35749c181f5c0816e994965c88228af49 Mon Sep 17 00:00:00 2001 From: Timothy Mothra Lee Date: Thu, 15 Jun 2023 17:36:45 -0700 Subject: [PATCH 14/15] update changelog --- src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md b/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md index 2fc112c078c..301d158d93a 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md @@ -2,7 +2,7 @@ ## Unreleased -* Updated [Http Semantic Conventions](https://github.com/open-telemetry/semantic-conventions/blob/main/specification/trace/semantic_conventions/http.md). +* Updated [Http Semantic Conventions](https://github.com/open-telemetry/opentelemetry-specification/blob/v1.21.0/specification/trace/semantic_conventions/http.md). * This library can emit either old, new, or both attributes. Users can control which attributes are emitted by setting the environment variable `OTEL_SEMCONV_STABILITY_OPT_IN`. From bf807ebd34ef2105673eaddad727d33f4c0c9a0e Mon Sep 17 00:00:00 2001 From: Timothy Mothra Lee Date: Mon, 19 Jun 2023 14:55:31 -0700 Subject: [PATCH 15/15] fix attribute: server.port --- src/OpenTelemetry.Api/Internal/SemanticConventions.cs | 2 +- .../Implementation/HttpInListener.cs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/OpenTelemetry.Api/Internal/SemanticConventions.cs b/src/OpenTelemetry.Api/Internal/SemanticConventions.cs index 5405a5008f8..2fb9b0022fe 100644 --- a/src/OpenTelemetry.Api/Internal/SemanticConventions.cs +++ b/src/OpenTelemetry.Api/Internal/SemanticConventions.cs @@ -111,7 +111,7 @@ internal static class SemanticConventions public const string AttributeExceptionMessage = "exception.message"; public const string AttributeExceptionStacktrace = "exception.stacktrace"; - // v1.21.0 Http Semantic Conventions https://github.com/open-telemetry/opentelemetry-specification/blob/v1.21.0/specification/trace/semantic_conventions/http.md + // Http v1.21.0 https://github.com/open-telemetry/opentelemetry-specification/blob/v1.21.0/specification/trace/semantic_conventions/http.md public const string AttributeClientSocketPort = "client.socket.port"; // replaces: "net.peer.port" (AttributeNetPeerPort) public const string AttributeHttpRequestMethod = "http.request.method"; // replaces: "http.method" (AttributeHttpMethod) public const string AttributeHttpResponseStatusCode = "http.response.status_code"; // replaces: "http.status_code" (AttributeHttpStatusCode) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs index dbe747aaf85..809473ad3aa 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs @@ -495,7 +495,7 @@ private void AddGrpcAttributes(Activity activity, string grpcMethod, HttpContext if (this.httpSemanticConvention.HasFlag(HttpSemanticConvention.New)) { - activity.SetTag(SemanticConventions.AttributeClientSocketPort, context.Connection.RemotePort); + activity.SetTag(SemanticConventions.AttributeServerPort, context.Connection.RemotePort); } bool validConversion = GrpcTagHelper.TryGetGrpcStatusCodeFromActivity(activity, out int status);