Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add zipkin.proto #56

Merged
merged 3 commits into from
Aug 8, 2019
Merged

Conversation

jan25
Copy link
Contributor

@jan25 jan25 commented Aug 4, 2019

Signed-off-by: Abhilash Gnan abhilashgnan@gmail.com

Signed-off-by: Abhilash Gnan <abhilashgnan@gmail.com>
Signed-off-by: Abhilash Gnan <abhilashgnan@gmail.com>
Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We didn't add URL for swagger and thrift, should we add for them too?

yes, it would be useful to have a reference to the source

@@ -0,0 +1,233 @@
// https://github.com/openzipkin/zipkin-api/blob/master/zipkin.proto
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Copied from ... (followed by blank line)

Signed-off-by: Abhilash Gnan <abhilashgnan@gmail.com>
@jan25
Copy link
Contributor Author

jan25 commented Aug 7, 2019

Looks like zipkin swagger and thrift model files have been updated on upstream and we have pretty old ones. We can update them in a separate PR.

@yurishkuro yurishkuro merged commit e85e7d6 into jaegertracing:master Aug 8, 2019
@yurishkuro
Copy link
Member

Thanks. We should also add a build step as a validation of .proto files, similar to how we build from thrift.

@jan25 jan25 deleted the add-zipkin-proto branch August 8, 2019 06:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants