[Instrumentation.GrpcNetClient] Support only stable HTTP semantic con…

…vention (#5259) Co-authored-by: Reiley Yang <[email protected]> Co-authored-by: Cijo Thomas <[email protected]> Co-authored-by: Alan West <[email protected]>
open-telemetry · Jan 27, 2024 · 0b9cf42 · 0b9cf42
1 parent 512c7dc
commit 0b9cf42
Show file tree

Hide file tree

Showing 7 changed files with 16 additions and 261 deletions.
diff --git a/src/OpenTelemetry.Instrumentation.GrpcNetClient/CHANGELOG.md b/src/OpenTelemetry.Instrumentation.GrpcNetClient/CHANGELOG.md
@@ -9,6 +9,12 @@
   [issue](https://github.com/open-telemetry/opentelemetry-dotnet/issues/5092)
   for details and workaround.
   ([#5077](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5077))
+* Removed support for the `OTEL_SEMCONV_STABILITY_OPT_IN` environment variable
+  which toggled the use of the new conventions for the
+  [server, client, and shared network attributes](https://github.com/open-telemetry/semantic-conventions/blob/v1.23.0/docs/general/attributes.md#server-client-and-shared-network-attributes).
+  Now that this suite of attributes are stable, this instrumentation will only
+  emit the new attributes.
+  ([#5259](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5259))
 
 ## 1.6.0-beta.3
 

diff --git a/src/OpenTelemetry.Instrumentation.GrpcNetClient/GrpcClientInstrumentationOptions.cs b/src/OpenTelemetry.Instrumentation.GrpcNetClient/GrpcClientInstrumentationOptions.cs
@@ -2,8 +2,6 @@
 // SPDX-License-Identifier: Apache-2.0
 
 using System.Diagnostics;
-using Microsoft.Extensions.Configuration;
-using static OpenTelemetry.Internal.HttpSemanticConventionHelper;
 
 namespace OpenTelemetry.Instrumentation.GrpcNetClient;
 
@@ -12,23 +10,6 @@ namespace OpenTelemetry.Instrumentation.GrpcNetClient;
 /// </summary>
 public class GrpcClientInstrumentationOptions
 {
-    internal readonly HttpSemanticConvention HttpSemanticConvention;
-
-    /// <summary>
-    /// Initializes a new instance of the <see cref="GrpcClientInstrumentationOptions"/> class.
-    /// </summary>
-    public GrpcClientInstrumentationOptions()
-        : this(new ConfigurationBuilder().AddEnvironmentVariables().Build())
-    {
-    }
-
-    internal GrpcClientInstrumentationOptions(IConfiguration configuration)
-    {
-        Debug.Assert(configuration != null, "configuration was null");
-
-        this.HttpSemanticConvention = GetSemanticConventionOptIn(configuration);
-    }
-
     /// <summary>
     /// Gets or sets a value indicating whether down stream instrumentation is suppressed (disabled).
     /// </summary>

diff --git a/...penTelemetry.Instrumentation.GrpcNetClient/Implementation/GrpcClientDiagnosticListener.cs b/...penTelemetry.Instrumentation.GrpcNetClient/Implementation/GrpcClientDiagnosticListener.cs
@@ -9,7 +9,6 @@
 using OpenTelemetry.Context.Propagation;
 using OpenTelemetry.Instrumentation.Http;
 using OpenTelemetry.Trace;
-using static OpenTelemetry.Internal.HttpSemanticConventionHelper;
 
 namespace OpenTelemetry.Instrumentation.GrpcNetClient.Implementation;
 
@@ -27,17 +26,11 @@ internal sealed class GrpcClientDiagnosticListener : ListenerHandler
     private static readonly PropertyFetcher<HttpResponseMessage> StopResponseFetcher = new("Response");
 
     private readonly GrpcClientInstrumentationOptions options;
-    private readonly bool emitOldAttributes;
-    private readonly bool emitNewAttributes;
 
     public GrpcClientDiagnosticListener(GrpcClientInstrumentationOptions options)
         : base("Grpc.Net.Client")
     {
         this.options = options;
-
-        this.emitOldAttributes = this.options.HttpSemanticConvention.HasFlag(HttpSemanticConvention.Old);
-
-        this.emitNewAttributes = this.options.HttpSemanticConvention.HasFlag(HttpSemanticConvention.New);
     }
 
     public override void OnEventWritten(string name, object payload)
@@ -133,35 +126,18 @@ public void OnStartActivity(Activity activity, object payload)
             }
 
             var uriHostNameType = Uri.CheckHostName(request.RequestUri.Host);
-            if (this.emitOldAttributes)
-            {
-                if (uriHostNameType == UriHostNameType.IPv4 || uriHostNameType == UriHostNameType.IPv6)
-                {
-                    activity.SetTag(SemanticConventions.AttributeNetPeerIp, request.RequestUri.Host);
-                }
-                else
-                {
-                    activity.SetTag(SemanticConventions.AttributeNetPeerName, request.RequestUri.Host);
-                }
 
-                activity.SetTag(SemanticConventions.AttributeNetPeerPort, request.RequestUri.Port);
+            if (uriHostNameType == UriHostNameType.IPv4 || uriHostNameType == UriHostNameType.IPv6)
+            {
+                activity.SetTag(SemanticConventions.AttributeServerSocketAddress, request.RequestUri.Host);
             }
-
-            // see the spec https://github.com/open-telemetry/semantic-conventions/blob/v1.21.0/docs/http/http-spans.md
-            if (this.emitNewAttributes)
+            else
             {
-                if (uriHostNameType == UriHostNameType.IPv4 || uriHostNameType == UriHostNameType.IPv6)
-                {
-                    activity.SetTag(SemanticConventions.AttributeServerSocketAddress, request.RequestUri.Host);
-                }
-                else
-                {
-                    activity.SetTag(SemanticConventions.AttributeServerAddress, request.RequestUri.Host);
-                }
-
-                activity.SetTag(SemanticConventions.AttributeServerPort, request.RequestUri.Port);
+                activity.SetTag(SemanticConventions.AttributeServerAddress, request.RequestUri.Host);
             }
 
+            activity.SetTag(SemanticConventions.AttributeServerPort, request.RequestUri.Port);
+
             try
             {
                 this.options.EnrichWithHttpRequestMessage?.Invoke(activity, request);

diff --git a/...elemetry.Instrumentation.GrpcNetClient/OpenTelemetry.Instrumentation.GrpcNetClient.csproj b/...elemetry.Instrumentation.GrpcNetClient/OpenTelemetry.Instrumentation.GrpcNetClient.csproj
@@ -13,8 +13,6 @@
   <ItemGroup>
     <Compile Include="$(RepoRoot)\src\OpenTelemetry.Instrumentation.Http\HttpRequestMessageContextPropagation.cs" Link="Includes\HttpRequestMessageContextPropagation.cs" />
     <Compile Include="$(RepoRoot)\src\Shared\Guard.cs" Link="Includes\Guard.cs" />
-    <Compile Include="$(RepoRoot)\src\Shared\EnvironmentVariables\*.cs" Link="Includes\EnvironmentVariables\%(Filename).cs" />
-    <Compile Include="$(RepoRoot)\src\Shared\HttpSemanticConventionHelper.cs" Link="Includes\HttpSemanticConventionHelper.cs" />
     <Compile Include="$(RepoRoot)\src\Shared\Options\*.cs" Link="Includes\Options\%(Filename).cs" />
     <Compile Include="$(RepoRoot)\src\Shared\Shims\NullableAttributes.cs" Link="Includes\Shims\NullableAttributes.cs" />
   </ItemGroup>

diff --git a/src/OpenTelemetry.Instrumentation.GrpcNetClient/TracerProviderBuilderExtensions.cs b/src/OpenTelemetry.Instrumentation.GrpcNetClient/TracerProviderBuilderExtensions.cs
@@ -50,15 +50,10 @@ public static TracerProviderBuilder AddGrpcClientInstrumentation(
 
         name ??= Options.DefaultName;
 
-        builder.ConfigureServices(services =>
+        if (configure != null)
         {
-            if (configure != null)
-            {
-                services.Configure(name, configure);
-            }
-
-            services.RegisterOptionsFactory(configuration => new GrpcClientInstrumentationOptions(configuration));
-        });
+            builder.ConfigureServices(services => services.Configure(name, configure));
+        }
 
         builder.AddSource(GrpcClientDiagnosticListener.ActivitySourceName);
         builder.AddLegacySource("Grpc.Net.Client.GrpcOut");

diff --git a/test/OpenTelemetry.Instrumentation.Grpc.Tests/GrpcTests.client.cs b/test/OpenTelemetry.Instrumentation.Grpc.Tests/GrpcTests.client.cs
@@ -8,7 +8,6 @@
 using Grpc.Core;
 #endif
 using Grpc.Net.Client;
-using Microsoft.Extensions.Configuration;
 using Microsoft.Extensions.DependencyInjection;
 #if !NETFRAMEWORK
 using OpenTelemetry.Context.Propagation;
@@ -91,213 +90,16 @@ public void GrpcClientCallsAreCollectedSuccessfully(string baseAddress, bool sho
 
         if (uriHostNameType == UriHostNameType.IPv4 || uriHostNameType == UriHostNameType.IPv6)
         {
-            Assert.Equal(uri.Host, activity.GetTagValue(SemanticConventions.AttributeNetPeerIp));
-            Assert.Null(activity.GetTagValue(SemanticConventions.AttributeNetPeerName));
-        }
-        else
-        {
-            Assert.Null(activity.GetTagValue(SemanticConventions.AttributeNetPeerIp));
-            Assert.Equal(uri.Host, activity.GetTagValue(SemanticConventions.AttributeNetPeerName));
-        }
-
-        Assert.Equal(uri.Port, activity.GetTagValue(SemanticConventions.AttributeNetPeerPort));
-        Assert.Equal(Status.Unset, activity.GetStatus());
-
-        // Tags added by the library then removed from the instrumentation
-        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);
-        }
-    }
-
-    [Theory]
-    [InlineData("http://localhost")]
-    [InlineData("http://localhost", false)]
-    [InlineData("http://127.0.0.1")]
-    [InlineData("http://127.0.0.1", false)]
-    [InlineData("http://[::1]")]
-    [InlineData("http://[::1]", false)]
-    public void GrpcClientCallsAreCollectedSuccessfully_New(string baseAddress, bool shouldEnrich = true)
-    {
-        var config = new KeyValuePair<string, string>[] { new("OTEL_SEMCONV_STABILITY_OPT_IN", "http") };
-        var configuration = new ConfigurationBuilder()
-            .AddInMemoryCollection(config)
-            .Build();
-
-        bool enrichWithHttpRequestMessageCalled = false;
-        bool enrichWithHttpResponseMessageCalled = false;
-
-        var uri = new Uri($"{baseAddress}:1234");
-        var uriHostNameType = Uri.CheckHostName(uri.Host);
-
-        using var httpClient = ClientTestHelpers.CreateTestClient(async request =>
-        {
-            var streamContent = await ClientTestHelpers.CreateResponseContent(new HelloReply());
-            var response = ResponseUtils.CreateResponse(HttpStatusCode.OK, streamContent, grpcStatusCode: global::Grpc.Core.StatusCode.OK);
-            response.TrailingHeaders().Add("grpc-message", "value");
-            return response;
-        });
-
-        var exportedItems = new List<Activity>();
-
-        using var parent = new Activity("parent")
-            .SetIdFormat(ActivityIdFormat.W3C)
-            .Start();
-
-        using (Sdk.CreateTracerProviderBuilder()
-                .SetSampler(new AlwaysOnSampler())
-                .ConfigureServices(services => services.AddSingleton<IConfiguration>(configuration))
-                .AddGrpcClientInstrumentation(options =>
-                {
-                    if (shouldEnrich)
-                    {
-                        options.EnrichWithHttpRequestMessage = (activity, httpRequestMessage) => { enrichWithHttpRequestMessageCalled = true; };
-                        options.EnrichWithHttpResponseMessage = (activity, httpResponseMessage) => { enrichWithHttpResponseMessageCalled = true; };
-                    }
-                })
-                .AddInMemoryExporter(exportedItems)
-                .Build())
-        {
-            var channel = GrpcChannel.ForAddress(uri, new GrpcChannelOptions
-            {
-                HttpClient = httpClient,
-            });
-            var client = new Greeter.GreeterClient(channel);
-            var rs = client.SayHello(new HelloRequest());
-        }
-
-        Assert.Single(exportedItems);
-        var activity = exportedItems[0];
-
-        ValidateGrpcActivity(activity);
-        Assert.Equal(parent.TraceId, activity.Context.TraceId);
-        Assert.Equal(parent.SpanId, activity.ParentSpanId);
-        Assert.NotEqual(parent.SpanId, activity.Context.SpanId);
-        Assert.NotEqual(default, activity.Context.SpanId);
-
-        Assert.Equal($"greet.Greeter/SayHello", activity.DisplayName);
-        Assert.Equal("grpc", activity.GetTagValue(SemanticConventions.AttributeRpcSystem));
-        Assert.Equal("greet.Greeter", activity.GetTagValue(SemanticConventions.AttributeRpcService));
-        Assert.Equal("SayHello", activity.GetTagValue(SemanticConventions.AttributeRpcMethod));
-
-        if (uriHostNameType == UriHostNameType.IPv4 || uriHostNameType == UriHostNameType.IPv6)
-        {
-            Assert.Equal(uri.Host, activity.GetTagValue(SemanticConventions.AttributeServerSocketAddress));
-            Assert.Null(activity.GetTagValue(SemanticConventions.AttributeServerAddress));
-        }
-        else
-        {
-            Assert.Null(activity.GetTagValue(SemanticConventions.AttributeServerSocketAddress));
-            Assert.Equal(uri.Host, activity.GetTagValue(SemanticConventions.AttributeServerAddress));
-        }
-
-        Assert.Equal(uri.Port, activity.GetTagValue(SemanticConventions.AttributeServerPort));
-        Assert.Equal(Status.Unset, activity.GetStatus());
-
-        // Tags added by the library then removed from the instrumentation
-        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);
-        }
-    }
-
-    [Theory]
-    [InlineData("http://localhost")]
-    [InlineData("http://localhost", false)]
-    [InlineData("http://127.0.0.1")]
-    [InlineData("http://127.0.0.1", false)]
-    [InlineData("http://[::1]")]
-    [InlineData("http://[::1]", false)]
-    public void GrpcClientCallsAreCollectedSuccessfully_Dupe(string baseAddress, bool shouldEnrich = true)
-    {
-        var config = new KeyValuePair<string, string>[] { new("OTEL_SEMCONV_STABILITY_OPT_IN", "http/dup") };
-        var configuration = new ConfigurationBuilder()
-            .AddInMemoryCollection(config)
-            .Build();
-
-        bool enrichWithHttpRequestMessageCalled = false;
-        bool enrichWithHttpResponseMessageCalled = false;
-
-        var uri = new Uri($"{baseAddress}:1234");
-        var uriHostNameType = Uri.CheckHostName(uri.Host);
-
-        using var httpClient = ClientTestHelpers.CreateTestClient(async request =>
-        {
-            var streamContent = await ClientTestHelpers.CreateResponseContent(new HelloReply());
-            var response = ResponseUtils.CreateResponse(HttpStatusCode.OK, streamContent, grpcStatusCode: global::Grpc.Core.StatusCode.OK);
-            response.TrailingHeaders().Add("grpc-message", "value");
-            return response;
-        });
-
-        var exportedItems = new List<Activity>();
-
-        using var parent = new Activity("parent")
-            .SetIdFormat(ActivityIdFormat.W3C)
-            .Start();
-
-        using (Sdk.CreateTracerProviderBuilder()
-                .SetSampler(new AlwaysOnSampler())
-                .ConfigureServices(services => services.AddSingleton<IConfiguration>(configuration))
-                .AddGrpcClientInstrumentation(options =>
-                {
-                    if (shouldEnrich)
-                    {
-                        options.EnrichWithHttpRequestMessage = (activity, httpRequestMessage) => { enrichWithHttpRequestMessageCalled = true; };
-                        options.EnrichWithHttpResponseMessage = (activity, httpResponseMessage) => { enrichWithHttpResponseMessageCalled = true; };
-                    }
-                })
-                .AddInMemoryExporter(exportedItems)
-                .Build())
-        {
-            var channel = GrpcChannel.ForAddress(uri, new GrpcChannelOptions
-            {
-                HttpClient = httpClient,
-            });
-            var client = new Greeter.GreeterClient(channel);
-            var rs = client.SayHello(new HelloRequest());
-        }
-
-        Assert.Single(exportedItems);
-        var activity = exportedItems[0];
-
-        ValidateGrpcActivity(activity);
-        Assert.Equal(parent.TraceId, activity.Context.TraceId);
-        Assert.Equal(parent.SpanId, activity.ParentSpanId);
-        Assert.NotEqual(parent.SpanId, activity.Context.SpanId);
-        Assert.NotEqual(default, activity.Context.SpanId);
-
-        Assert.Equal($"greet.Greeter/SayHello", activity.DisplayName);
-        Assert.Equal("grpc", activity.GetTagValue(SemanticConventions.AttributeRpcSystem));
-        Assert.Equal("greet.Greeter", activity.GetTagValue(SemanticConventions.AttributeRpcService));
-        Assert.Equal("SayHello", activity.GetTagValue(SemanticConventions.AttributeRpcMethod));
-
-        if (uriHostNameType == UriHostNameType.IPv4 || uriHostNameType == UriHostNameType.IPv6)
-        {
-            Assert.Equal(uri.Host, activity.GetTagValue(SemanticConventions.AttributeNetPeerIp));
-            Assert.Null(activity.GetTagValue(SemanticConventions.AttributeNetPeerName));
             Assert.Equal(uri.Host, activity.GetTagValue(SemanticConventions.AttributeServerSocketAddress));
             Assert.Null(activity.GetTagValue(SemanticConventions.AttributeServerAddress));
         }
         else
         {
-            Assert.Null(activity.GetTagValue(SemanticConventions.AttributeNetPeerIp));
-            Assert.Equal(uri.Host, activity.GetTagValue(SemanticConventions.AttributeNetPeerName));
             Assert.Null(activity.GetTagValue(SemanticConventions.AttributeServerSocketAddress));
             Assert.Equal(uri.Host, activity.GetTagValue(SemanticConventions.AttributeServerAddress));
         }
 
         Assert.Equal(uri.Port, activity.GetTagValue(SemanticConventions.AttributeServerPort));
-        Assert.Equal(uri.Port, activity.GetTagValue(SemanticConventions.AttributeNetPeerPort));
         Assert.Equal(Status.Unset, activity.GetStatus());
 
         // Tags added by the library then removed from the instrumentation

diff --git a/test/OpenTelemetry.Instrumentation.Grpc.Tests/GrpcTests.server.cs b/test/OpenTelemetry.Instrumentation.Grpc.Tests/GrpcTests.server.cs
@@ -14,7 +14,6 @@
 using OpenTelemetry.Instrumentation.GrpcNetClient;
 using OpenTelemetry.Trace;
 using Xunit;
-using static OpenTelemetry.Internal.HttpSemanticConventionHelper;
 using Status = OpenTelemetry.Trace.Status;
 
 namespace OpenTelemetry.Instrumentation.Grpc.Tests;
@@ -43,7 +42,6 @@ public void GrpcAspNetCoreInstrumentationAddsCorrectAttributes(string enableGrpc
         var configuration = new ConfigurationBuilder()
             .AddInMemoryCollection(new Dictionary<string, string>
             {
-                [SemanticConventionOptInKeyName] = "http",
                 ["OTEL_DOTNET_EXPERIMENTAL_ASPNETCORE_ENABLE_GRPC_INSTRUMENTATION"] = enableGrpcAspNetCoreSupport,
             })
             .Build();
@@ -116,7 +114,6 @@ public void GrpcAspNetCoreInstrumentationAddsCorrectAttributesWhenItCreatesNewAc
             var configuration = new ConfigurationBuilder()
            .AddInMemoryCollection(new Dictionary<string, string>
            {
-               [SemanticConventionOptInKeyName] = "http",
                ["OTEL_DOTNET_EXPERIMENTAL_ASPNETCORE_ENABLE_GRPC_INSTRUMENTATION"] = enableGrpcAspNetCoreSupport,
            })
            .Build();