Repository navigation
Conversation
![sentry-warden[bot]](https://avatars.githubusercontent.com/in/2755838?s=60&v=4){kind=small}
philprime
changed the title
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
Local end-to-end testing exposed a parsing limit that is not part of the flamegraph contract. The MetricKit example has 719 frame entries, 19 trees, and a maximum call-path depth of 130. It is only about 128 KiB, but its nested nodes/children exceed serde_json's default recursion limit. Relay consequently dropped the attachment while retaining the event. The attachment specification limits serialized size to 10 MiB, not call-tree depth. Truncating the input to fit the parser would lose the diagnostic context this attachment format is intended to preserve. Fixed in 28b2c56. Parsing uses stack growth for deep call trees, with iterative node destruction so dropping the parsed tree is also stack-safe. The byte budget is checked before parsing. Flattened frame data retains the normal JSON nesting limit, and the existing schema and sample-count validation remain in place. Coverage includes a synthetic 1,000-frame call path and rejection of an invalid count at that depth. Replaying the untouched example through the local stacked implementation retained all 19 trees and 719 frame entries through ingestion, attachment processing, and browser rendering. |
| managed.retain(select, |attachment, _| { | ||
| if attachment.attachment_type() == Some(AttachmentType::Flamegraph) | ||
| && !has_valid_flamegraph_headers(attachment) | ||
| { | ||
| return Err(ProcessingError::InvalidFlamegraph); | ||
| } | ||
| if ctx.is_processing() { | ||
| validate(attachment, ctx.config) | ||
| } else { | ||
| Ok(()) | ||
| } | ||
| }); |
There was a problem hiding this comment.
I want to clean up attachment validation in a separate PR. That is, be very defensive about what can be a standalone attachment, and only run deep validation when the given attachment type is allowed in the current processor.
| if attachment.attachment_type() == Some(AttachmentType::Flamegraph) | ||
| && !has_valid_flamegraph_headers(attachment) | ||
| { | ||
| return Err(ProcessingError::InvalidFlamegraph); | ||
| } |
There was a problem hiding this comment.
This check is duplicated now between this function and validate. I think it only belongs in the latter.
| - Span name and description inference now support array attributes. ([#6482](https://github.com/getsentry/relay/pull/6482)) | ||
| - Added support for Integrity policy ingestion, not publicly accessible. ([#6465](https://github.com/getsentry/relay/pull/6465), [#6512](https://github.com/getsentry/relay/pull/6512)) | ||
| - Add dimensions to rate limit caches. ([#6479](https://github.com/getsentry/relay/pull/6479)) | ||
| - Process `event.flamegraph` attachments associated with events. ([#6473](https://github.com/getsentry/relay/pull/6473)) |
There was a problem hiding this comment.
This needs to be moved to the correct section.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 48c22e0. Configure here.
| let serialized = serde_json::to_vec(&graph)?; | ||
|
|
||
| Ok(serialized.into()) | ||
| } |
There was a problem hiding this comment.
Validation rewrites flamegraph payloads
High Severity
validate re-serializes the parsed graph and replaces the item payload, so unknown fields and symbolication inputs such as debug-image arch are stripped. The module docs and attachment contract describe validation without rewriting captured data, and downstream Kafka and objectstore ingestion then store this reduced payload.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 48c22e0. Configure here.
There was a problem hiding this comment.
This also seems correct, perhaps it's enough to give the relevant types an other field?
| validate(attachment, ctx.config) | ||
| } else { | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Non-processing Relays skip header checks
Medium Severity
validate_attachments returns success for every item when is_processing() is false, so required flamegraph filename and content-type checks never run on non-processing Relays. Those checks live only inside validate, which is skipped on this path and compiled out without the processing feature. The function comment and test_flamegraph_headers_on_non_processing_relay both expect transport checks on every Relay.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 48c22e0. Configure here.
There was a problem hiding this comment.
It's true that the validation now only happens in processing Relays, but I don't know how much of a problem that is.
There was a problem hiding this comment.
I think @Dav1dde had an opinion on that, but not 100% I can recite the conclusion
This branch has not been deployed
4 participants
Note
This is my first contribution to relay, so it's heavily assisted by agents. Please let me know how you want me to continue working on it. Happy for any guidance for working no Relay without breaking best-practices.
Recognize and validate
event.flamegraphattachments against the draft contract in getsentry/sentry-docs#19774, preserving their type and original payload through Kafka and objectstore ingestion.Validation
Required headers and association with an explicit event or transaction in the same envelope are checked through shared attachment processing, including on non-processing Relays. Invalid attachments produce
invalid_flamegraphoutcomes without discarding the associated event. The 10 MiB cap uses the existing size-limit path, preserving shared attachment count and byte budgets, rejection metrics, and whole-envelope HTTP 413 behavior for oversized items.Processing Relays validate version 1 payloads using the V2 profiling frame and debug metadata schemas: non-empty tables and roots, native addresses, relative image references, frame indices, integer sample-count bounds, and child-count invariants. Debug-image IDs are indexed once to avoid quadratic lookup work. Validation leaves unknown fields and original symbolication inputs unchanged.
Boundary
Symbolication, its 2,500-input-frame budget, inline expansion, symbolication state, and rendering belong to separate Sentry backend and frontend PRs. Relay does not infer source thread identity or process provenance, and cannot determine whether an omitted absolute-address debug image was known to the producer. Existing attachment quotas and privacy rules still apply. This change does not add structured scrubbing or multipart filename inference.
Verification
make check, the all-feature build, default-feature Clippy with warnings denied, seven default-feature flamegraph Rust tests, eleven all-feature flamegraph Rust tests, and three transport integration cases passed locally. Added coverage checks shared size budgets, non-processing header validation, association outcomes, and large relative-address tables.Processing integration remains blocked locally by unavailable Redis services. Corrected invalid-attachment integration tests to read the preserved event from the events topic and assert that the attachments topic remains empty. Kafka and objectstore payload-preservation coverage is included but was not reverified locally during this update. The full all-feature Rust suite was not rerun.