ref(server): Maintain copy of event item headers #3414

jjbayer · 2024-04-12T06:59:35Z

#3375 added a third event item header that needs to be extracted from the incoming item, updated and then re-serialized back into the event item manually:

flowchart LR
Item --> payload
Item --> headers
headers --> metrics_extracted
headers --> sample_rates
headers --> spans_extracted
payload --> Event
Event --> payload2
metrics_extracted --> headers2
sample_rates --> headers2
spans_extracted --> headers2
payload2 --> Item2
headers2 --> Item2

By maintaining an instance of ItemHeaders instead of a bunch of individual fields on ProcessEnvelopeState, simplify to:

flowchart LR
Item --> payload
Item --> headers
payload --> Event
Event --> payload2
payload2 --> Item2
headers ---> Item2

Eventually, we could remove both event and event_headers from the stat, and make them return values of event::extract instead.

#skip-changelog

Dav1dde

I think we should just use a default if we don't have headers, then set all the things we need to set and when serializing we can rely on skip_serializing_if.

Dav1dde · 2024-04-16T14:26:25Z

relay-server/src/envelope.rs

@@ -1003,6 +1013,11 @@ impl Item {
            ItemType::Unknown(_) => false,
        }
    }
+
+    pub fn into_parts(self) -> (ItemHeaders, Bytes) {


Missing docs, isn't there a clippy lint 🤔

Removed in favor of destructuring.

Dav1dde · 2024-04-16T14:27:03Z

relay-server/src/services/processor.rs

    ///
-    /// Only applies to envelopes with a transaction item.
-    spans_extracted: bool,
+    /// `None` if the envelope does not contain an event item.


Should we just use a default here?

ItemHeaders does not implement Default atm, it would require a default ItemType which sounds dangerous. In the end I think we should remove this from the state (see other comment).

Should we use something like item headers instead which can represent the "default" case and also can be constructed from ItemHeaders as well as be turned into Option<ItemHeaders> again?

Dav1dde · 2024-04-16T14:28:31Z

relay-server/src/services/processor/span/processing.rs

@@ -346,7 +350,9 @@ pub fn extract_from_event(

    add_span(transaction_span.into());

-    state.spans_extracted = true;
+    if let Some(headers) = state.event_headers.as_mut() {


What if it is not None, we should still set it to true?

I realized I can keep the reference from the beginning of the function alive and make it mutable.

Semantically, state.event_headers cannot be None if state.event is not empty, so my first attempt at this PR was to put them in an Option together. But that made event handling more complicated because of the two option levels (Option(Annotated(Option(Event), _))). Long term, I believe we should remove both from the state and pass them as plain old variables, as you suggested.

jjbayer · 2024-04-16T11:41:31Z

relay-server/src/envelope.rs

@@ -481,34 +481,34 @@ fn is_false(val: &bool) -> bool {
 pub struct ItemHeaders {
    /// The type of the item.
    #[serde(rename = "type")]
-    ty: ItemType,
+    pub ty: ItemType,


Item and ItemHeaders were some of the few serializable data types on which we restricted field access. Making them public seems more aligned with other data types, and is necessary to access its fields during processing.

jjbayer · 2024-04-16T11:43:09Z

relay-server/src/envelope.rs

+    /// The release, used by security reports.
+    #[serde(default, skip_serializing_if = "Option::is_none")]
+    pub sentry_release: Option<String>,
+
+    /// The environment, used by security reports.
+    #[serde(default, skip_serializing_if = "Option::is_none")]
+    pub sentry_environment: Option<String>,


These were previously set and read using string access.

Dav1dde · 2024-04-17T09:28:25Z

relay-server/src/services/processor/span/processing.rs

+    let Some(event_headers) = state.event_headers.as_mut() else {
+        return;
+    };


Should we log an error here if we don't have any headers? This seems very error prone and is going back to the problem we tried to solve (implicit optional state).

Dav1dde · 2024-04-17T09:29:55Z

relay-server/src/services/processor.rs

    ///
-    /// Only applies to envelopes with a transaction item.
-    spans_extracted: bool,
+    /// `None` if the envelope does not contain an event item.


Should we use something like item headers instead which can represent the "default" case and also can be constructed from ItemHeaders as well as be turned into Option<ItemHeaders> again?

Dav1dde

This feels like a step backward and forward at the same time. It makes a bunch of stuff nicer but at the same time adds more of the implicit state we wanted to get rid of to the ProcessEnvelopeState.

There is maybe a way to make the event_headers not optional (see previous comments), but I can't tell if that's worth the effort.

Overall I think it makes it better and it's just a temporary "bandaid" until we will further improve the processor.

jjbayer · 2024-04-17T12:46:06Z

Should we use something like item headers instead which can represent the "default" case and also can be constructed from ItemHeaders as well as be turned into Option again?

Thanks, this made me realize there is a bug in the PR (which no test caught): Item::set_payload and Item::new modify the headers, so we should not overwrite the entire header struct afterwards. This is a good reason to keep Item::headers a private field.

Let's skip this change and work toward moving these intermediary values off the state instead.

ref(server): Maintain copy of event item headers

55c1bae

Dav1dde assigned jjbayer Apr 12, 2024

jjbayer added 2 commits April 12, 2024 12:47

fixes

4849aa4

forgot pub

89238c3

jjbayer marked this pull request as ready for review April 16, 2024 11:45

jjbayer requested a review from a team as a code owner April 16, 2024 11:45

Dav1dde reviewed Apr 16, 2024

View reviewed changes

jjbayer commented Apr 17, 2024

View reviewed changes

jjbayer added 2 commits April 17, 2024 09:46

Merge remote-tracking branch 'origin/master' into ref/item-headers

c39b678

review comments

715e61a

Dav1dde reviewed Apr 17, 2024

View reviewed changes

Dav1dde approved these changes Apr 17, 2024

View reviewed changes

jjbayer closed this Apr 17, 2024

jan-auer deleted the ref/item-headers branch July 4, 2024 11:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ref(server): Maintain copy of event item headers #3414

ref(server): Maintain copy of event item headers #3414

jjbayer commented Apr 12, 2024 •

edited

Loading

Dav1dde left a comment

Dav1dde Apr 16, 2024

jjbayer Apr 17, 2024

Dav1dde Apr 16, 2024

jjbayer Apr 17, 2024

Dav1dde Apr 17, 2024

Dav1dde Apr 16, 2024

jjbayer Apr 17, 2024

jjbayer Apr 16, 2024

jjbayer Apr 16, 2024

Dav1dde Apr 17, 2024

Dav1dde Apr 17, 2024

Dav1dde left a comment

jjbayer commented Apr 17, 2024

ref(server): Maintain copy of event item headers #3414

ref(server): Maintain copy of event item headers #3414

Conversation

jjbayer commented Apr 12, 2024 • edited Loading

Dav1dde left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dav1dde left a comment

Choose a reason for hiding this comment

jjbayer commented Apr 17, 2024

jjbayer commented Apr 12, 2024 •

edited

Loading