Add gRPC attributes for ASP.NET Core

[alanwest]

#482 Adds gRPC attributes to activity generated by ASP.NET Core.

[alanwest]

While this seems to be what the spec suggest, do we really want to trim the leading slash? It is what I did with the gRPC client instrumentation. Though, doing this here does effectively rename the activity as generated by ASP.NET Core.

I would either like to:

• Delete this TODO altogether and change the client-side instrumentation to maintain the the leading /, or
• Uncomment this and trim the / to match the client-side instrumentation

Link to spec

[MikeGoldsmith]

Left a few comments, I think this looks good.
We can always iterate on instrumentation to improve them.

[MikeGoldsmith]

It looks like different tags are added depending on whether the GrpcMethod can be retrieved, is that correct?

For example:
DisplayName (currently commented out, AttributeRpcSystem, service, method, etc
vs
AttributeHttpStatusCode, Status

Should we drop the else so the later tags are always added?

[alanwest]

My decision here was intentional, though I did raise the question earlier whether all the http.* attributes should be omitted from the span in the context of a gRPC invocation.

Two things occur in the else clause:

1. The http.status_code attribute is set
2. The Status of the span gets set

At a minimum we should not set the span's status based on the HTTP status because the gRPC status should be used instead. Though, I could see an argument for leaving the http.status_code attribute. Other http.* attributes are added in OnStartActivity. Given that this PR currently leaves those untouched, then it probably makes sense to leave http.status_code.

I'm ok coming back to this question of keeping vs. omitting the http.* attributes and iterating on this instrumentation later, though I think I need to make sure the test actually reflects these decisions.

[alanwest]

I expanded the test to reflect this conversation.

opentelemetry-dotnet/test/OpenTelemetry.Instrumentation.Grpc.Tests/GrpcTests.server.cs

Lines 63 to 72 in 8232d33

	// The following are http.* attributes that are also included on the span for the gRPC invocation.
	Assert.Equal($"localhost:{this.fixture.Port}", span.Tags.FirstOrDefault(i => i.Key == SemanticConventions.AttributeHttpHost).Value);
	Assert.Equal("POST", span.Tags.FirstOrDefault(i => i.Key == SemanticConventions.AttributeHttpMethod).Value);
	Assert.Equal("/greet.Greeter/SayHello", span.Tags.FirstOrDefault(i => i.Key == SpanAttributeConstants.HttpPathKey).Value);
	Assert.Equal($"http://localhost:{this.fixture.Port}/greet.Greeter/SayHello", span.Tags.FirstOrDefault(i => i.Key == SemanticConventions.AttributeHttpUrl).Value);
	Assert.StartsWith("grpc-dotnet", span.Tags.FirstOrDefault(i => i.Key == SemanticConventions.AttributeHttpUserAgent).Value);
	// This attribute is added by the gRPC for .NET library. There is a discussion of having the OTel instrumentation remove it.
	// See: https://github.com/open-telemetry/opentelemetry-dotnet/issues/482#issuecomment-655753756
	Assert.Equal($"0", span.Tags.FirstOrDefault(i => i.Key == "grpc.status_code").Value);

[MikeGoldsmith]

I think we discussed a long time ago, but I personally like having both the http and grpc tags in there. GRPC is not limited to working over HTTP, even though not many alternatives exist. I may be considered unnecessary details for now though 🤷
@cijothomas @CodeBlanch What are your thoughts?

[alanwest]

personally like having both the http and grpc tags in there

I agree that it probably makes sense to leave them in. I'm not necessarily advocating for one way or another at this point, but just to highlight some things for ongoing consideration:

• For the client side grpc-dotnet instrumentation we discussed the possibility of allowing the suppression of the underlying HTTP span that is generated. There exist other examples where this is desired as well.
• For the ASP.NET Core server side instrumentation, things are a bit different, there is no "other" span. Attributes are applied to the span (activity) that ASP.NET Core started. The library itself adds attributes here.

GRPC is not limited to working over HTTP

Yes, true, and I definitely see value in reflecting transport type on calls both server-side and client-side. This may be a thing that needs to be continued to be pushed at the spec level. I personally have experience with instrumenting WCF - which is a complex beast, and I've confronted a lot of these same considerations with it.

Slight tangent, but with WCF and perhaps eventually with gRPC, transport type used affects whether we can do auto-context propagation. For example, if the transport is a raw TCP channel, then there exist no structured headers mechanism to inject context - it has to be done manually.

[codecov]

Codecov Report

Merging #803 into master will increase coverage by 0.18%.
The diff coverage is 87.50%.

@@            Coverage Diff             @@
##           master     #803      +/-   ##
==========================================
+ Coverage   68.35%   68.54%   +0.18%     
==========================================
  Files         220      220              
  Lines        5989     6002      +13     
  Branches      981      983       +2     
==========================================
+ Hits         4094     4114      +20     
+ Misses       1621     1615       -6     
+ Partials      274      273       -1     

Impacted Files	Coverage Δ
...penTelemetry.Instrumentation.Grpc/GrpcTagHelper.cs	100.00% <ø> (ø)
...tation.AspNetCore/Implementation/HttpInListener.cs	79.12% <86.66%> (+2.80%)	⬆️
...rpc/Implementation/GrpcClientDiagnosticListener.cs	79.31% <100.00%> (ø)
src/OpenTelemetry.Api/Trace/TracerProvider.cs	85.71% <0.00%> (-14.29%)	⬇️
src/OpenTelemetry.Api/Trace/TelemetrySpan.cs	54.16% <0.00%> (-2.70%)	⬇️
src/OpenTelemetry/Trace/TracerProviderSdk.cs	100.00% <0.00%> (ø)
.../Context/NoopDistributedContextBinarySerializer.cs	0.00% <0.00%> (ø)
...umentation.AspNet/Implementation/HttpInListener.cs	0.00% <0.00%> (ø)
...elemetry/Context/Propagation/SerializationUtils.cs	100.00% <0.00%> (+1.51%)	⬆️
... and 4 more

[MikeGoldsmith]

Looks good to me, thanks for working through the feedback 👍

[cijothomas]

@alanwest Can you resolve the conflict?