Opentelemetry-dotnet: How to get http.host tags on server zipkin spans?

[christopher-taormina-zocdoc]

Question

I'm using the 0.4.0-beta.132 package setup up with the aspnetcore instrumentation and a Zipkin exporter. It seems the http.host tage the HttpInListenter adds to the server span is not added to the ZipkinSpan object when the exporter is converting the Activity to an instance of Zipkin span, but instead is pulled out to attempt to be used for the RemoteServiceEndpoint field for Client and Producer spans. How can I have this tag stay on the server span?

Describe any aspect of your environment relevant to the question.

What are you trying to achieve?
Tag a zipkin span with http.host

What did you expect to see?
The span to have the http.host tag

Additional Context

Add any other context about the problem here. If you followed an existing
documentation, please share the link to it.

[cijothomas]

@eddynaka Can you help/take a look?

[eddynaka]

@christopher-taormina-zocdoc do u have a demo where I can test?

[christopher-taormina-zocdoc]

@eddynaka sure how much do you need? all I have is a server instrumented with the following code

collection.AddOpenTelemetry((otelBuilder) =>
                        {
                            otelBuilder
                                .AddSource("ServiceName")
                                .SetResource(Resources.CreateServiceResource("ServiceName"))
                                .SetSampler(new ParentOrElseSampler(new ProbabilitySampler(1)))
                                .AddAspNetCoreInstrumentation((options) =>
                                {
                                    options.TextFormat = new B3Format();
                                })
                                .AddZipkinExporter((o) =>
                                {
                                    o.ServiceName = "ServiceName";
                                    o.Endpoint = "otelcollector:9411/api/v2/spans";
                                });
                        });

and debug through ToZipkinSpan method in ZipkinActivityConversionExtensions and later see the resulting json body for the http call not having the tag http.host.

[eddynaka]

@christopher-taormina-zocdoc , i was looking at this. Running the Examples.AspNetCore. I can see some data:

The http.host is removed on purpose from the activity.Tags. The same happen for other fields: peer.service, peer.hostname, peer.address, http.host, db.instance. Not sure if that is a requisite from the spec.

[christopher-taormina-zocdoc]

I it was removed as an attempt to set RemoteServiceEndpoint as referenced in the spec to convert from otel to zipkin here, but it doesn't seem to need to also remove the tag? Is it possible to allow the tag to still be written to the Zipkin span in addition to this logic? As far as I can tell the java and python zipkin exporters don't remove the tags.

[eddynaka]

@cijothomas @pjanotti @CodeBlanch , since i'm not that involved in the ZipkinExporter from the beginning, would you think that maintaining the http.host in the tags would be bad? Today we are not adding / exporting it as tags.

[CodeBlanch]

@eddynaka Basically, it's a bug. It should be transported. The code linked in the description:
https://github.com/open-telemetry/opentelemetry-dotnet/blob/4915d74b1c4ddc213ec1a125cf940d686797f908/src/OpenTelemetry.Exporter.Zipkin/Implementation/ZipkinActivityConversionExtensions.cs#L185-L190
Should really be:

                if (RemoteEndpointServiceNameKeyResolutionDictionary.TryGetValue(key, out int priority)
                    && (state.RemoteEndpointServiceName == null || priority < state.RemoteEndpointServiceNamePriority))
                {
                    state.RemoteEndpointServiceName = strVal;
                    state.RemoteEndpointServiceNamePriority = priority;

                    PooledList<KeyValuePair<string, object>>.Add(ref state.Tags, new KeyValuePair<string, object>(key, strVal));
                }

The same could also be said for AttributeNetPeerIp & AttributeNetPeerPort tags. Basically we should send them all, but still do the selection for RemoteEndpointServiceName.

Not sure if your recent refactor actually fixed this? Could you check on it? Also, JaegerExporter probably has the same bug.

[eddynaka]

HI @CodeBlanch , my last refactor solves the Write stuff, but, if we don't have those in the tags, it won't write. I will check and push this change to Zipkin and check JaegerExporter as well.

[eddynaka]

If we could send everything, we could maintain all the logic and remove the else section and add those tags every time. What do you think?

[CodeBlanch]

The instrumentation is adding them here, so they should be available at the time of export.

If we could send everything, we could maintain all the logic and remove the else section and add those tags every time. What do you think?

Ya that sounds good to me. Always send all the tags. A handful of them while we are sending we want to grab for remote endpoint resolution.

/cc @alanwest

[eddynaka]

@CodeBlanch , for JaegerExporter, we don't have this issue:

        private static void ProcessJaegerTag(ref TagState state, string key, JaegerTag jaegerTag)
        {
            if (jaegerTag.VStr != null
                && PeerServiceKeyResolutionDictionary.TryGetValue(key, out int priority)
                && (state.PeerService == null || priority < state.PeerServicePriority))
            {
                state.PeerService = jaegerTag.VStr;
                state.PeerServicePriority = priority;
            }

            PooledList<JaegerTag>.Add(ref state.Tags, jaegerTag);
        }

[eddynaka]

@christopher-taormina-zocdoc , as soon as the PR get's merged, it will push all tags. Below an example:

[christopher-taormina-zocdoc]

@eddynaka awesome thanks again for the help. Sorry if I just manage to create work lol

[eddynaka]

@christopher-taormina-zocdoc , we are solving bugs! that's the best part!

[alanwest]

Ya that sounds good to me. Always send all the tags. A handful of them while we are sending we want to grab for remote endpoint resolution.

Yes, this makes sense. In fact I was just working on Jaeger this morning and realized that it does not have this issue. That is, it sends up all the tags. Oh wait... thank you, I think I may have introduced a similar issue in #1195 and I need to fix it. Nevermind, sorry for the false alarm 😄, I think we're good. Jaeger always adds all the tags here:

https://github.com/open-telemetry/opentelemetry-dotnet/blob/99dce1539cc5bb8d949d49084f22d9b7342d652a/src/OpenTelemetry.Exporter.Jaeger/Implementation/JaegerActivityExtensions.cs#L336

[eddynaka]

@christopher-taormina-zocdoc , the code is merged. We will have the new beta soon AND you can still use the myget which will be generated automatically at 00:xx UTC.

thanks again!

[christopher-taormina-zocdoc]

@eddynaka oh nice, whens the new beta? is there a schedule for that I can follow?

[eddynaka]

@christopher-taormina-zocdoc , it is scheduled for today but can extend. We don't have an exact time. We are creating PR's to add more docs to all the things we have done so far...etc.

[eddynaka]

@christopher-taormina-zocdoc , we just released the new beta!

[christopher-taormina-zocdoc]

awesome! thanks for the heads up