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 JSON input to zipkin plugin #3150

Merged
merged 12 commits into from
Aug 22, 2017
Merged

Add JSON input to zipkin plugin #3150

merged 12 commits into from
Aug 22, 2017

Conversation

goller
Copy link
Contributor

@goller goller commented Aug 21, 2017

Fix #3137

Required for all PRs:

  • Signed CLA.
  • Associated README.md updated.
  • Has appropriate unit tests.

This PR updates the zipkin plugin to accept JSON V1 zipkin span format: https://github.com/openzipkin/zipkin-api/blob/master/zipkin-api.yaml

Refactored the decoding and encoding of spans into Trace by creating a codec package.

Test coverage significantly increased.

This structure should allow supporting zipkin JSON v2 span format: openzipkin/zipkin#1499

return nil, fmt.Errorf("No value for key %s at binaryAnnotations[%d]", a.K, i)
}
if a.Value() != "" && a.Key() == "" {
return nil, fmt.Errorf("No at binaryAnnotations[%d]", i)
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure what this error means, also is binaryAnnotations going to be clear to user?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in b64e90c

// ContentType returns a Decoder that is able to produce Traces from bytes.
// Failure should yield an HTTP 415 (`http.StatusUnsupportedMediaType`)
// If a Content-Type is not set, zipkin assumes application/json
func ContentType(r *http.Request) (codec.Decoder, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would consider renaming this function, the current name does not indicate what the function does.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 63d07e2

@danielnelson danielnelson added this to the 1.4.0 milestone Aug 22, 2017
@danielnelson danielnelson merged commit 13a6b91 into master Aug 22, 2017
@danielnelson danielnelson deleted the feature/zipkin-json branch August 22, 2017 00:25
danielnelson pushed a commit that referenced this pull request Aug 22, 2017
maxunt pushed a commit that referenced this pull request Jun 26, 2018
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