feat(processing): Generic trace attachments #5457
Conversation
…om:getsentry/relay into tobias-wilfert/feat/attachmentv2-scrubbing
…-scrubbing' into feat/trace-attachments
jjbayer
changed the base branch from
master
to
tobias-wilfert/feat/attachmentv2-scrubbing
…-scrubbing' into feat/trace-attachments
| let attachments = envelope | ||
| .envelope_mut() | ||
| .take_items_by(|item| item.is_attachment_v2()) | ||
| .take_items_by(|item| item.is_span_attachment()) |
There was a problem hiding this comment.
The spans processor is now only concerned with trace attachments that have a span_id header.
| item.set_parent_id(parent_id); | ||
|
|
||
| Ok(item) | ||
| } |
There was a problem hiding this comment.
I moved some of the functions into the new trace_attachments module.
| Ok(ExpandedAttachment { | ||
| parent_id: item.parent_id().cloned(), | ||
| meta, | ||
| body: payload.slice_ref(body), |
There was a problem hiding this comment.
This function was moved over from the spans module, but I changed this to slice_ref to prevent copying the data.
|
|
||
| metric!( | ||
| timer(RelayTimers::AttachmentScrubbing), | ||
| attachment_type = "trace_attachment", |
There was a problem hiding this comment.
I changed the metric tag from attachmentv2 to trace_attachment here.
|
|
||
|
|
||
| def test_standalone_attachment_forwarding(mini_sentry, relay): | ||
| @pytest.mark.parametrize("owned_by", ["spans", "trace"]) |
There was a problem hiding this comment.
I tried to do most of the testing by extending / parametrizing the existing span attachment tests.
| def test_span_attachment_independent_rate_limiting( | ||
| mini_sentry, | ||
| relay, | ||
| outcomes_consumer, |
There was a problem hiding this comment.
I removed the dependency on the outcomes_consumer here to speed up the test. Since the test uses a non-processing relay, the outcomes_consumer would always be empty anyway.
|
|
||
|
|
||
| def _add_sampling_config( | ||
| def add_sampling_config( |
There was a problem hiding this comment.
This function is imported from other modules, so I removed the leading underscore.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| #[derive(Clone, Debug, Default, PartialEq, Empty, FromValue, IntoValue, ProcessValue)] | ||
| pub struct AttachmentV2Meta { | ||
| pub struct TraceAttachmentMeta { |
There was a problem hiding this comment.
The name of this file is still attachment_v2 do we want to change that over to something else as well?
The only other spot we now have this naming is ContentType::AttachmentV2 I think there we can leave it though unless you think ContentType::TraceAttachment is better.
There was a problem hiding this comment.
I changed the name to TraceAttachment now, and also the content-type string. I added application/vnd.sentry.attachment.v2 as a legacy alias.
Co-authored-by: Sebastian Zivota <[email protected]>
| (!items.is_empty()).then(|| { | ||
| let work = SerializedAttachments { headers, items }; | ||
| Managed::from_envelope(envelope, work) | ||
| }) |
There was a problem hiding this comment.
This is purely defensive, right? Don't think items can/should every be empty.
There was a problem hiding this comment.
I believe @Dav1dde's intention is to eventually get rid of split_envelope, and then prepare_envelope needs to be written in a way that they can handle any envelope.
tobias-wilfert
left a comment
tobias-wilfert
left a comment
There was a problem hiding this comment.
Looks good and thanks for all the small fix ups along the way 🙏🏻
Add a new processing pipeline for attachments that are not explicitly associated to spans. These attachments are only associated by their trace ID, so they do not check span rate limits, but are stored in the same way as span attachments.
See https://develop.sentry.dev/sdk/data-model/envelope-items/#trace-attachment.
Closes INGEST-654.