From 4261b76ae00900a20e85a0d37308ccdfcee89d9f Mon Sep 17 00:00:00 2001 From: Vishwesh Bankwar Date: Fri, 21 Oct 2022 12:49:37 -0700 Subject: [PATCH 1/7] update grpc enrich callbacks --- .../netstandard2.0/PublicAPI.Unshipped.txt | 6 +- .../netstandard2.1/PublicAPI.Unshipped.txt | 6 +- .../GrpcClientInstrumentationOptions.cs | 18 +++-- .../GrpcClientDiagnosticListener.cs | 4 +- .../GrpcTests.client.cs | 73 +++++++++---------- 5 files changed, 56 insertions(+), 51 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.GrpcNetClient/.publicApi/netstandard2.0/PublicAPI.Unshipped.txt b/src/OpenTelemetry.Instrumentation.GrpcNetClient/.publicApi/netstandard2.0/PublicAPI.Unshipped.txt index 82eecf60816..db708294cc8 100644 --- a/src/OpenTelemetry.Instrumentation.GrpcNetClient/.publicApi/netstandard2.0/PublicAPI.Unshipped.txt +++ b/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 -OpenTelemetry.Instrumentation.GrpcNetClient.GrpcClientInstrumentationOptions.Enrich.set -> void +OpenTelemetry.Instrumentation.GrpcNetClient.GrpcClientInstrumentationOptions.EnrichWithHttpRequestMessage.get -> System.Action +OpenTelemetry.Instrumentation.GrpcNetClient.GrpcClientInstrumentationOptions.EnrichWithHttpRequestMessage.set -> void +OpenTelemetry.Instrumentation.GrpcNetClient.GrpcClientInstrumentationOptions.EnrichWithHttpResponseMessage.get -> System.Action +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 diff --git a/src/OpenTelemetry.Instrumentation.GrpcNetClient/.publicApi/netstandard2.1/PublicAPI.Unshipped.txt b/src/OpenTelemetry.Instrumentation.GrpcNetClient/.publicApi/netstandard2.1/PublicAPI.Unshipped.txt index 82eecf60816..db708294cc8 100644 --- a/src/OpenTelemetry.Instrumentation.GrpcNetClient/.publicApi/netstandard2.1/PublicAPI.Unshipped.txt +++ b/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 -OpenTelemetry.Instrumentation.GrpcNetClient.GrpcClientInstrumentationOptions.Enrich.set -> void +OpenTelemetry.Instrumentation.GrpcNetClient.GrpcClientInstrumentationOptions.EnrichWithHttpRequestMessage.get -> System.Action +OpenTelemetry.Instrumentation.GrpcNetClient.GrpcClientInstrumentationOptions.EnrichWithHttpRequestMessage.set -> void +OpenTelemetry.Instrumentation.GrpcNetClient.GrpcClientInstrumentationOptions.EnrichWithHttpResponseMessage.get -> System.Action +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 diff --git a/src/OpenTelemetry.Instrumentation.GrpcNetClient/GrpcClientInstrumentationOptions.cs b/src/OpenTelemetry.Instrumentation.GrpcNetClient/GrpcClientInstrumentationOptions.cs index 91735e3a864..ebca6cacaa7 100644 --- a/src/OpenTelemetry.Instrumentation.GrpcNetClient/GrpcClientInstrumentationOptions.cs +++ b/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; } /// - /// Gets or sets an action to enrich an Activity. + /// Gets or sets an action to enrich an Activity with . /// /// /// : the activity being enriched. - /// string: the name of the event. - /// 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. + /// object from which additional information can be extracted to enrich the activity. /// - public Action Enrich { get; set; } + public Action EnrichWithHttpRequestMessage { get; set; } + + /// + /// Gets or sets an action to enrich an Activity with . + /// + /// + /// : the activity being enriched. + /// object from which additional information can be extracted to enrich the activity. + /// + public Action EnrichWithHttpResponseMessage { get; set; } } } diff --git a/src/OpenTelemetry.Instrumentation.GrpcNetClient/Implementation/GrpcClientDiagnosticListener.cs b/src/OpenTelemetry.Instrumentation.GrpcNetClient/Implementation/GrpcClientDiagnosticListener.cs index 299d9ff9f8e..179831d2a67 100644 --- a/src/OpenTelemetry.Instrumentation.GrpcNetClient/Implementation/GrpcClientDiagnosticListener.cs +++ b/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) { diff --git a/test/OpenTelemetry.Instrumentation.Grpc.Tests/GrpcTests.client.cs b/test/OpenTelemetry.Instrumentation.Grpc.Tests/GrpcTests.client.cs index 044f9739f4c..5bfc68657af 100644 --- a/test/OpenTelemetry.Instrumentation.Grpc.Tests/GrpcTests.client.cs +++ b/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>(); - processor.Setup(x => x.OnStart(It.IsAny())).Callback(c => c.SetTag("enriched", "no")); + processor.Setup(x => x.OnStart(It.IsAny())).Callback(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>(); @@ -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() .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 GeneratePredicateForMoqProcessorActivity(string methodName, string activityOperationName) { return invo => invo.Method.Name == methodName && (invo.Arguments[0] as Activity)?.OperationName == activityOperationName; From 1e7ed602c51afe168b8af9f0fecf62324196d587 Mon Sep 17 00:00:00 2001 From: Vishwesh Bankwar Date: Fri, 21 Oct 2022 12:57:27 -0700 Subject: [PATCH 2/7] update readme and changelog --- .../CHANGELOG.md | 11 +++++++ .../README.md | 31 +++++++++---------- 2 files changed, 26 insertions(+), 16 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.GrpcNetClient/CHANGELOG.md b/src/OpenTelemetry.Instrumentation.GrpcNetClient/CHANGELOG.md index 8506f0d6163..49a35cdd09a 100644 --- a/src/OpenTelemetry.Instrumentation.GrpcNetClient/CHANGELOG.md +++ b/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. + ([#]()) + ## 1.0.0-rc9.8 Released 2022-Oct-17 diff --git a/src/OpenTelemetry.Instrumentation.GrpcNetClient/README.md b/src/OpenTelemetry.Instrumentation.GrpcNetClient/README.md index 7dcb2d2c98c..ce1c5d764c1 100644 --- a/src/OpenTelemetry.Instrumentation.GrpcNetClient/README.md +++ b/src/OpenTelemetry.Instrumentation.GrpcNetClient/README.md @@ -99,31 +99,30 @@ 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`. +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 `Enrich`. +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) => { - if (rawObject is HttpRequestMessage request) - { - activity.SetTag("requestVersion", request.Version); - } - } + activity.SetTag("requestVersion", httpRequestMessage.Version); + }; + options.EnrichWithHttpResponseMessage = (activity, httpResponseMessage) => + { + activity.SetTag("responseVersion", httpResponseMessage.Version); + }; }) -}); ``` [Processor](../../docs/trace/extending-the-sdk/README.md#processor), From 92e0b38cbc0917dad17f1214b6aab46736fb1b8a Mon Sep 17 00:00:00 2001 From: Vishwesh Bankwar Date: Fri, 21 Oct 2022 12:58:15 -0700 Subject: [PATCH 3/7] pr # --- src/OpenTelemetry.Instrumentation.GrpcNetClient/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/OpenTelemetry.Instrumentation.GrpcNetClient/CHANGELOG.md b/src/OpenTelemetry.Instrumentation.GrpcNetClient/CHANGELOG.md index 49a35cdd09a..08545533222 100644 --- a/src/OpenTelemetry.Instrumentation.GrpcNetClient/CHANGELOG.md +++ b/src/OpenTelemetry.Instrumentation.GrpcNetClient/CHANGELOG.md @@ -11,7 +11,7 @@ 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 From 17f0151fc4211ba391966c3e03112c68ffffff20 Mon Sep 17 00:00:00 2001 From: Vishwesh Bankwar Date: Fri, 21 Oct 2022 13:01:37 -0700 Subject: [PATCH 4/7] fix md error --- src/OpenTelemetry.Instrumentation.GrpcNetClient/README.md | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.GrpcNetClient/README.md b/src/OpenTelemetry.Instrumentation.GrpcNetClient/README.md index ce1c5d764c1..d2cf5b117b3 100644 --- a/src/OpenTelemetry.Instrumentation.GrpcNetClient/README.md +++ b/src/OpenTelemetry.Instrumentation.GrpcNetClient/README.md @@ -104,9 +104,8 @@ This instrumentation library provides `EnrichWithHttpRequestMessage` and 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. +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) => From ecb947edb8348f5addbaadb76292878880492b7b Mon Sep 17 00:00:00 2001 From: Vishwesh Bankwar Date: Fri, 21 Oct 2022 13:03:06 -0700 Subject: [PATCH 5/7] nit --- src/OpenTelemetry.Instrumentation.GrpcNetClient/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/OpenTelemetry.Instrumentation.GrpcNetClient/README.md b/src/OpenTelemetry.Instrumentation.GrpcNetClient/README.md index d2cf5b117b3..462f5439321 100644 --- a/src/OpenTelemetry.Instrumentation.GrpcNetClient/README.md +++ b/src/OpenTelemetry.Instrumentation.GrpcNetClient/README.md @@ -103,7 +103,7 @@ 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 +`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. From b1525b0f4b0543ec9816d49fb7d22e5ac5d7533d Mon Sep 17 00:00:00 2001 From: Vishwesh Bankwar Date: Fri, 21 Oct 2022 13:36:27 -0700 Subject: [PATCH 6/7] fix test --- .../GrpcTests.client.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/OpenTelemetry.Instrumentation.Grpc.Tests/GrpcTests.client.cs b/test/OpenTelemetry.Instrumentation.Grpc.Tests/GrpcTests.client.cs index 5bfc68657af..89525480fbb 100644 --- a/test/OpenTelemetry.Instrumentation.Grpc.Tests/GrpcTests.client.cs +++ b/test/OpenTelemetry.Instrumentation.Grpc.Tests/GrpcTests.client.cs @@ -203,7 +203,7 @@ public void GrpcAndHttpClientInstrumentationWithSuppressInstrumentation() using (Sdk.CreateTracerProviderBuilder() .SetSampler(new AlwaysOnSampler()) - .AddGrpcClientInstrumentation() + .AddGrpcClientInstrumentation(o => o.SuppressDownstreamInstrumentation = true) .AddHttpClientInstrumentation() .AddProcessor(processor.Object) .Build()) From 7db2512d435c74843e93749600ea56cd2683c2ae Mon Sep 17 00:00:00 2001 From: Vishwesh Bankwar Date: Fri, 21 Oct 2022 14:11:07 -0700 Subject: [PATCH 7/7] Update src/OpenTelemetry.Instrumentation.GrpcNetClient/GrpcClientInstrumentationOptions.cs Co-authored-by: Cijo Thomas --- .../GrpcClientInstrumentationOptions.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/OpenTelemetry.Instrumentation.GrpcNetClient/GrpcClientInstrumentationOptions.cs b/src/OpenTelemetry.Instrumentation.GrpcNetClient/GrpcClientInstrumentationOptions.cs index ebca6cacaa7..8a055128b03 100644 --- a/src/OpenTelemetry.Instrumentation.GrpcNetClient/GrpcClientInstrumentationOptions.cs +++ b/src/OpenTelemetry.Instrumentation.GrpcNetClient/GrpcClientInstrumentationOptions.cs @@ -31,7 +31,7 @@ public class GrpcClientInstrumentationOptions public bool SuppressDownstreamInstrumentation { get; set; } /// - /// Gets or sets an action to enrich an Activity with . + /// Gets or sets an action to enrich the Activity with . /// /// /// : the activity being enriched.