-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
feat: add metadata field to consumer message #2914
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Furkan Aksoy <[email protected]>
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.
I’m failing to understand why this change should be at all necessary. We can already pass metadata in the Headers
field, and the provided tests don’t indicate how this new Metadata field would be at all more useful than the existing Headers.
// This field is used to hold arbitrary data you wish to include | ||
// Sarama completely ignores this field and is only to be used for | ||
// pass-through data. | ||
Metadata interface{} |
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.
We already have Headers
… why wouldn’t we use that instead?
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.
Together with the interceptor, I think it will be useful for trace. Context or span can be moved down with this field. For example, in the otelsarama library, an extra channel is opened, messages are read and thrown back into this channel. It can cause performance problems under heavy load.
This is the usage scenario for me, but those who need it in the OnConsume interceptor can move the information they want from here. Or those who want can also use this fielder in the handlers they write.
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.
The header section consists of fields of type []byte and is highly related to kafka messages. I thought it would be nice to have a field that is not controlled by Sarama, but only by the user.
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.
I suppose that’s where my concern comes from: the name isn’t immediately clear that this is a user-controlled side-channel… it sounds like it’s something related to Kafka functionality. As well, what if two interceptors try to use this field for cross-purposes? They would end up clobbering each other’s use, right?
As for putting contexts into the field, my concern is double from the note on the usage from the ̀ context` package:
Do not store Contexts inside a struct type; instead, pass a Context explicitly to each function that needs it.
(Yes, http.Request
has a context embedded in it, this is because http
is under go1 compat rules, and thus cannot change the signature of http.ServeHTTP
, but they still had to put the context somewhere.)
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.
I followed the same approach with ProducerMessage struct. When we did this, I honestly wasn't too worried as the responsibility lies with the user. Nothing in Sarama should depend on/use this field.
Actually, the best thing is to pass the context as a parameter as you said. But as far as I have searched from the issue section, such a thing is not planned at least until v2.
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.
Huh… 🤷♀️ yeah, ok then.
Signed-off-by: Furkan Aksoy <[email protected]>
Thank you for your contribution! However, this pull request has not had any activity in the past 90 days and will be closed in 30 days if no updates occur. |
Hello everyone!
I've been using sarama for a while both in production and in my personal projects. One of the issues I'm lacking is that we don't have the chance to transfer information in the message we consume. I added the Metadata field in the produced messages into the consumed message structure. I think it will be very useful especially for tracing.
I am waiting for your comments, thanks in advance.