Conversation
485b7d2 to
0c9a2f3
Compare
evanh
approved these changes
Apr 23, 2026
0c9a2f3 to
fb8d2bb
Compare
| @@ -110,7 +110,7 @@ def __init__( | |||
| schema: Optional[str] = None, | |||
There was a problem hiding this comment.
Bug: The PyMessage and PyRawMessage constructors accept a headers parameter but silently ignore it, always initializing headers to an empty list.
Severity: MEDIUM
Suggested Fix
The headers parameter should be used to initialize the self._headers attribute in the PyMessage and PyRawMessage constructors. If the intention is to permanently remove header handling, the headers parameter should be removed from the constructor signatures and all call sites to prevent silent data loss.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: sentry_streams/sentry_streams/pipeline/message.py#L110
Potential issue: The constructors for `PyMessage` and `PyRawMessage` accept a `headers`
parameter but the implementation ignores it, always initializing the internal `_headers`
attribute to an empty list. This means any headers passed when creating these message
objects are silently discarded. This violates the API contract and can lead to
unexpected behavior or silent failures in any code that relies on headers being
preserved across processing steps, such as custom filters or monitoring logic.
Did we get this right? 👍 / 👎 to inform future reviews.
This file contains hidden or 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
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.
BUilt on top of #299
One of the main throughput issues we observed in the tests mentioned in
#299 is that headers are
really expensive to pass to and from rust. Most of the issue seems
related with https://github.com/getsentry/streams/blob/main/sentry_streams/src/messages.rs#L47-L82.
Specifically this code is executed every time python references
headers.
This is a test to confirm the impact by removing all the headers related
logic.
All messages are created with empty headers, the header fetching logic
is never called.
I left the support for headers in the rust code because the consumer
to test relies on a header filter so I cannto remove it entirely.
This change is suppsoed to be reverted after we perform a production test