-
Notifications
You must be signed in to change notification settings - Fork 206
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 support for header transferring #136
Conversation
Lines Of Code
|
Go API Changes# github.com/adjust/rmq/v5 ## compatible changes (*TestBatchConsumer).Consumed: added (*TestBatchConsumer).Last: added (*TestConsumer).Deliveries: added (*TestConsumer).Last: added ExtractHeaderAndPayload: added PayloadBytesWithHeader: added PayloadWithHeader: added WithHeader: added # summary Inferred base version: v5.0.2 Suggested version: v5.1.0 |
Unit Test Coveragetotal: (statements) 71.6% Coverage of changed lines
Coverage diff with base branch
|
Benchmark ResultBenchmark diff with base branch
Benchmark result
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a quite dangerous change as it changes how deliveries are stored in Redis. I think we should explicitly point this out in the PR description, in the Readme and in the release notes of this PR.
It's not a breaking change as it doesn't affect you if you only upgrade but don't start using the headers. But for clients who want to start using the headers I think we need to point out that the rollout needs to be properly planned.
The simplest would probably just be to suggest to update all consumers to run with the new version and be ready to consume deliveries with headers (while still also handling deliveries without headers), before then updating the producers to start producing deliveries with headers.
The approach and the implementation seem very good. 👍 |
@wellle I've added notes on compatibility to README, please check if they are clear enough (if you have better wording, please share). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, seems good, thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Redis protocol does not define a specific way to pass additional data like header.
However, there is often need to pass them (for example for traces propagation).
This implementation injects optional header values marked with a signature into
payload body during publishing. When message is consumed, if signature is present,
header and original payload are extracted from augmented payload.
Header is defined as
http.Header
for better interoperability with existing libraries,for example with
propagation.HeaderCarrier
.This change is intended to be backwards compatible.
Header parsing is only done if payload starts with
"\xFF\x00\xBE\xBEJ"
, such signature is added along with header values explicitly by caller during publishing (usingPayloadWithHeader
helper).