Instrumentation to store context object into Activity.CustomProperty (#…

…1128)

* Modify instrumentations to populate context object only if Activity.IsAllDataRequested is set

* AspNetCore tests

* Asp.Net tests

* httpclient tet

* httpwebrequest test

* sqlclient tests

* grpc test

* changelog

* move inside alldatarequested

* remove test which validate customproperty population in propagation only mode

* cop caught

* fix test

src/OpenTelemetry.Instrumentation.AspNet/CHANGELOG.md

@@ -2,6 +2,9 @@
 
 ## Unreleased
 
+* Asp.Net Instrumentation automatically populates HttpRequest, HttpResponse in
+  Activity custom property
+
 * Changed the default propagation to support W3C Baggage
   ([#1048](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1048))
   * The default ITextFormat is now `CompositePropagator(TraceContextFormat,

src/OpenTelemetry.Instrumentation.AspNet/Implementation/HttpInListener.cs

@@ -26,6 +26,8 @@ namespace OpenTelemetry.Instrumentation.AspNet.Implementation
 {
     internal class HttpInListener : ListenerHandler
     {
+        public const string RequestCustomPropertyName = "OTel.AspNet.Request";
+        public const string ResponseCustomPropertyName = "OTel.AspNet.Response";
         private const string ActivityNameByHttpInListener = "ActivityCreatedByHttpInListener";
         private static readonly Func<HttpRequest, string, IEnumerable<string>> HttpRequestHeaderValuesGetter = (request, name) => request.Headers.GetValues(name);
         private readonly PropertyFetcher routeFetcher = new PropertyFetcher("Route");
@@ -102,6 +104,7 @@ public override void OnStartActivity(Activity activity, object payload)
 
             if (activity.IsAllDataRequested)
             {
+                activity.SetCustomProperty(RequestCustomPropertyName, request);
                 if (request.Url.Port == 80 || request.Url.Port == 443)
                 {
                     activity.SetTag(SemanticConventions.AttributeHttpHost, request.Url.Host);
@@ -151,6 +154,7 @@ public override void OnStopActivity(Activity activity, object payload)
 
                 var response = context.Response;
 
+                activityToEnrich.SetCustomProperty(ResponseCustomPropertyName, response);
                 activityToEnrich.SetTag(SemanticConventions.AttributeHttpStatusCode, response.StatusCode);
 
                 activityToEnrich.SetStatus(

src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md

@@ -2,6 +2,9 @@
 
 ## Unreleased
 
+* Asp.Net Core Instrumentation automatically populates HttpRequest,
+  HttpResponse in Activity custom property
+
 * Changed the default propagation to support W3C Baggage
   ([#1048](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1048))
   * The default ITextFormat is now `CompositePropagator(TraceContextFormat,

src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs

@@ -29,6 +29,8 @@ namespace OpenTelemetry.Instrumentation.AspNetCore.Implementation
 {
     internal class HttpInListener : ListenerHandler
     {
+        public const string RequestCustomPropertyName = "OTel.AspNetCore.Request";
+        public const string ResponseCustomPropertyName = "OTel.AspNetCore.Response";
         private const string UnknownHostName = "UNKNOWN-HOST";
         private const string ActivityNameByHttpInListener = "ActivityCreatedByHttpInListener";
         private static readonly Func<HttpRequest, string, IEnumerable<string>> HttpRequestHeaderValuesGetter = (request, name) => request.Headers[name];
@@ -98,6 +100,8 @@ public override void OnStartActivity(Activity activity, object payload)
 
             if (activity.IsAllDataRequested)
             {
+                activity.SetCustomProperty(RequestCustomPropertyName, request);
+
                 var path = (request.PathBase.HasValue || request.Path.HasValue) ? (request.PathBase + request.Path).ToString() : "/";
                 activity.DisplayName = path;
 
@@ -135,6 +139,7 @@ public override void OnStopActivity(Activity activity, object payload)
                 }
 
                 var response = context.Response;
+                activity.SetCustomProperty(ResponseCustomPropertyName, response);
                 activity.SetTag(SemanticConventions.AttributeHttpStatusCode, response.StatusCode);
 
                 if (TryGetGrpcMethod(activity, out var grpcMethod))

src/OpenTelemetry.Instrumentation.Grpc/CHANGELOG.md

@@ -2,9 +2,10 @@
 
 ## Unreleased
 
-* Grpc instrumentation will now add the raw Request object to the Activity it
-  creates
+* Grpc Instrumentation automatically populates HttpRequest in
+  Activity custom property
   ([#1099](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1099))
+  ([#1128](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1128))
 
 ## 0.4.0-beta.2
 

src/OpenTelemetry.Instrumentation.Grpc/Implementation/GrpcClientDiagnosticListener.cs

@@ -50,12 +50,12 @@ public override void OnStartActivity(Activity activity, object payload)
 
             activity.SetKind(ActivityKind.Client);
             activity.DisplayName = grpcMethod?.Trim('/');
-            activity.SetCustomProperty(RequestCustomPropertyName, request);
 
             this.activitySource.Start(activity);
 
             if (activity.IsAllDataRequested)
             {
+                activity.SetCustomProperty(RequestCustomPropertyName, request);
                 activity.SetTag(SemanticConventions.AttributeRpcSystem, GrpcTagHelper.RpcSystemGrpc);
 
                 if (GrpcTagHelper.TryParseRpcServiceAndRpcMethod(grpcMethod, out var rpcService, out var rpcMethod))

src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs

@@ -94,12 +94,12 @@ public override void OnStartActivity(Activity activity, object payload)
 
             activity.SetKind(ActivityKind.Client);
             activity.DisplayName = HttpTagHelper.GetOperationNameForHttpMethod(request.Method);
-            activity.SetCustomProperty(RequestCustomPropertyName, request);
 
             this.activitySource.Start(activity);
 
             if (activity.IsAllDataRequested)
             {
+                activity.SetCustomProperty(RequestCustomPropertyName, request);
                 activity.SetTag(SemanticConventions.AttributeHttpMethod, HttpTagHelper.GetNameForHttpMethod(request.Method));
                 activity.SetTag(SemanticConventions.AttributeHttpHost, HttpTagHelper.GetHostTagValueFromRequestUri(request.RequestUri));
                 activity.SetTag(SemanticConventions.AttributeHttpUrl, request.RequestUri.OriginalString);

src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs

@@ -101,10 +101,9 @@ private static void AddRequestTagsAndInstrumentRequest(HttpWebRequest request, A
 
             InstrumentRequest(request, activity);
 
-            activity.SetCustomProperty(RequestCustomPropertyName, request);
-
             if (activity.IsAllDataRequested)
             {
+                activity.SetCustomProperty(RequestCustomPropertyName, request);
                 activity.SetTag(SemanticConventions.AttributeHttpMethod, request.Method);
                 activity.SetTag(SemanticConventions.AttributeHttpHost, HttpTagHelper.GetHostTagValueFromRequestUri(request.RequestUri));
                 activity.SetTag(SemanticConventions.AttributeHttpUrl, request.RequestUri.OriginalString);
@@ -118,10 +117,9 @@ private static void AddRequestTagsAndInstrumentRequest(HttpWebRequest request, A
         [MethodImpl(MethodImplOptions.AggressiveInlining)]
         private static void AddResponseTags(HttpWebResponse response, Activity activity)
         {
-            activity.SetCustomProperty(ResponseCustomPropertyName, response);
-
             if (activity.IsAllDataRequested)
             {
+                activity.SetCustomProperty(ResponseCustomPropertyName, response);
                 activity.SetTag(SemanticConventions.AttributeHttpStatusCode, (int)response.StatusCode);
 
                 activity.SetStatus(
@@ -134,13 +132,13 @@ private static void AddResponseTags(HttpWebResponse response, Activity activity)
         [MethodImpl(MethodImplOptions.AggressiveInlining)]
         private static void AddExceptionTags(Exception exception, Activity activity)
         {
-            activity.SetCustomProperty(ExceptionCustomPropertyName, exception);
-
             if (!activity.IsAllDataRequested)
             {
                 return;
             }
 
+            activity.SetCustomProperty(ExceptionCustomPropertyName, exception);
+
             Status status;
             if (exception is WebException wexc)
             {

src/OpenTelemetry.Instrumentation.SqlClient/Implementation/SqlClientDiagnosticListener.cs

@@ -83,14 +83,13 @@ public override void OnCustom(string name, Activity activity, object payload)
                             return;
                         }
 
-                        var connection = this.connectionFetcher.Fetch(command);
-                        var database = this.databaseFetcher.Fetch(connection);
-
-                        activity.DisplayName = (string)database;
-                        activity.SetCustomProperty(CommandCustomPropertyName, command);
-
                         if (activity.IsAllDataRequested)
                         {
+                            var connection = this.connectionFetcher.Fetch(command);
+                            var database = this.databaseFetcher.Fetch(connection);
+
+                            activity.DisplayName = (string)database;
+                            activity.SetCustomProperty(CommandCustomPropertyName, command);
                             var dataSource = this.dataSourceFetcher.Fetch(connection);
                             var commandText = this.commandTextFetcher.Fetch(command);
 

test/OpenTelemetry.Instrumentation.AspNet.Tests/HttpInListenerTests.cs

@@ -24,6 +24,7 @@
 using System.Web.Routing;
 using Moq;
 using OpenTelemetry.Context.Propagation;
+using OpenTelemetry.Instrumentation.AspNet.Implementation;
 using OpenTelemetry.Trace;
 using Xunit;
 
@@ -265,6 +266,13 @@ public void AspNetRequestsAreCollectedSuccessfully(
                 span.Tags.FirstOrDefault(i => i.Key == SemanticConventions.AttributeHttpUserAgent).Value as string);
 
             Assert.Equal(expectedResource, span.GetResource());
+            var request = span.GetCustomProperty(HttpInListener.RequestCustomPropertyName);
+            Assert.NotNull(request);
+            Assert.True(request is HttpRequest);
+
+            var response = span.GetCustomProperty(HttpInListener.ResponseCustomPropertyName);
+            Assert.NotNull(response);
+            Assert.True(response is HttpResponse);
         }
 
         private class FakeAspNetDiagnosticSource : IDisposable

test/OpenTelemetry.Instrumentation.AspNetCore.Tests/BasicTests.cs

@@ -27,6 +27,7 @@
 using Microsoft.Extensions.DependencyInjection;
 using Moq;
 using OpenTelemetry.Context.Propagation;
+using OpenTelemetry.Instrumentation.AspNetCore.Implementation;
 using OpenTelemetry.Trace;
 #if NETCOREAPP2_1
 using TestApp.AspNetCore._2._1;
@@ -60,13 +61,13 @@ public void AddAspNetCoreInstrumentation_BadArgs()
         public async Task SuccessfulTemplateControllerCallGeneratesASpan()
         {
             var expectedResource = Resources.Resources.CreateServiceResource("test-service");
-            var spanProcessor = new Mock<ActivityProcessor>();
+            var activityProcessor = new Mock<ActivityProcessor>();
             void ConfigureTestServices(IServiceCollection services)
             {
                 this.openTelemetrySdk = Sdk.CreateTracerProviderBuilder()
                     .AddAspNetCoreInstrumentation()
                     .SetResource(expectedResource)
-                    .AddProcessor(spanProcessor.Object)
+                    .AddProcessor(activityProcessor.Object)
                     .Build();
             }
 
@@ -82,21 +83,19 @@ void ConfigureTestServices(IServiceCollection services)
                 // Assert
                 response.EnsureSuccessStatusCode(); // Status Code 200-299
 
-                WaitForProcessorInvocations(spanProcessor, 2);
+                WaitForProcessorInvocations(activityProcessor, 2);
             }
 
-            Assert.Equal(2, spanProcessor.Invocations.Count); // begin and end was called
-            var span = (Activity)spanProcessor.Invocations[1].Arguments[0];
+            Assert.Equal(2, activityProcessor.Invocations.Count); // begin and end was called
+            var activity = (Activity)activityProcessor.Invocations[1].Arguments[0];
 
-            Assert.Equal(ActivityKind.Server, span.Kind);
-            Assert.Equal("/api/values", span.Tags.FirstOrDefault(i => i.Key == SpanAttributeConstants.HttpPathKey).Value);
-            Assert.Equal(expectedResource, span.GetResource());
+            ValidateAspNetCoreActivity(activity, "/api/values", expectedResource);
         }
 
         [Fact]
         public async Task SuccessfulTemplateControllerCallUsesParentContext()
         {
-            var spanProcessor = new Mock<ActivityProcessor>();
+            var activityProcessor = new Mock<ActivityProcessor>();
 
             var expectedTraceId = ActivityTraceId.CreateRandom();
             var expectedSpanId = ActivitySpanId.CreateRandom();
@@ -107,7 +106,7 @@ public async Task SuccessfulTemplateControllerCallUsesParentContext()
                     builder.ConfigureTestServices(services =>
                     {
                         this.openTelemetrySdk = Sdk.CreateTracerProviderBuilder().AddAspNetCoreInstrumentation()
-                        .AddProcessor(spanProcessor.Object)
+                        .AddProcessor(activityProcessor.Object)
                         .Build();
                     })))
             {
@@ -121,11 +120,11 @@ public async Task SuccessfulTemplateControllerCallUsesParentContext()
                 // Assert
                 response.EnsureSuccessStatusCode(); // Status Code 200-299
 
-                WaitForProcessorInvocations(spanProcessor, 2);
+                WaitForProcessorInvocations(activityProcessor, 2);
             }
 
-            Assert.Equal(2, spanProcessor.Invocations.Count); // begin and end was called
-            var span = (Activity)spanProcessor.Invocations[1].Arguments[0];
+            Assert.Equal(2, activityProcessor.Invocations.Count); // begin and end was called
+            var span = (Activity)activityProcessor.Invocations[1].Arguments[0];
 
             Assert.Equal(ActivityKind.Server, span.Kind);
             Assert.Equal("api/Values/{id}", span.DisplayName);
@@ -138,7 +137,7 @@ public async Task SuccessfulTemplateControllerCallUsesParentContext()
         [Fact]
         public async Task CustomTextFormat()
         {
-            var spanProcessor = new Mock<ActivityProcessor>();
+            var activityProcessor = new Mock<ActivityProcessor>();
 
             var expectedTraceId = ActivityTraceId.CreateRandom();
             var expectedSpanId = ActivitySpanId.CreateRandom();
@@ -157,20 +156,20 @@ public async Task CustomTextFormat()
                     {
                         this.openTelemetrySdk = Sdk.CreateTracerProviderBuilder()
                             .AddAspNetCoreInstrumentation((opt) => opt.TextFormat = textFormat.Object)
-                            .AddProcessor(spanProcessor.Object)
+                            .AddProcessor(activityProcessor.Object)
                             .Build();
                     })))
             {
                 using var client = testFactory.CreateClient();
                 var response = await client.GetAsync("/api/values/2");
                 response.EnsureSuccessStatusCode(); // Status Code 200-299
 
-                WaitForProcessorInvocations(spanProcessor, 2);
+                WaitForProcessorInvocations(activityProcessor, 2);
             }
 
             // begin and end was called once each.
-            Assert.Equal(2, spanProcessor.Invocations.Count);
-            var span = (Activity)spanProcessor.Invocations[1].Arguments[0];
+            Assert.Equal(2, activityProcessor.Invocations.Count);
+            var span = (Activity)activityProcessor.Invocations[1].Arguments[0];
 
             Assert.Equal(ActivityKind.Server, span.Kind);
             Assert.True(span.Duration != TimeSpan.Zero);
@@ -184,13 +183,13 @@ public async Task CustomTextFormat()
         [Fact]
         public async Task FilterOutRequest()
         {
-            var spanProcessor = new Mock<ActivityProcessor>();
+            var activityProcessor = new Mock<ActivityProcessor>();
 
             void ConfigureTestServices(IServiceCollection services)
             {
                 this.openTelemetrySdk = Sdk.CreateTracerProviderBuilder()
                     .AddAspNetCoreInstrumentation((opt) => opt.RequestFilter = (ctx) => ctx.Request.Path != "/api/values/2")
-                    .AddProcessor(spanProcessor.Object)
+                    .AddProcessor(activityProcessor.Object)
                     .Build();
             }
 
@@ -209,12 +208,12 @@ void ConfigureTestServices(IServiceCollection services)
                 response1.EnsureSuccessStatusCode(); // Status Code 200-299
                 response2.EnsureSuccessStatusCode(); // Status Code 200-299
 
-                WaitForProcessorInvocations(spanProcessor, 2);
+                WaitForProcessorInvocations(activityProcessor, 2);
             }
 
             // we should only create one span and never call processor with another
-            Assert.Equal(2, spanProcessor.Invocations.Count); // begin and end was called
-            var span = (Activity)spanProcessor.Invocations[1].Arguments[0];
+            Assert.Equal(2, activityProcessor.Invocations.Count); // begin and end was called
+            var span = (Activity)activityProcessor.Invocations[1].Arguments[0];
 
             Assert.Equal(ActivityKind.Server, span.Kind);
             Assert.Equal("/api/values", span.Tags.FirstOrDefault(i => i.Key == SpanAttributeConstants.HttpPathKey).Value);
@@ -225,7 +224,7 @@ public void Dispose()
             this.openTelemetrySdk?.Dispose();
         }
 
-        private static void WaitForProcessorInvocations(Mock<ActivityProcessor> spanProcessor, int invocationCount)
+        private static void WaitForProcessorInvocations(Mock<ActivityProcessor> activityProcessor, int invocationCount)
         {
             // 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
@@ -234,9 +233,23 @@ private static void WaitForProcessorInvocations(Mock<ActivityProcessor> spanProc
                 () =>
                 {
                     Thread.Sleep(10);
-                    return spanProcessor.Invocations.Count >= invocationCount;
+                    return activityProcessor.Invocations.Count >= invocationCount;
                 },
                 TimeSpan.FromSeconds(1)));
         }
+
+        private static void ValidateAspNetCoreActivity(Activity activityToValidate, string expectedHttpPath, Resources.Resource expectedResource)
+        {
+            Assert.Equal(ActivityKind.Server, activityToValidate.Kind);
+            Assert.Equal(expectedHttpPath, activityToValidate.Tags.FirstOrDefault(i => i.Key == SpanAttributeConstants.HttpPathKey).Value);
+            Assert.Equal(expectedResource, activityToValidate.GetResource());
+            var request = activityToValidate.GetCustomProperty(HttpInListener.RequestCustomPropertyName);
+            Assert.NotNull(request);
+            Assert.True(request is HttpRequest);
+
+            var response = activityToValidate.GetCustomProperty(HttpInListener.ResponseCustomPropertyName);
+            Assert.NotNull(response);
+            Assert.True(response is HttpResponse);
+        }
     }
 }