gRPC: When exported to Zipkin, server address doesn't include port

[JamesNK]

Using OT 0.4.0-beta.2

There is inconsistency between the gRPC call telemetry and HttpClient telemetry in Zipkin. I don't know where the inconsistency starts, but I observed it there.

Out of localhost and localhost:5001, I'm guessing localhost:5001 is the correct value, and the gRPC telemetry is reporting an incorrect value.

grpc.net.client.grpcout telemetry:

system.net.httphttprequestout telemetry:

[cijothomas]

@alanwest Can you help take a look?

[alanwest]

I've found out what's going on.

The Zipkin exporter sets Server Address by considering a prioritized list of keys on the exported span.

• The HTTP instrumentation sets the http.host attribute from the request URI and includes the port when doing so.
• The gRPC instrumentation sets the net.peer.name attribute (i.e., SemanticConventions.AttributeNetPeerName) which does not include the port. It separately adds the net.peer.port attribute.

I do not yet have a recommendation for what to do. I'm confident we're doing the right thing in the gRPC instrumentation per the spec, but I will review the HTTP spec tomorrow to make sure we're doing the right thing there. I'll also consider whether we're doing the correct thing in the Zipkin exporter by comparing what other languages are doing.

[alanwest]

@CodeBlanch I was looking at #483 - This PR seems to be where the logic affecting what shows up as Server Address in the Zipkin UI was introduced.

Question: In the case of gRPC instrumentation that sets net.peer.name and net.peer.port do you think it makes sense to combine these two when present to set the remote_endpoint on the Zipkin span?

This would make the Server Address value equal between the gRPC and HTTP span. I guess this also raises the question of whether net.peer.name:net.peer.port from the gRPC instrumentation will always equal http.host from the HTTP instrumentation, but I think this may be true.

It's a little unclear to me what information remote_endpoint should convey. Should it even include the port?

[CodeBlanch]

You know how Zipkin & Jager show each service in a trace with a unique color? When I first started using OpenTelemetry, spans going to un-instrumented services (think 3rd party) all showed with the same color as the hosting service making the call in the trace. Turns out you need to pass something as RemoteEndpoint on your spans otherwise Zipkin & Jaeger think they are internal. For instrumented services that receive the call, and export the continuation of the trace to Zipkin/Jaeger, the UI displays the service name reported on those spans and ignores the RemoteEndpoint (where "AGGREGATOR" is displayed in the above) from the upstream data. It really only applies for things that aren't reporting. There's the example project in the solution that makes calls to www.google.com, if you run that it should demo what I'm talking about. So, long story short, you can add the port, it should be fine. Whether or not the port should be there, as far as Zipkin or Jaeger spec is concerned, I really have no idea. Probably useful to the user to have the port in there if it isn't 80/443.

Hopefully enough of that made sense!

PS: Please also update Jaeger if you make a change it has nearly identical logic for the same reasons.

[alanwest]

This is great, Thank you for providing all this context! I'll take a look at both Zipkin and Jaeger.