Opentelemetry-dotnet: ZipkinExporter cannot properly handle tags that are null

[christopher-taormina-zocdoc]

Describe your environment. Describe any aspect of your environment relevant to the problem:

• SDK version: 0.4.0-beta.111
• .NET runtime version (.NET or .NET Core, TargetFramework in the .csproj file): aspnetcore 2.1
• Platform and OS version: macos 10.14.5

It looks the ZipkinActivityConversionExtension method ProcessTags does not correctly handle tags that are not strings. The HttpInListener for the AspNetCore instrumentation is setting the tag http.status_code to an int here. In the ProcessTags method it looks like we get the string value for the KeyValuePair that is the activity tag through this conversion. However this does not convert the int value of http.status_code to a string, which results in the code setting the tag on the ZipkinSpan to be null in this line. This causes a NullReferenceException when trying to write the ZipkinSpan to JSON here.

Steps to reproduce.
Setup the AspNetCore Instrumentation to use the zipkin exporter and make a request to the server that would be sampled.

What is the expected behavior?
Zipkin Exporter should successfully make call to endpoint

What is the actual behavior?
A null reference exception is thrown when trying to convert the ZipkinSpan to json due to a null tag for http.status_code

Additional context.
Add any other context about the problem here.

[eddynaka]

@christopher-taormina-zocdoc , i will create a PR solving this issue. I will probably check if the object is string, if yes, we will maintain that logic, otherwise, it will just maintain as object. That will probably solve your issue.

[eddynaka]

@christopher-taormina-zocdoc , yesterday we merged the bugfix. Can you try the latest version in MyGet? The version would be 114. Let us know if that solves the issue, ok?
Thank you.

[christopher-taormina-zocdoc]

@eddynaka I pulled in 114 and I still get a null reference exception, as the http.status_code tag still seems to be set to null on the ZipkinSpan object

[eddynaka]

@christopher-taormina-zocdoc , after i saw, i don't know if the package 114 contains my change (we are doing nightly builds). Can you test in the new build that we will generate tonight? Looking at the tests that we made, we are covering that case.

[christopher-taormina-zocdoc]

@eddynaka about what time do those get released?

[eddynaka]

@christopher-taormina-zocdoc , it's released at 00:00 where GH Actions run. Not sure exactly what is the timezone.

Below, the tests that I'm covering: https://github.com/open-telemetry/opentelemetry-dotnet/blob/8bd8641e3e4177c25df622cfbc5b3661d2f04a0c/test/OpenTelemetry.Exporter.Zipkin.Tests/Implementation/ZipkinActivityConversionExtensionsTest.cs#L43-L57

[eddynaka]

@eddynaka about what time do those get released?

Ok, i know the time: Wed, 19 Aug 2020 00:15:08 GMT

[christopher-taormina-zocdoc]

Hey @eddynaka , so I think this surfaced one other issue. I'm actually working with sending traces to an opentelemetry collector with a zipkin reciever. In your fix the http.status_code tag remains an integer which would normally be fine, but the json request for the span will keep it as an integer. This causes the opentelemetry collector to respond with a 400 because the tag value is not a string. I think since you now explicitly check for null, you can toString the attribute.Value on this line now without worry.

[eddynaka]

@christopher-taormina-zocdoc , if that's true, we would have a bigger issue:
https://github.com/open-telemetry/opentelemetry-dotnet/blob/46907830e282ae3cc3b5890a27ce69ac4e8feeb7/src/OpenTelemetry.Exporter.Zipkin/Implementation/ZipkinSpan.cs#L283-L306

If you see this, we would convert almost every know type to their specific type.

[christopher-taormina-zocdoc]

Ok but this is exactly the behavior I just observed locally.

[eddynaka]

@christopher-taormina-zocdoc , let me test and see what is happening. Give me some time :)

[eddynaka]

@christopher-taormina-zocdoc , I tested locally with the latest version using our ConsoleExample.

Added a simple tag:

activity?.SetTag("status", 200);

And, i can see the data in Zipkin:

[christopher-taormina-zocdoc]

Is this using an opentelemetry collector? When I make a post to my opentelemetry collector's zipkin receiver then I receive a 400.

[
    {
        "traceId": "09d534bb0759e447b23fc947f8c0ee88",
        "name": "test/v1/values",
        "id": "5f0b9d147f4e6241",
        "kind": "SERVER",
        "timestamp": 1597935434406358,
        "duration": 11061,
        "localEndpoint": {
            "serviceName": "TestApiServer"
        },
        "annotations": [],
        "tags": {
            "http.method": "GET",
            "http.path": "/test/v1/values",
            "http.url": "http://localhost:9000/test/v1/values",
            "http.user_agent": "PostmanRuntime/7.26.3",
            "http.request_content_length": "92",
            "http.request_content_type": "application/json",
            "http.scheme": "http",
            "http.target": "/test/v1/values",
            "net.peer.port": "55206",
            "http.route": "test/v1/values",
            "span_name": "GET test/v1/values",
            "http.status_text": "OK",
            "http.response_content_type": "application/json; charset=utf-8",
            "http.status_code": 200,
            "otel.status_code": "Ok"
        }
    }
]

[eddynaka]

oh..i see, so u are using zipking exporter with Otel collector, is that it?

[christopher-taormina-zocdoc]

@eddynaka yeah, in our case we've dropped in the otel collector to start migrating away from zipkin, but primarily use the zipkin receiver to not change all of our existing tracing calls.

[eddynaka]

IMO, i think u should use OtelExporter instead of ZipkinExporter. We don't have a scenario that uses one exporter to send to a different collector. So, we are assuming (at least in my option), that when you use ZipkinExporter you will send data to Zipkin.

[eddynaka]

@cijothomas @CodeBlanch , have you seen this case? Do you have another suggestion?

[christopher-taormina-zocdoc]

@eddynaka the otelexporter uses grpc correct? Our current infrastructure wouldn't allow for that, as the collector is deployed behind an AWS ALB.

[christopher-taormina-zocdoc]

It also appears the the zipkin API contract states that tags should be strings. https://zipkin.io/zipkin-api/

[CodeBlanch]

I don't have any hands-on experience with it, but I don't see any reason why it shouldn't work. ZipkinExporter sends span data in Zipkin format. As long as the receiver understands that format, everything should work. So our job is to make sure we are sending valid Zipkin format. If we're not, we should fix it!

[christopher-taormina-zocdoc]

I can help out with this

[eddynaka]

IF i create a branch, can you test in ur scenario?

[eddynaka]

@christopher-taormina-zocdoc , the branch: https://github.com/open-telemetry/opentelemetry-dotnet/compare/master...eddynaka:bugfix/zipkin-format?expand=1, if u download and build you will be able to check if that works for u.

[christopher-taormina-zocdoc]

Sure let me pull that and work locally for a bit

[christopher-taormina-zocdoc]

@eddynaka It looks like that fix should work for me.

[eddynaka]

@eddynaka It looks like that fix should work for me.

Nice to know. I openened the PR!

[eddynaka]

@christopher-taormina-zocdoc , i'm not sure if we merged in time. can you check in the latest build or today's build if that works?

thank you.

[christopher-taormina-zocdoc]

@eddynaka looks like it made it in. Thanks for all the help!

[eddynaka]

Nice to know! Thank you for pointing everything! 👍