-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
feat(profiles): Use the profile specific retention value #103234
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
| profile["retention_days"] = quotas.backend.get_event_retention( | ||
| organization=organization, category=DataCategory.PROFILE_DURATION | ||
| ) |
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: Removal of or 90 fallback allows profile["retention_days"] to be None, causing serialization failures.
Severity: CRITICAL | Confidence: 0.95
🔍 Detailed Analysis
The new code removes the or 90 fallback for profile["retention_days"], allowing it to be None if get_event_retention() returns None. This violates the implicit integer type contract expected by downstream serialization processes, such as vroomrs and Kafka message building. If options.get("system.event-retention-days") is not set or is set to 0, None will be passed where an integer is expected, potentially causing serialization failures or type errors during profile processing.
💡 Suggested Fix
Reintroduce a fallback for profile["retention_days"] to ensure it's always an integer, or modify get_event_retention() to guarantee a non-None integer return value.
🤖 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/profiles/task.py#L494-L496
Potential issue: The new code removes the `or 90` fallback for
`profile["retention_days"]`, allowing it to be `None` if `get_event_retention()` returns
`None`. This violates the implicit integer type contract expected by downstream
serialization processes, such as `vroomrs` and Kafka message building. If
`options.get("system.event-retention-days")` is not set or is set to 0, `None` will be
passed where an integer is expected, potentially causing serialization failures or type
errors during profile processing.
Did we get this right? 👍 / 👎 to inform future reviews.
| profile["retention_days"] = quotas.backend.get_event_retention(organization=organization) or 90 | ||
| profile["retention_days"] = quotas.backend.get_event_retention( | ||
| organization=organization, category=DataCategory.PROFILE_DURATION | ||
| ) |
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: Null Retention Value Disrupts Data Serialization
The removal of the or 90 fallback means profile["retention_days"] can now be None when get_event_retention returns None (which happens when the retention setting is 0 or falsy). Downstream code expects retention_days to be an integer for Kafka messages and TraceItem serialization, potentially causing serialization errors or unexpected behavior in consumers.
No description provided.