openzipkin / zipkin-api

Zipkin's language independent model and HTTP Api Definitions
https://zipkin.io/zipkin-api/
Apache License 2.0
59 stars 32 forks source link

int64 format not specified in some (at least one?) location #93

Closed philipbawn closed 8 months ago

philipbawn commented 3 years ago

https://github.com/openzipkin/zipkin-api/blob/master/zipkin2-api.yaml line 319 has type: integer, but format: int64 is not specified. due to this, my client generator (nswag) made the Annotation.timestamp property an int32 which of course blows up. Is this yml file the source of truth or is something generating it? If the yaml file can just be edited i can go through it and see if there is this problem anywhere else and make a PR.

Thanks.

devinsba commented 3 years ago

The YAML file is maintained by hand. A contribution would be very welcome, thanks

jcchavezs commented 3 years ago

+1

José Carlos Chávez

ons. 24. feb. 2021 kl. 00:44 skrev Brian Devins-Suresh < notifications@github.com>:

The YAML file is maintained by hand. A contribution would be very welcome, thanks

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/openzipkin/zipkin-api/issues/93#issuecomment-784611571, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAXOYATJGXZDASFR7NPENX3TAQ4WFANCNFSM4YDMG75A .

philipbawn commented 3 years ago

I'm looking through it now as well as the output from my client generator :)

philipbawn commented 3 years ago

This is solved by this PR https://github.com/openzipkin/zipkin-api/pull/94 which makes the swagger file work well with nswag. I at first thought write operations were already OK, but before encountering issues with the yaml I didn't try any of those. It turns out the timestamp annotation is used both for retrieval as well as posting spans, which makes sense.

philipbawn commented 3 years ago

You know what.. I don't see how writes would work if someone generated a client using this before the modification. Here is what the current master 7692ca7 looks like on my screen:

span

Maybe I'm agonizing over this too much but I do worry about breaking other people's stuff.

Here is my client after the fix from the PR. span2

That looks really good. I'm using NSwag v13.0.5.0

philipbawn commented 3 years ago

The opentelemetry-dotnet people appear to have wrote their own class (rather than generate one from the swagger file) which makes use of long data type, similar to my result after adding the format to this YAML file. This must be why I did not encounter any problems sending telemetry to zipkin but only querying from it when I raised the issue. I am using their package for sending telemetry.

codefromthecrypt commented 8 months ago

thanks for the PR @philipbawn, so sorry that it didn't resolve earlier. cc @reta also, as I noticed I wasn't watching this repo :blush: