feat(minidump): Stream Minidumps to objectstore#5909
feat(minidump): Stream Minidumps to objectstore#5909tobias-wilfert wants to merge 15 commits intomasterfrom
Conversation
| pub async fn upload_to_objectstore<S, E>( | ||
| stream: S, | ||
| content_type: Option<String>, | ||
| mut item: Item, | ||
| config: &Config, | ||
| scoping: Scoping, | ||
| upload: &Addr<Upload>, | ||
| referrer: &'static str, |
There was a problem hiding this comment.
I think a 'typed stream' would be nice here as a follow up. As in something that bundles the stream, content_type and referrer.
|
|
||
| let upload_context = if should_fetch_project_config(&state, meta.project_id()) { | ||
| // Ensure that we really make it here. | ||
| relay_statsd::metric!(counter(RelayCounters::MinidumpEndpointConfigFetching) += 1); |
There was a problem hiding this comment.
Left this in for now since it gives us a free insight on how often we get raw_minidumps (will remove when adding the kill switch).
tobias-wilfert
changed the title
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Co-authored-by: Joris Bayer <joris.bayer@sentry.io>
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 1297179. Configure here.
| if rate_limits.is_limited() { | ||
| return Err(BadStoreRequest::RateLimited(rate_limits)); | ||
| } |
There was a problem hiding this comment.
Bug: The new error quota check in upload_context_for_project can prematurely return a 429 error, bypassing logic that explicitly ignores rate limits for minidump uploads.
Severity: HIGH
Suggested Fix
The error quota check should not reject the request with a BadStoreRequest::RateLimited error. Instead, it should be used to conditionally control whether minidumps are streamed to the object store (e.g., by setting a boolean flag), while still allowing the rest of the minidump processing to complete without returning a 429 error.
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: relay-server/src/endpoints/minidump.rs#L318-L320
Potential issue: A new rate limit check in `upload_context_for_project` checks for error
quotas and returns `Err(BadStoreRequest::RateLimited(...))` if a project's quota is
exceeded. In the minidump `handle` function, this error propagates and is converted into
an HTTP 429 response before the envelope is processed. This bypasses the existing
`ignore_rate_limits()` call, which is specifically designed to prevent the minidump
endpoint from ever returning a 429 status, as clients often retry these requests. The
new check breaks this intended behavior.
There was a problem hiding this comment.
Talked with Joris and the plan is to address this.
3 participants
Follow up to #5877. This now also adds the logic for streaming minidumps to the objectstore. Just as for attachments, streaming minidumps is guarded by a feature flag.
Note: This only deals with the "happy path". Streaming compressed minidumps to the object store is not supported, neither is embedded minidumps. Furthermore streaming minidumps means no pii-scrubbing.
Fix: INGEST-871