From 02a7ddf8bb0448adac4ec7ccc18234f169f8225e Mon Sep 17 00:00:00 2001 From: Ilia Brahinets Date: Mon, 24 Jun 2024 07:57:07 +0300 Subject: [PATCH] [Instrumentation.GrpcCore] Use shared semantic conventions (#1917) --- .../ClientTracingInterceptor.cs | 13 +------ .../ClientTracingInterceptorOptions.cs | 6 +-- ...nTelemetry.Instrumentation.GrpcCore.csproj | 1 + .../RpcScope.cs | 2 +- .../SemanticConventions.cs | 30 -------------- .../ServerTracingInterceptor.cs | 12 +----- .../ServerTracingInterceptorOptions.cs | 6 +-- .../GrpcCoreClientInterceptorTests.cs | 30 ++++++++------ .../GrpcCoreServerInterceptorTests.cs | 12 +++--- .../InterceptorActivityListener.cs | 7 ++-- .../TestActivityTags.cs | 39 +++++++++++++++++++ 11 files changed, 77 insertions(+), 81 deletions(-) delete mode 100644 src/OpenTelemetry.Instrumentation.GrpcCore/SemanticConventions.cs create mode 100644 test/OpenTelemetry.Instrumentation.GrpcCore.Tests/TestActivityTags.cs diff --git a/src/OpenTelemetry.Instrumentation.GrpcCore/ClientTracingInterceptor.cs b/src/OpenTelemetry.Instrumentation.GrpcCore/ClientTracingInterceptor.cs index bfb982dee1..80933e0e6a 100644 --- a/src/OpenTelemetry.Instrumentation.GrpcCore/ClientTracingInterceptor.cs +++ b/src/OpenTelemetry.Instrumentation.GrpcCore/ClientTracingInterceptor.cs @@ -2,7 +2,6 @@ // SPDX-License-Identifier: Apache-2.0 using System; -using System.Collections.Generic; using System.Diagnostics; using System.Threading.Tasks; using Grpc.Core; @@ -311,16 +310,6 @@ public ClientRpcScope(ClientInterceptorContext context, Cli return; } - // This if block is for unit testing only. - IEnumerable> customTags = null; - if (options.ActivityIdentifierValue != default) - { - customTags = new List> - { - new KeyValuePair(SemanticConventions.AttributeActivityIdentifier, options.ActivityIdentifierValue), - }; - } - // We want to start an activity but don't activate it. // After calling StartActivity, Activity.Current will be the new Activity. // This scope is created synchronously before the RPC invocation starts and so this new Activity will overwrite @@ -331,7 +320,7 @@ public ClientRpcScope(ClientInterceptorContext context, Cli this.FullServiceName, ActivityKind.Client, this.parentActivity == default ? default : this.parentActivity.Context, - tags: customTags); + tags: options.AdditionalTags); if (rpcActivity == null) { diff --git a/src/OpenTelemetry.Instrumentation.GrpcCore/ClientTracingInterceptorOptions.cs b/src/OpenTelemetry.Instrumentation.GrpcCore/ClientTracingInterceptorOptions.cs index 73200193d2..72ff93a4fe 100644 --- a/src/OpenTelemetry.Instrumentation.GrpcCore/ClientTracingInterceptorOptions.cs +++ b/src/OpenTelemetry.Instrumentation.GrpcCore/ClientTracingInterceptorOptions.cs @@ -1,7 +1,7 @@ // Copyright The OpenTelemetry Authors // SPDX-License-Identifier: Apache-2.0 -using System; +using System.Collections.Generic; using OpenTelemetry.Context.Propagation; namespace OpenTelemetry.Instrumentation.GrpcCore; @@ -27,7 +27,7 @@ public class ClientTracingInterceptorOptions public TextMapPropagator Propagator { get; internal set; } = Propagators.DefaultTextMapPropagator; /// - /// Gets or sets a custom identifier used during unit testing. + /// Gets or sets additional activity tags used during unit testing. /// - internal Guid ActivityIdentifierValue { get; set; } + internal IReadOnlyDictionary AdditionalTags { get; set; } } diff --git a/src/OpenTelemetry.Instrumentation.GrpcCore/OpenTelemetry.Instrumentation.GrpcCore.csproj b/src/OpenTelemetry.Instrumentation.GrpcCore/OpenTelemetry.Instrumentation.GrpcCore.csproj index 7801f29915..099152bc14 100644 --- a/src/OpenTelemetry.Instrumentation.GrpcCore/OpenTelemetry.Instrumentation.GrpcCore.csproj +++ b/src/OpenTelemetry.Instrumentation.GrpcCore/OpenTelemetry.Instrumentation.GrpcCore.csproj @@ -22,5 +22,6 @@ + diff --git a/src/OpenTelemetry.Instrumentation.GrpcCore/RpcScope.cs b/src/OpenTelemetry.Instrumentation.GrpcCore/RpcScope.cs index 889a761fa2..b09cdf7fd3 100644 --- a/src/OpenTelemetry.Instrumentation.GrpcCore/RpcScope.cs +++ b/src/OpenTelemetry.Instrumentation.GrpcCore/RpcScope.cs @@ -243,7 +243,7 @@ private void AddMessageEvent(string eventName, IMessage message, bool request) { new KeyValuePair("name", "message"), new KeyValuePair(SemanticConventions.AttributeMessageType, request ? "SENT" : "RECEIVED"), - new KeyValuePair(SemanticConventions.AttributeMessageID, request ? this.requestMessageCounter : this.responseMessageCounter), + new KeyValuePair(SemanticConventions.AttributeMessageId, request ? this.requestMessageCounter : this.responseMessageCounter), // TODO how to get the real compressed or uncompressed sizes new KeyValuePair(SemanticConventions.AttributeMessageCompressedSize, messageSize), diff --git a/src/OpenTelemetry.Instrumentation.GrpcCore/SemanticConventions.cs b/src/OpenTelemetry.Instrumentation.GrpcCore/SemanticConventions.cs deleted file mode 100644 index b4463adc93..0000000000 --- a/src/OpenTelemetry.Instrumentation.GrpcCore/SemanticConventions.cs +++ /dev/null @@ -1,30 +0,0 @@ -// Copyright The OpenTelemetry Authors -// SPDX-License-Identifier: Apache-2.0 - -namespace OpenTelemetry.Instrumentation.GrpcCore; - -/// -/// Semantic conventions. -/// -internal static class SemanticConventions -{ -#pragma warning disable SA1600 // Elements should be documented - public const string AttributeRpcSystem = "rpc.system"; - public const string AttributeRpcService = "rpc.service"; - public const string AttributeRpcMethod = "rpc.method"; - public const string AttributeRpcGrpcStatusCode = "rpc.grpc.status_code"; - public const string AttributeMessageType = "message.type"; - public const string AttributeMessageID = "message.id"; - public const string AttributeMessageCompressedSize = "message.compressed_size"; - public const string AttributeMessageUncompressedSize = "message.uncompressed_size"; - - // Used for unit testing only. - internal const string AttributeActivityIdentifier = "activityidentifier"; - - // https://github.com/open-telemetry/semantic-conventions/blob/main/docs/exceptions/exceptions-spans.md - internal const string AttributeExceptionEventName = "exception"; - internal const string AttributeExceptionType = "exception.type"; - internal const string AttributeExceptionMessage = "exception.message"; - internal const string AttributeExceptionStacktrace = "exception.stacktrace"; -#pragma warning restore SA1600 // Elements should be documented -} diff --git a/src/OpenTelemetry.Instrumentation.GrpcCore/ServerTracingInterceptor.cs b/src/OpenTelemetry.Instrumentation.GrpcCore/ServerTracingInterceptor.cs index a1e5d99ed5..682f2e2ef0 100644 --- a/src/OpenTelemetry.Instrumentation.GrpcCore/ServerTracingInterceptor.cs +++ b/src/OpenTelemetry.Instrumentation.GrpcCore/ServerTracingInterceptor.cs @@ -205,21 +205,11 @@ public ServerRpcScope(ServerCallContext context, ServerTracingInterceptorOptions } } - // This if block is for unit testing only. - IEnumerable> customTags = null; - if (options.ActivityIdentifierValue != default) - { - customTags = new List> - { - new KeyValuePair(SemanticConventions.AttributeActivityIdentifier, options.ActivityIdentifierValue), - }; - } - var activity = GrpcCoreInstrumentation.ActivitySource.StartActivity( this.FullServiceName, ActivityKind.Server, currentContext ?? default, - tags: customTags); + tags: options.AdditionalTags); this.SetActivity(activity); } diff --git a/src/OpenTelemetry.Instrumentation.GrpcCore/ServerTracingInterceptorOptions.cs b/src/OpenTelemetry.Instrumentation.GrpcCore/ServerTracingInterceptorOptions.cs index 8d3af09835..d13b705104 100644 --- a/src/OpenTelemetry.Instrumentation.GrpcCore/ServerTracingInterceptorOptions.cs +++ b/src/OpenTelemetry.Instrumentation.GrpcCore/ServerTracingInterceptorOptions.cs @@ -1,7 +1,7 @@ // Copyright The OpenTelemetry Authors // SPDX-License-Identifier: Apache-2.0 -using System; +using System.Collections.Generic; using OpenTelemetry.Context.Propagation; namespace OpenTelemetry.Instrumentation.GrpcCore; @@ -27,7 +27,7 @@ public class ServerTracingInterceptorOptions public TextMapPropagator Propagator { get; internal set; } = Propagators.DefaultTextMapPropagator; /// - /// Gets or sets a custom identfier used during unit testing. + /// Gets or sets additional activity tags used during unit testing. /// - internal Guid ActivityIdentifierValue { get; set; } + internal IReadOnlyDictionary AdditionalTags { get; set; } } diff --git a/test/OpenTelemetry.Instrumentation.GrpcCore.Tests/GrpcCoreClientInterceptorTests.cs b/test/OpenTelemetry.Instrumentation.GrpcCore.Tests/GrpcCoreClientInterceptorTests.cs index e1ec20a73a..4aa61712bc 100644 --- a/test/OpenTelemetry.Instrumentation.GrpcCore.Tests/GrpcCoreClientInterceptorTests.cs +++ b/test/OpenTelemetry.Instrumentation.GrpcCore.Tests/GrpcCoreClientInterceptorTests.cs @@ -10,7 +10,9 @@ using Grpc.Core.Interceptors; using OpenTelemetry.Context.Propagation; using OpenTelemetry.Tests; +using OpenTelemetry.Trace; using Xunit; +using StatusCode = Grpc.Core.StatusCode; namespace OpenTelemetry.Instrumentation.GrpcCore.Tests; @@ -237,7 +239,8 @@ public async Task DownstreamInterceptorActivityAccess() return metadata; }); - var interceptorOptions = new ClientTracingInterceptorOptions { ActivityIdentifierValue = Guid.NewGuid() }; + var testTags = new TestActivityTags(); + var interceptorOptions = new ClientTracingInterceptorOptions { AdditionalTags = testTags.Tags }; callInvoker = callInvoker.Intercept(new ClientTracingInterceptor(interceptorOptions)); var client = new Foobar.FoobarClient(callInvoker); @@ -248,7 +251,7 @@ static void ValidateNewTagOnActivity(InterceptorActivityListener listener) } // Check the blocking async call - using (var activityListener = new InterceptorActivityListener(interceptorOptions.ActivityIdentifierValue)) + using (var activityListener = new InterceptorActivityListener(testTags)) { Assert.Equal(parentActivity, Activity.Current); var response = client.Unary(FoobarService.DefaultRequestMessage); @@ -259,7 +262,7 @@ static void ValidateNewTagOnActivity(InterceptorActivityListener listener) } // Check unary async - using (var activityListener = new InterceptorActivityListener(interceptorOptions.ActivityIdentifierValue)) + using (var activityListener = new InterceptorActivityListener(testTags)) { Assert.Equal(parentActivity, Activity.Current); using var call = client.UnaryAsync(FoobarService.DefaultRequestMessage); @@ -274,7 +277,7 @@ static void ValidateNewTagOnActivity(InterceptorActivityListener listener) } // Check a streaming async call - using (var activityListener = new InterceptorActivityListener(interceptorOptions.ActivityIdentifierValue)) + using (var activityListener = new InterceptorActivityListener(testTags)) { Assert.Equal(parentActivity, Activity.Current); using var call = client.DuplexStreaming(); @@ -343,7 +346,7 @@ static void ValidateCommonEventAttributes(ActivityEvent activityEvent) { Assert.NotNull(activityEvent.Tags); Assert.Contains(activityEvent.Tags, t => t.Key == "name" && (string)t.Value == "message"); - Assert.Contains(activityEvent.Tags, t => t.Key == SemanticConventions.AttributeMessageID && (int)t.Value == 1); + Assert.Contains(activityEvent.Tags, t => t.Key == SemanticConventions.AttributeMessageId && (int)t.Value == 1); } Assert.NotEqual(default, requestMessage); @@ -403,15 +406,16 @@ private static async Task TestHandlerSuccess(Func(async () => await clientRequestFunc(client, null).ConfigureAwait(false)); var activity = activityListener.Activity; @@ -515,8 +520,9 @@ private static async Task TestHandlerFailure( private void TestActivityIsCancelledWhenHandlerDisposed(Action clientRequestAction) { using var server = FoobarService.Start(); - var clientInterceptorOptions = new ClientTracingInterceptorOptions { Propagator = new TraceContextPropagator(), ActivityIdentifierValue = Guid.NewGuid() }; - using var activityListener = new InterceptorActivityListener(clientInterceptorOptions.ActivityIdentifierValue); + var testTags = new TestActivityTags(); + var clientInterceptorOptions = new ClientTracingInterceptorOptions { Propagator = new TraceContextPropagator(), AdditionalTags = testTags.Tags }; + using var activityListener = new InterceptorActivityListener(testTags); var client = FoobarService.ConstructRpcClient(server.UriString, new ClientTracingInterceptor(clientInterceptorOptions)); clientRequestAction(client); diff --git a/test/OpenTelemetry.Instrumentation.GrpcCore.Tests/GrpcCoreServerInterceptorTests.cs b/test/OpenTelemetry.Instrumentation.GrpcCore.Tests/GrpcCoreServerInterceptorTests.cs index 4357262e9e..65b00b4383 100644 --- a/test/OpenTelemetry.Instrumentation.GrpcCore.Tests/GrpcCoreServerInterceptorTests.cs +++ b/test/OpenTelemetry.Instrumentation.GrpcCore.Tests/GrpcCoreServerInterceptorTests.cs @@ -104,11 +104,12 @@ public async Task DuplexStreamingServerHandlerFail() private static async Task TestHandlerSuccess(Func clientRequestFunc, Metadata additionalMetadata = null) { // starts the server with the server interceptor - var interceptorOptions = new ServerTracingInterceptorOptions { Propagator = new TraceContextPropagator(), RecordMessageEvents = true, ActivityIdentifierValue = Guid.NewGuid() }; + var testTags = new TestActivityTags(); + var interceptorOptions = new ServerTracingInterceptorOptions { Propagator = new TraceContextPropagator(), RecordMessageEvents = true, AdditionalTags = testTags.Tags }; using var server = FoobarService.Start(new ServerTracingInterceptor(interceptorOptions)); // No parent Activity, no context from header - using (var activityListener = new InterceptorActivityListener(interceptorOptions.ActivityIdentifierValue)) + using (var activityListener = new InterceptorActivityListener(testTags)) { var client = FoobarService.ConstructRpcClient(server.UriString); await clientRequestFunc(client, additionalMetadata); @@ -119,7 +120,7 @@ private static async Task TestHandlerSuccess(Func clientRequestFunc, Metadata additionalMetadata = null) { // starts the server with the server interceptor - var interceptorOptions = new ServerTracingInterceptorOptions { Propagator = new TraceContextPropagator(), ActivityIdentifierValue = Guid.NewGuid(), RecordException = true }; + var testTags = new TestActivityTags(); + var interceptorOptions = new ServerTracingInterceptorOptions { Propagator = new TraceContextPropagator(), AdditionalTags = testTags.Tags, RecordException = true }; using var server = FoobarService.Start(new ServerTracingInterceptor(interceptorOptions)); - using var activityListener = new InterceptorActivityListener(interceptorOptions.ActivityIdentifierValue); + using var activityListener = new InterceptorActivityListener(testTags); var client = FoobarService.ConstructRpcClient( server.UriString, additionalMetadata: new List diff --git a/test/OpenTelemetry.Instrumentation.GrpcCore.Tests/InterceptorActivityListener.cs b/test/OpenTelemetry.Instrumentation.GrpcCore.Tests/InterceptorActivityListener.cs index 0c43a7e003..e736367c75 100644 --- a/test/OpenTelemetry.Instrumentation.GrpcCore.Tests/InterceptorActivityListener.cs +++ b/test/OpenTelemetry.Instrumentation.GrpcCore.Tests/InterceptorActivityListener.cs @@ -3,7 +3,6 @@ using System; using System.Diagnostics; -using System.Linq; namespace OpenTelemetry.Instrumentation.GrpcCore.Tests; @@ -20,15 +19,15 @@ internal sealed class InterceptorActivityListener : IDisposable /// /// Initializes a new instance of the class. /// - /// The activity identifier. - public InterceptorActivityListener(Guid activityIdentifier) + /// The test activity tags. + public InterceptorActivityListener(TestActivityTags testTags) { this.activityListener = new ActivityListener { ShouldListenTo = source => source.Name == GrpcCoreInstrumentation.ActivitySourceName, ActivityStarted = activity => { - if (activity.TagObjects.Any(t => t.Key == SemanticConventions.AttributeActivityIdentifier && (Guid)t.Value == activityIdentifier)) + if (testTags.HasTestTags(activity)) { this.Activity = activity; } diff --git a/test/OpenTelemetry.Instrumentation.GrpcCore.Tests/TestActivityTags.cs b/test/OpenTelemetry.Instrumentation.GrpcCore.Tests/TestActivityTags.cs new file mode 100644 index 0000000000..6994c4bdd8 --- /dev/null +++ b/test/OpenTelemetry.Instrumentation.GrpcCore.Tests/TestActivityTags.cs @@ -0,0 +1,39 @@ +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +using System; +using System.Collections.Generic; +using System.Diagnostics; +using System.Linq; +using OpenTelemetry.Internal; + +namespace OpenTelemetry.Instrumentation.GrpcCore.Tests; + +internal class TestActivityTags +{ + public const string ActivityIdentifierTag = "activityidentifier"; + + public TestActivityTags() + { + this.Tags = new Dictionary() + { + [ActivityIdentifierTag] = Guid.NewGuid(), + }; + } + + internal IReadOnlyDictionary Tags { get; } + + /// + /// Checks whether the activity has test tags. + /// + /// The activity. + /// Returns true if the activty has test tags, false otherwise. + internal bool HasTestTags(Activity activity) + { + Guard.ThrowIfNull(activity); + + return this.Tags + .Select(tag => activity.TagObjects.Any(t => t.Key == tag.Key && t.Value == tag.Value)) + .All(v => v); + } +}