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

Adds reverse-mapped proto file for the json scheme in OpenApi/Swagger #77

Closed codefromthecrypt closed 5 years ago

codefromthecrypt commented 5 years ago

The reverse engineers the json format from our swagger into proto in attempts to unblock envoy from being able to generate json. It appears the only allowed tool is protobuf. Usually, we wouldn't host a file like this, but the otherwise is no-one being able to move off json v1 unless they go to protobuf, and we know that is not likely to occur.

See https://github.com/envoyproxy/envoy/pull/6985

codefromthecrypt commented 5 years ago

ps I don't know if this actually works with the proto-json conversion, into our format. someone will need to eyeball or otherwise that what appears to be correct and valid indeed is cc @dio

dio commented 5 years ago

@adriancole Thank you! 🙇

codefromthecrypt commented 5 years ago

we're in this together @dio!! I think this is a good solution. ping back if there are any glitches in the file

dio commented 5 years ago

Looks good!

Please take a look at the demo example here: https://github.com/envoy-zipkin/example. This example contains set up, so we can have HTTP_JSON_V1, HTTP_JSON_V2 and HTTP_PROTO collector endpoint version. The results for these three versions are the same (as far as I can observe).

Sorry for taking so long. Thank you!

dio commented 5 years ago

Hum, seems like I'm wrong. Some of the server annotations are missing for v2.

codefromthecrypt commented 5 years ago

If you want, use a gist or something to show the compared things.. proto vs this vs v1. maybe I can help identify easier as eyes are trained.

On Sun, Aug 18, 2019 at 12:11 AM Dhi Aurrahman notifications@github.com wrote:

Hum, seems like I'm wrong. Some of the server annotations are missing.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

dio commented 5 years ago

@adriancole here you are https://gist.github.com/dio/528ab1e0b3de40ef85bb074fe560efd7.

codefromthecrypt commented 5 years ago

@dio thanks loading them into lens now. cheers

codefromthecrypt commented 5 years ago

@dio so your three examples look identical except the variables like IDs. What is the part that you are concerned with about them? I can see some data oddities, but not sure they are new. ex "downstream_cluster": "-", is quite a strange choice to serialize.

codefromthecrypt commented 5 years ago

also the client order finish being before the server finish isn't terribly unexpected. it is reasonable for this to occur depending on where the timers are at.

codefromthecrypt commented 5 years ago

@dio as far as I can tell from your change, all is good except possibly the annotations array hasn't been tested yet. So, the concern is we merge this and it doesn't serialize properly into json. Is that right?

dio commented 5 years ago

Yeah, I think it is good. Apparently, I need to wrap ListOfSpans.spans to be loaded in [<span>, <span>, ...]. But that's OK since we don't depend on rapidjson to do that.

I think this is good to be merged.

codefromthecrypt commented 5 years ago

@dio does that imply we don't need the type ListOfSpans here? if so maybe we chop it

dio commented 5 years ago

@adriancole it is not required, but I take advantage of it as a temporary container here: https://github.com/envoyproxy/envoy/pull/6985/files#diff-b3abe4bcec8952e21f1fdb0115878167R91, so the code looks "similar" with the proto3. If you remove it I can replace that using vector. I have no strong opinion here.

codefromthecrypt commented 5 years ago

hi, @dio because the intent is to match the json form, and I think with the ListOfSpans container doesn't match the json form.. seems least confusing to remove it. I don't think ListOfSpans is buying us enough over vector to keep it in. I wouldn't want to have to explain to someone passing by who tried to use this and found it didn't match.

make sense?

dio commented 5 years ago

@adriancole sure. I'll update the code.

codefromthecrypt commented 5 years ago

ok I bumped the commit here. @dio when you say go, I'll squash and cut a release tag.

dio commented 5 years ago

@adriancole, looks good! Thanks everyone!