Update grpc client enrich callbacks (#3804)

src/OpenTelemetry.Instrumentation.GrpcNetClient/.publicApi/netstandard2.0/PublicAPI.Unshipped.txt

@@ -1,6 +1,8 @@
 OpenTelemetry.Instrumentation.GrpcNetClient.GrpcClientInstrumentationOptions
-OpenTelemetry.Instrumentation.GrpcNetClient.GrpcClientInstrumentationOptions.Enrich.get -> System.Action<System.Diagnostics.Activity, string, object>
-OpenTelemetry.Instrumentation.GrpcNetClient.GrpcClientInstrumentationOptions.Enrich.set -> void
+OpenTelemetry.Instrumentation.GrpcNetClient.GrpcClientInstrumentationOptions.EnrichWithHttpRequestMessage.get -> System.Action<System.Diagnostics.Activity, System.Net.Http.HttpRequestMessage>
+OpenTelemetry.Instrumentation.GrpcNetClient.GrpcClientInstrumentationOptions.EnrichWithHttpRequestMessage.set -> void
+OpenTelemetry.Instrumentation.GrpcNetClient.GrpcClientInstrumentationOptions.EnrichWithHttpResponseMessage.get -> System.Action<System.Diagnostics.Activity, System.Net.Http.HttpResponseMessage>
+OpenTelemetry.Instrumentation.GrpcNetClient.GrpcClientInstrumentationOptions.EnrichWithHttpResponseMessage.set -> void
 OpenTelemetry.Instrumentation.GrpcNetClient.GrpcClientInstrumentationOptions.GrpcClientInstrumentationOptions() -> void
 OpenTelemetry.Instrumentation.GrpcNetClient.GrpcClientInstrumentationOptions.SuppressDownstreamInstrumentation.get -> bool
 OpenTelemetry.Instrumentation.GrpcNetClient.GrpcClientInstrumentationOptions.SuppressDownstreamInstrumentation.set -> void

src/OpenTelemetry.Instrumentation.GrpcNetClient/.publicApi/netstandard2.1/PublicAPI.Unshipped.txt

@@ -1,6 +1,8 @@
 OpenTelemetry.Instrumentation.GrpcNetClient.GrpcClientInstrumentationOptions
-OpenTelemetry.Instrumentation.GrpcNetClient.GrpcClientInstrumentationOptions.Enrich.get -> System.Action<System.Diagnostics.Activity, string, object>
-OpenTelemetry.Instrumentation.GrpcNetClient.GrpcClientInstrumentationOptions.Enrich.set -> void
+OpenTelemetry.Instrumentation.GrpcNetClient.GrpcClientInstrumentationOptions.EnrichWithHttpRequestMessage.get -> System.Action<System.Diagnostics.Activity, System.Net.Http.HttpRequestMessage>
+OpenTelemetry.Instrumentation.GrpcNetClient.GrpcClientInstrumentationOptions.EnrichWithHttpRequestMessage.set -> void
+OpenTelemetry.Instrumentation.GrpcNetClient.GrpcClientInstrumentationOptions.EnrichWithHttpResponseMessage.get -> System.Action<System.Diagnostics.Activity, System.Net.Http.HttpResponseMessage>
+OpenTelemetry.Instrumentation.GrpcNetClient.GrpcClientInstrumentationOptions.EnrichWithHttpResponseMessage.set -> void
 OpenTelemetry.Instrumentation.GrpcNetClient.GrpcClientInstrumentationOptions.GrpcClientInstrumentationOptions() -> void
 OpenTelemetry.Instrumentation.GrpcNetClient.GrpcClientInstrumentationOptions.SuppressDownstreamInstrumentation.get -> bool
 OpenTelemetry.Instrumentation.GrpcNetClient.GrpcClientInstrumentationOptions.SuppressDownstreamInstrumentation.set -> void

src/OpenTelemetry.Instrumentation.GrpcNetClient/CHANGELOG.md

@@ -2,6 +2,17 @@
 
 ## Unreleased
 
+ **Breaking change** The `Enrich` callback option has been removed. For better
+  usability, it has been replaced by two separate options:
+  `EnrichWithHttpRequestMessage`and `EnrichWithHttpResponseMessage`. Previously,
+  the single `Enrich` callback required the consumer to detect which event
+  triggered the callback to be invoked (e.g., request start or response end) and
+  then cast the object received to the appropriate type: `HttpRequestMessage`
+  and `HttpResponseMessage`. The separate callbacks make it clear what event
+  triggers them and there is no longer the need to cast the argument to the
+  expected type.
+  ([#3804](https://github.com/open-telemetry/opentelemetry-dotnet/pull/3804))
+
 ## 1.0.0-rc9.8
 
 Released 2022-Oct-17

src/OpenTelemetry.Instrumentation.GrpcNetClient/GrpcClientInstrumentationOptions.cs

@@ -16,6 +16,7 @@
 
 using System;
 using System.Diagnostics;
+using System.Net.Http;
 
 namespace OpenTelemetry.Instrumentation.GrpcNetClient
 {
@@ -30,14 +31,21 @@ public class GrpcClientInstrumentationOptions
         public bool SuppressDownstreamInstrumentation { get; set; }
 
         /// <summary>
-        /// Gets or sets an action to enrich an Activity.
+        /// Gets or sets an action to enrich the Activity with <see cref="HttpRequestMessage"/>.
         /// </summary>
         /// <remarks>
         /// <para><see cref="Activity"/>: the activity being enriched.</para>
-        /// <para>string: the name of the event.</para>
-        /// <para>object: the raw object from which additional information can be extracted to enrich the activity.
-        /// The type of this object depends on the event, which is given by the above parameter.</para>
+        /// <para><see cref="HttpRequestMessage"/> object from which additional information can be extracted to enrich the activity.</para>
         /// </remarks>
-        public Action<Activity, string, object> Enrich { get; set; }
+        public Action<Activity, HttpRequestMessage> EnrichWithHttpRequestMessage { get; set; }
+
+        /// <summary>
+        /// Gets or sets an action to enrich an Activity with <see cref="HttpResponseMessage"/>.
+        /// </summary>
+        /// <remarks>
+        /// <para><see cref="Activity"/>: the activity being enriched.</para>
+        /// <para><see cref="HttpResponseMessage"/> object from which additional information can be extracted to enrich the activity.</para>
+        /// </remarks>
+        public Action<Activity, HttpResponseMessage> EnrichWithHttpResponseMessage { get; set; }
     }
 }

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

@@ -150,7 +150,7 @@ public void OnStartActivity(Activity activity, object payload)
 
                 try
                 {
-                    this.options.Enrich?.Invoke(activity, "OnStartActivity", request);
+                    this.options.EnrichWithHttpRequestMessage?.Invoke(activity, request);
                 }
                 catch (Exception ex)
                 {
@@ -182,7 +182,7 @@ public void OnStopActivity(Activity activity, object payload)
                 {
                     try
                     {
-                        this.options.Enrich?.Invoke(activity, "OnStopActivity", response);
+                        this.options.EnrichWithHttpResponseMessage?.Invoke(activity, response);
                     }
                     catch (Exception ex)
                     {

src/OpenTelemetry.Instrumentation.GrpcNetClient/README.md

@@ -99,31 +99,29 @@ using var tracerProvider = Sdk.CreateTracerProviderBuilder()
 
 ### Enrich
 
-This option allows one to enrich the activity with additional information
-from the raw `HttpRequestMessage` object. The `Enrich` action is called only
-when `activity.IsAllDataRequested` is `true`. It contains the activity itself
-(which can be enriched), the name of the event, and the actual raw object.
-For event name "OnStartActivity", the actual object will be
-`HttpRequestMessage`.
-
-The following code snippet shows how to add additional tags using `Enrich`.
+This instrumentation library provides `EnrichWithHttpRequestMessage` and
+`EnrichWithHttpResponseMessage` options that can be used to enrich the activity
+with additional information from the raw `HttpRequestMessage` and
+`HttpResponseMessage` objects respectively. These actions are called only when
+`activity.IsAllDataRequested` is `true`. It contains the activity itself (which
+can be enriched), the name of the event, and the actual raw object. The
+following code snippet shows how to add additional tags using these options.
 
 ```csharp
 services.AddOpenTelemetryTracing((builder) =>
 {
     builder
-    .AddGrpcClientInstrumentation(opt => opt.Enrich
-        = (activity, eventName, rawObject) =>
+    .AddGrpcClientInstrumentation((options) =>
     {
-        if (eventName.Equals("OnStartActivity"))
+        options.EnrichWithHttpRequestMessage = (activity, httpRequestMessage) =>
+        {
+            activity.SetTag("requestVersion", httpRequestMessage.Version);
+        };
+        options.EnrichWithHttpResponseMessage = (activity, httpResponseMessage) =>
         {
-            if (rawObject is HttpRequestMessage request)
-            {
-                activity.SetTag("requestVersion", request.Version);
-            }
-        }
+            activity.SetTag("responseVersion", httpResponseMessage.Version);
+        };
     })
-});
 ```
 
 [Processor](../../docs/trace/extending-the-sdk/README.md#processor),

test/OpenTelemetry.Instrumentation.Grpc.Tests/GrpcTests.client.cs

@@ -24,9 +24,6 @@
 using Grpc.Core;
 using Grpc.Net.Client;
 using Microsoft.Extensions.DependencyInjection;
-#if NET6_0_OR_GREATER
-using Microsoft.AspNetCore.Http;
-#endif
 using Moq;
 using OpenTelemetry.Context.Propagation;
 using OpenTelemetry.Instrumentation.Grpc.Tests.GrpcTestHelpers;
@@ -49,6 +46,9 @@ public partial class GrpcTests
         [InlineData("http://[::1]", false)]
         public void GrpcClientCallsAreCollectedSuccessfully(string baseAddress, bool shouldEnrich = true)
         {
+            bool enrichWithHttpRequestMessageCalled = false;
+            bool enrichWithHttpResponseMessageCalled = false;
+
             var uri = new Uri($"{baseAddress}:1234");
             var uriHostNameType = Uri.CheckHostName(uri.Host);
 
@@ -72,7 +72,8 @@ public void GrpcClientCallsAreCollectedSuccessfully(string baseAddress, bool sho
                     {
                         if (shouldEnrich)
                         {
-                            options.Enrich = ActivityEnrichment;
+                            options.EnrichWithHttpRequestMessage = (activity, httpRequestMessage) => { enrichWithHttpRequestMessageCalled = true; };
+                            options.EnrichWithHttpResponseMessage = (activity, httpResponseMessage) => { enrichWithHttpResponseMessageCalled = true; };
                         }
                     })
                     .AddProcessor(processor.Object)
@@ -118,6 +119,12 @@ public void GrpcClientCallsAreCollectedSuccessfully(string baseAddress, bool sho
             Assert.Null(activity.GetTagValue(GrpcTagHelper.GrpcMethodTagName));
             Assert.Null(activity.GetTagValue(GrpcTagHelper.GrpcStatusCodeTagName));
             Assert.Equal(0, activity.GetTagValue(SemanticConventions.AttributeRpcGrpcStatusCode));
+
+            if (shouldEnrich)
+            {
+                Assert.True(enrichWithHttpRequestMessageCalled);
+                Assert.True(enrichWithHttpResponseMessageCalled);
+            }
         }
 
 #if NET6_0_OR_GREATER
@@ -128,7 +135,12 @@ public void GrpcAndHttpClientInstrumentationIsInvoked(bool shouldEnrich)
         {
             var uri = new Uri($"http://localhost:{this.server.Port}");
             var processor = new Mock<BaseProcessor<Activity>>();
-            processor.Setup(x => x.OnStart(It.IsAny<Activity>())).Callback<Activity>(c => c.SetTag("enriched", "no"));
+            processor.Setup(x => x.OnStart(It.IsAny<Activity>())).Callback<Activity>(c =>
+            {
+                c.SetTag("enrichedWithHttpRequestMessage", "no");
+                c.SetTag("enrichedWithHttpResponseMessage", "no");
+            });
+
             var parent = new Activity("parent")
                 .Start();
 
@@ -138,7 +150,15 @@ public void GrpcAndHttpClientInstrumentationIsInvoked(bool shouldEnrich)
                     {
                         if (shouldEnrich)
                         {
-                            options.Enrich = ActivityEnrichment;
+                            options.EnrichWithHttpRequestMessage = (activity, httpRequestMessage) =>
+                            {
+                                activity.SetTag("enrichedWithHttpRequestMessage", "yes");
+                            };
+
+                            options.EnrichWithHttpResponseMessage = (activity, httpResponseMessage) =>
+                            {
+                                activity.SetTag("enrichedWithHttpResponseMessage", "yes");
+                            };
                         }
                     })
                     .AddHttpClientInstrumentation()
@@ -166,14 +186,14 @@ public void GrpcAndHttpClientInstrumentationIsInvoked(bool shouldEnrich)
             Assert.Equal($"HTTP POST", httpSpan.DisplayName);
             Assert.Equal(grpcSpan.SpanId, httpSpan.ParentSpanId);
 
-            Assert.NotEmpty(grpcSpan.Tags.Where(tag => tag.Key == "enriched"));
-            Assert.Equal(shouldEnrich ? "yes" : "no", grpcSpan.Tags.Where(tag => tag.Key == "enriched").FirstOrDefault().Value);
+            Assert.NotEmpty(grpcSpan.Tags.Where(tag => tag.Key == "enrichedWithHttpRequestMessage"));
+            Assert.NotEmpty(grpcSpan.Tags.Where(tag => tag.Key == "enrichedWithHttpResponseMessage"));
+            Assert.Equal(shouldEnrich ? "yes" : "no", grpcSpan.Tags.Where(tag => tag.Key == "enrichedWithHttpRequestMessage").FirstOrDefault().Value);
+            Assert.Equal(shouldEnrich ? "yes" : "no", grpcSpan.Tags.Where(tag => tag.Key == "enrichedWithHttpResponseMessage").FirstOrDefault().Value);
         }
 
-        [Theory]
-        [InlineData(true)]
-        [InlineData(false)]
-        public void GrpcAndHttpClientInstrumentationWithSuppressInstrumentation(bool shouldEnrich)
+        [Fact]
+        public void GrpcAndHttpClientInstrumentationWithSuppressInstrumentation()
         {
             var uri = new Uri($"http://localhost:{this.server.Port}");
             var processor = new Mock<BaseProcessor<Activity>>();
@@ -183,14 +203,7 @@ public void GrpcAndHttpClientInstrumentationWithSuppressInstrumentation(bool sho
 
             using (Sdk.CreateTracerProviderBuilder()
                     .SetSampler(new AlwaysOnSampler())
-                    .AddGrpcClientInstrumentation(o =>
-                    {
-                        o.SuppressDownstreamInstrumentation = true;
-                        if (shouldEnrich)
-                        {
-                            o.Enrich = ActivityEnrichment;
-                        }
-                    })
+                    .AddGrpcClientInstrumentation(o => o.SuppressDownstreamInstrumentation = true)
                     .AddHttpClientInstrumentation()
                     .AddProcessor(processor.Object)
                     .Build())
@@ -470,26 +483,6 @@ private static void ValidateGrpcActivity(Activity activityToValidate)
             Assert.Equal(ActivityKind.Client, activityToValidate.Kind);
         }
 
-        private static void ActivityEnrichment(Activity activity, string method, object obj)
-        {
-            Assert.True(activity.IsAllDataRequested);
-            switch (method)
-            {
-                case "OnStartActivity":
-                    Assert.True(obj is HttpRequestMessage);
-                    break;
-
-                case "OnStopActivity":
-                    Assert.True(obj is HttpResponseMessage);
-                    break;
-
-                default:
-                    break;
-            }
-
-            activity.SetTag("enriched", "yes");
-        }
-
         private static Predicate<IInvocation> GeneratePredicateForMoqProcessorActivity(string methodName, string activityOperationName)
         {
             return invo => invo.Method.Name == methodName && (invo.Arguments[0] as Activity)?.OperationName == activityOperationName;