Content-Type for Zipkin Protobuf should be application/x-protobuf. #2101
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Motivation: Zipkin receiver expects Content-Type of application/x-protobuf. Why is this change being made? Correct Content-Type is needed for receivers to correctly identity the payload format. Modifications: - HttpReporter's PROTO_CONTENT_TYPE is updated. Result: When using Codec.PROTO3, the outbound request to the trace collector will have the correct Content-Type.
There was a problem hiding this comment.
Thank you, @saturnism, good catch!
Just for the history of this PR, consider adding references to where you found it out.
| @@ -69,7 +69,7 @@ | |||
| static final String V1_PATH = "/api/v1/spans"; | |||
| static final String V2_PATH = "/api/v2/spans"; | |||
| static final CharSequence THRIFT_CONTENT_TYPE = newAsciiString("application/x-thrift"); | |||
| static final CharSequence PROTO_CONTENT_TYPE = newAsciiString("application/protobuf"); | |||
| static final CharSequence PROTO_CONTENT_TYPE = newAsciiString("application/x-protobuf"); | |||
There was a problem hiding this comment.
Consider enhancing HttpReporterTest#verifyRequest (btw, while you are there, please make that method static) to also validate content-type.
Scottmitch
approved these changes
Feb 23, 2022
|
References to Zipkin and OpenTelemetry that require |
idelpivnitskiy
approved these changes
Feb 23, 2022
There was a problem hiding this comment.
@saturnism we are planning to release today, I'm going to pull this in and we can enhance tests in a follow-up. Thank you for the great catch!
idelpivnitskiy
pushed a commit
that referenced
this pull request
Feb 23, 2022
…2101) Motivation: Zipkin receiver expects Content-Type of `application/x-protobuf`. Why is this change being made? Correct Content-Type is needed for receivers to correctly identity the payload format. Modifications: - HttpReporter's `PROTO_CONTENT_TYPE` is updated. Result: When using `Codec.PROTO3`, the outbound request to the trace collector will have the correct Content-Type.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation:
Zipkin receiver expects Content-Type of application/x-protobuf.
Why is this change being made?
Correct Content-Type is needed for receivers to correctly identity the payload format.
Modifications:
Result:
When using Codec.PROTO3, the outbound request to the trace collector will have the correct Content-Type.