Adds reverse-mapped proto file for the json scheme in OpenApi/Swagger #77
Conversation
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 envoyproxy/envoy#6985
codefromthecrypt
requested review from
devinsba,
anuraaga,
jcchavezs and
basvanbeek
|
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 |
|
@adriancole Thank you! 🙇 |
|
we're in this together @dio!! I think this is a good solution. ping back if there are any glitches in the file |
There was a problem hiding this comment.
LGTM... let's wait for @dio to come back and tell us if it serializes properly.
|
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 Sorry for taking so long. Thank you! |
|
Hum, seems like I'm wrong. Some of the server annotations are missing for v2. |
|
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 ***@***.***> 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 thanks loading them into lens now. cheers |
|
@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 |
|
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. |
|
@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? |
|
Yeah, I think it is good. Apparently, I need to wrap I think this is good to be merged. |
|
@dio does that imply we don't need the type ListOfSpans here? if so maybe we chop it |
|
@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. |
|
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? |
|
@adriancole sure. I'll update the code. |
|
ok I bumped the commit here. @dio when you say go, I'll squash and cut a release tag. |
|
@adriancole, looks good! Thanks everyone! |
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 envoyproxy/envoy#6985