-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
feat(segment-enrichment): Bump segment clusterer rule lifetimes #104280
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
base: master
Are you sure you want to change the base?
Conversation
|
|
||
|
|
||
| def _bump_rule_lifetime(project: Project, event_data: Mapping[str, Any]) -> None: | ||
| applied_rules = event_data.get("_meta", {}).get("transaction", {}).get("", {}).get("rem", {}) |
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.
Bug: applied_rules on line 191 defaults to a dictionary ({}) instead of a list ([]), causing a type mismatch with expected Sequence iteration.
Severity: MEDIUM | Confidence: High
🔍 Detailed Analysis
On line 191 of src/sentry/ingest/transaction_clusterer/datasource/redis.py, the applied_rules variable defaults to an empty dictionary ({}) when the rem field is absent from transaction event data. This creates a type mismatch because the code expects applied_rules to be a Sequence (specifically a list) for iteration, as evidenced by test data, type annotations, and the loop logic on lines 210-217. This inconsistency with the segment case (line 200, which defaults to []) means that if a non-empty dictionary is ever provided, the iteration logic will fail.
💡 Suggested Fix
Change the default value for applied_rules on line 191 from {} to [] to align with the expected Sequence type and the segment implementation.
🤖 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: src/sentry/ingest/transaction_clusterer/datasource/redis.py#L191
Potential issue: On line 191 of
`src/sentry/ingest/transaction_clusterer/datasource/redis.py`, the `applied_rules`
variable defaults to an empty dictionary (`{}`) when the `rem` field is absent from
transaction event data. This creates a type mismatch because the code expects
`applied_rules` to be a `Sequence` (specifically a list) for iteration, as evidenced by
test data, type annotations, and the loop logic on lines 210-217. This inconsistency
with the segment case (line 200, which defaults to `[]`) means that if a non-empty
dictionary is ever provided, the iteration logic will fail.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 4996900
| ) | ||
| meta = orjson.loads(meta_str) | ||
| applied_rules = meta.get("meta", {}).get("", {}).get("rem", []) | ||
| _bump_rule_lifetime_inner(project, applied_rules) |
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.
Bug: Missing null check before parsing JSON metadata
The _bump_rule_lifetime_for_segment function calls attribute_value which returns None when the requested attribute doesn't exist on the span. The result is immediately passed to orjson.loads(meta_str) without a null check, which will raise a TypeError when meta_str is None. This occurs when a segment span hasn't had any clustering rules applied to it (e.g., new segment name patterns before rules are generated). While safe_execute catches the exception, this causes unnecessary errors rather than gracefully handling the case where no rules were applied.
Last in a series of PRs (#103739, #103913, #104192) to port transaction name clustering to segment enrichment for segments.
This adds the final step, bumping lifetime rules after their application. Since the arguments for transactions and segments are different, refactored the existing
_bump_rule_lifetimeinto two entry points that munge arguments before passing them to the existing logic.