-
Notifications
You must be signed in to change notification settings - Fork 93
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(kafka): Report producer errors #677
Conversation
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.
🍪
relay-server/src/utils/kafka.rs
Outdated
type DeliveryOpaque = (); | ||
fn delivery(&self, result: &DeliveryResult, _delivery_opaque: Self::DeliveryOpaque) { | ||
if let Err((error, _message)) = result { | ||
log::error!("callback producer error: {}", LogError(error)); |
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.
nit: Since this is logged in Sentry, maybe make this more verbose?
log::error!("callback producer error: {}", LogError(error)); | |
log::error!("failed to produce message to kafka: {}", LogError(error)); |
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.
Makes sense, but I think we should include a hint that this callback was triggered asynchronously. What do you think about now?
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.
Ah, missed your question earlier. Yeah, looks good now!
Currently, asynchronous errors (those happening after the event was queued) in the kafka producer are silently dropped. With this change, the goal is to report those errors to Sentry (or at least write them in the logs).