fix(user_report): Treat UserReport like standalone attachment #2984

olksdr · 2024-01-23T07:58:54Z

If the user reports are send in separate envelope from the event, we treat it like a standalone attachment.

changed the processing grouping
added few tests to verify the behaviour
client reports are in separate processing group, since they do not require any events

Dav1dde · 2024-01-23T08:10:37Z

relay-server/src/services/processor.rs

@@ -110,10 +110,11 @@ pub enum ProcessingGroup {
    Error,
    /// Session events.
    Session,
-    /// Attachments which can be sent alone without any event attached to it in the current
+    /// Attachments (including UserReports) which can be sent alone without any event attached to it in the current


Would it make sense to have a separate group of StandaloneUserReport?

The user reports are sent to the attachment kafka topic as well, and from what I understood we treat them like attachments, so the group name kind of reflects the purpose, I think.

But maybe @jan-auer correct me here and provide a little bit more context here.

Makes sense 👍

Maybe we should simply rename this group. This is a group of standalone items that belong to events. Not sure what the best concise name is, but effectively everything that has requires_event() = true while there's no event in the envelope has to go in there.

Done and renamed it to Standalone for now.

iker-barriocanal · 2024-01-23T08:09:18Z

CHANGELOG.md

@@ -13,7 +13,7 @@

 - Add automatic PII scrubbing to `logentry.params`. ([#2956](https://github.com/getsentry/relay/pull/2956))
 - Avoid producing `null` values in metric data. These values were the result of Infinity or NaN values extracted from event data. The values are now discarded during extraction. ([#2958](https://github.com/getsentry/relay/pull/2958))
- Process user reports in separate processing group. ([#2981](https://github.com/getsentry/relay/pull/2981))
+- Add user reports to a correct processing group. ([#2981](https://github.com/getsentry/relay/pull/2981), [#2984](https://github.com/getsentry/relay/pull/2984))


nit: "separate" seems more explicit than "correct", could we find another term to make the changelog a bit more clear?

I will think about this a bit hot to rephrase it.

iker-barriocanal · 2024-01-23T08:10:09Z

relay-server/src/services/processor.rs

-        // Note: only if there is no items in the envelope which can create events.
+        // Extract all standalone attachments and reports.
+        //
+        // Note: only if there is no items in the envelope which can create events, otherwise they


Suggested change

// Note: only if there is no items in the envelope which can create events, otherwise they

// Note: only if there are no items in the envelope which can create events, otherwise they

iker-barriocanal · 2024-01-23T08:17:28Z

tests/integration/test_user_report.py

+
+    event_id = uuid.uuid1().hex
+
+    error_payload = {


I think we could benefit from a test where the error is dropped but not the user report, what do you think?

That's might be good, what condition do you have in mind for this to happen?

Sending an incorrect error should be good enough; but alternatively, send an error that's filtered out by e.g. inbound filters. The user report should be consumed in both cases.

jan-auer · 2024-01-23T09:58:11Z

relay-server/src/services/processor.rs

@@ -110,10 +110,11 @@ pub enum ProcessingGroup {
    Error,
    /// Session events.
    Session,
-    /// Attachments which can be sent alone without any event attached to it in the current
+    /// Attachments (including UserReports) which can be sent alone without any event attached to it in the current


Maybe we should simply rename this group. This is a group of standalone items that belong to events. Not sure what the best concise name is, but effectively everything that has requires_event() = true while there's no event in the envelope has to go in there.

jan-auer · 2024-01-23T09:58:34Z

relay-server/src/services/processor.rs

-                matches!(item.ty(), &ItemType::Attachment | &ItemType::FormData)
+                matches!(
+                    item.ty(),
+                    &ItemType::Attachment | &ItemType::FormData | &ItemType::UserReport


Can we replace this with requires_event to prevent future items missing here?

jan-auer · 2024-01-23T09:59:49Z

relay-server/src/services/processor.rs

    /// envelope.
    StandaloneAttachment,
-    UserReport,
+    /// Outcomes.
+    ClientReport,
    Replay,


While we're at it, could we add docs to the remaining group variants and fix typos in the existing docs?

/// Unknown item types will be forwarded upstream (to processing Relay), where we will /// decide what to do with it. ForwardUnknown, /// All the items in the envelope that could not be grouped. Ungrouped,

fixed I think 😄 thanks for pointing this out

jan-auer

Looks good, thank you for applying the suggestions!

In a follow-up, we can clarify the semantics of creates_event and requires_event, and consider better alternatives to these functions. They do not live up to today's needs anymore.

fix(user_report): Treat UserReport like standalone attachemnt

8f20df2

olksdr requested a review from jan-auer January 23, 2024 07:58

olksdr self-assigned this Jan 23, 2024

olksdr requested a review from a team as a code owner January 23, 2024 07:58

changelog

1f074f7

olksdr changed the title ~~fix(user_report): Treat UserReport like standalone attachemnt~~ fix(user_report): Treat UserReport like standalone attachment Jan 23, 2024

Dav1dde reviewed Jan 23, 2024

View reviewed changes

cleanup

5eda9c9

iker-barriocanal approved these changes Jan 23, 2024

View reviewed changes

olksdr added 2 commits January 23, 2024 10:13

changelog and typo

9e53189

Make sure we drop everything if the errors are rate limited

3391011

jan-auer reviewed Jan 23, 2024

View reviewed changes

review comments

1ce632f

olksdr requested a review from jan-auer January 23, 2024 10:53

jan-auer approved these changes Jan 23, 2024

View reviewed changes

olksdr enabled auto-merge (squash) January 23, 2024 11:20

olksdr merged commit c7e4281 into master Jan 23, 2024
20 checks passed

olksdr deleted the fix/user-feedback branch January 23, 2024 11:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(user_report): Treat UserReport like standalone attachment #2984

fix(user_report): Treat UserReport like standalone attachment #2984

olksdr commented Jan 23, 2024

Dav1dde Jan 23, 2024

olksdr Jan 23, 2024

Dav1dde Jan 23, 2024

jan-auer Jan 23, 2024

olksdr Jan 23, 2024

iker-barriocanal Jan 23, 2024

olksdr Jan 23, 2024

iker-barriocanal Jan 23, 2024

iker-barriocanal Jan 23, 2024

olksdr Jan 23, 2024

iker-barriocanal Jan 23, 2024

jan-auer Jan 23, 2024

jan-auer Jan 23, 2024

olksdr Jan 23, 2024

jan-auer Jan 23, 2024

olksdr Jan 23, 2024

jan-auer left a comment

	// Note: only if there is no items in the envelope which can create events, otherwise they
	// Note: only if there are no items in the envelope which can create events, otherwise they

fix(user_report): Treat UserReport like standalone attachment #2984

fix(user_report): Treat UserReport like standalone attachment #2984

Conversation

olksdr commented Jan 23, 2024

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

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

jan-auer left a comment

Choose a reason for hiding this comment