feat(feedback): add User Feedbacks as Events #2604

JoshFerge · 2023-10-13T01:24:34Z

Relay changes for the revamped User Feedback feature. User Feedbacks (Known as UserFeedbackV2), will be events that flow through our system instead of attachments. See here for more information on the tech spec (and the link at the bottom for what we do once they're ingested).

Note that within this code, we're calling it UserReportV2. This is to avoid confusion with the existing UserReport. A PR to shim the previous UserReports as new UserReportV2's will follow, at which point we'll rename this in the code to be UserFeedback. The string feedback is used for the context and ItemType / EventType parsing.

Adds a feature flag for ingest UserReportV2Ingest. This is backed by options https://github.com/getsentry/getsentry/issues/11573
Creates a new EventType UserReportV2
Creates a new ItemType UserReportV2
Creates a new DataCategory UserReportV2
Adds a new context UserReportV2Context that for now contains two properties, contact_email (no pii stripping), and the message which contains the feedback text.

iker-barriocanal

Leaving some questions/points from a preliminary review:

Since we're adding a new data category, we should update the C ABI headers -- make header in relay-cabi should work. We should also update the py/Changelog.
What's the plan for current feedback items that live as attachments?
Should these new feedback items go into a new Kafka topic?
See my comment below on UserReport -> UserFeedback transition.

iker-barriocanal · 2023-10-17T11:48:48Z

relay-base-schema/src/data_category.rs

@@ -108,6 +114,7 @@ impl DataCategory {
            Self::Monitor => "monitor",
            Self::Span => "span",
            Self::MonitorSeat => "monitor_seat",
+            Self::UserReportV2 => "user_report_v2",


Please also update the from_name function above.

relay-base-schema/src/events.rs

iker-barriocanal · 2023-10-17T11:53:25Z

relay-base-schema/src/data_category.rs

+    /// Currently standardized on name UserReportV2 to avoid clashing with the old UserReport.
+    /// TODO(jferg): Rename this to UserFeedback once old UserReport is deprecated.
+    UserReportV2 = 14,


Depending on when UserReport -> UserFeedback transition happens, we may need to support both alternatives to avoid future breaking changes. Have we considered this? Is there any ETA on the deprecation?

Why not use UserFeedback right away? I agree with Iker that renaming later could bring extra complexity and possible compatibility issues.

when discussing with @jan-auer over initial call, he called out to not have it so there are "synonyms" in the code. I can change to UserFeedback if that is preferred.

I think more important than the internal naming is making sure that the serialized string representations in the public interface do not have to be changed, e.g "user_report_v2" for the data category.

The internal names of the types are less important, but I did a case insensitive search for "feedback" and found only a few references in comments, so I would personally go for the new naming straight away, but I'll leave it up to you. cc @jan-auer

relay-dynamic-config/src/feature.rs

iker-barriocanal · 2023-10-17T11:58:52Z

relay-event-schema/src/protocol/contexts/mod.rs

@@ -27,6 +27,7 @@ pub use reprocessing::*;
 pub use response::*;
 pub use runtime::*;
 pub use trace::*;
+pub use user_report_v2::*;


* exports above aren't ideal, could we only export the types we need?

should i include that refactor in this PR, or could I follow up with that? not sure how you all like to split changes like that into separate PRs or not.

relay-server/src/actors/processor.rs

iker-barriocanal · 2023-10-17T12:14:07Z

relay-server/src/envelope.rs

@@ -546,6 +551,7 @@ impl Item {
            ItemType::Statsd | ItemType::MetricBuckets => None,
            ItemType::FormData => None,
            ItemType::UserReport => None,
+            ItemType::UserReportV2 => None,


This means no outcomes will be generated. Is this intended?

for now, yes. will change this once we have data flowing through system and we're closer to alpha etc.

jjbayer · 2023-10-19T09:04:42Z

relay-base-schema/src/data_category.rs

+    /// Currently standardized on name UserReportV2 to avoid clashing with the old UserReport.
+    /// TODO(jferg): Rename this to UserFeedback once old UserReport is deprecated.
+    UserReportV2 = 14,


Why not use UserFeedback right away? I agree with Iker that renaming later could bring extra complexity and possible compatibility issues.

relay-base-schema/src/events.rs

relay-server/src/actors/processor.rs

relay-base-schema/src/data_category.rs

Co-authored-by: Iker Barriocanal <32816711+iker-barriocanal@users.noreply.github.com>

JoshFerge · 2023-10-23T23:59:26Z

Since we're adding a new data category, we should update the C ABI headers -- make header in relay-cabi should work. We should also update the py/Changelog.

done.

What's the plan for current feedback items that live as attachments?

see PR description:

A PR to shim the previous UserReports as new UserReportV2's will follow, at which point we'll rename this in the code to be UserFeedback.

Should these new feedback items go into a new Kafka topic?

No -- we'll process them in our ingest consumer conditioned on the event type. this was chosen over a separate Kafka topic in conversation w/ @jan-auer last week.

See my comment below on UserReport -> UserFeedback transition.

Replied, when Jan and I spoke, there was a decision to avoid synonyms in the code. can change if ya'll would like -- I don't have a strong opinion here. also see PR description:

at which point we'll rename this in the code to be UserFeedback

iker-barriocanal

Pending the relay-cabi update, LGTM. Thanks a lot for working on this PR!

iker-barriocanal · 2023-10-24T13:17:24Z

CHANGELOG.md

@@ -30,6 +30,7 @@
 - Accept spans needed for the mobile Starfish module. ([#2570](https://github.com/getsentry/relay/pull/2570))
 - Extract size metrics and blocking status tag for resource spans. ([#2578](https://github.com/getsentry/relay/pull/2578))
 - Add a setting to rollout ingesting all resource spans. ([#2586](https://github.com/getsentry/relay/pull/2586))
+- Add User Feedback Ingestion. ([#2604](https://github.com/getsentry/relay/pull/2604))


Please also update py/changelog

Also, please move this to the ## Unreleased section at the top of this file.

jjbayer · 2023-10-25T14:58:05Z

py/CHANGELOG.md

- Remove event breadcrumbs dating before January 1, 1970 UTC. ([#2635](https://github.com/getsentry/relay/pull/2635))
- Add `locale` ,`screen_width_pixels`, `screen_height_pixels`, and `uuid` to the device context. ([#2640](https://github.com/getsentry/relay/pull/2640))
+- Add `scraping_attempts` field to the event schema.
+  ([#2575](https://github.com/getsentry/relay/pull/2575))


Did the autoformatter run wild here?

fixed! not sure why my markdown formatter does this.

relay-event-schema/src/protocol/contexts/user_report_v2.rs

JoshFerge · 2023-10-27T18:17:27Z

@iker-barriocanal could you take a look again? need your approval since you requested changes.

iker-barriocanal · 2023-10-30T08:57:58Z

relay-base-schema/src/data_category.rs

@@ -108,6 +114,7 @@ impl DataCategory {
            Self::Monitor => "monitor",
            Self::Span => "span",
            Self::MonitorSeat => "monitor_seat",
+            Self::UserReportV2 => "feedback",


Please add this string to the from_name above before merging, otherwise Relay won't recognize the data category when deserializing.

JoshFerge and others added 8 commits October 12, 2023 15:12

init

6c717ff

feedback context

aafea3e

add ingest feature flag

3909c5a

naming

a24ea5f

renames

4af3e0c

adjust naming

62cf1bd

adjust naming

ab75fe7

Merge branch 'master' into jferg/feedback

a6e1631

JoshFerge changed the title ~~Jferg/feedback~~ feat(feedback): add User Feedbacks as Events Oct 13, 2023

JoshFerge added 2 commits October 12, 2023 18:44

accept snapshot

6e80db1

fix integration test

f27108b

JoshFerge marked this pull request as ready for review October 13, 2023 06:29

JoshFerge requested a review from a team October 13, 2023 06:29

add changelog

d217857

iker-barriocanal assigned JoshFerge Oct 16, 2023

iker-barriocanal suggested changes Oct 17, 2023

View reviewed changes

jjbayer reviewed Oct 19, 2023

View reviewed changes

JoshFerge and others added 3 commits October 23, 2023 16:47

merge

253ec29

Update relay-dynamic-config/src/feature.rs

ae47896

Co-authored-by: Iker Barriocanal <32816711+iker-barriocanal@users.noreply.github.com>

Update relay-base-schema/src/events.rs

137f750

Co-authored-by: Iker Barriocanal <32816711+iker-barriocanal@users.noreply.github.com>

iker-barriocanal reviewed Oct 24, 2023

View reviewed changes

JoshFerge added 5 commits October 24, 2023 13:12

update C headers and changelogs

fac716f

merge

bca72e7

add data category

7ac8207

bump timeout

d346dcf

change the deserialized name to feedback

ab93f41

JoshFerge requested review from iker-barriocanal and jjbayer October 25, 2023 06:56

jjbayer reviewed Oct 25, 2023

View reviewed changes

JoshFerge added 2 commits October 25, 2023 10:06

Fix python changelog

307be2f

use feedback as serialized value

4b73892

JoshFerge requested a review from jjbayer October 25, 2023 17:12

Merge branch 'master' into jferg/feedback

1d5aa20

jjbayer approved these changes Oct 27, 2023

View reviewed changes

fix integration test

95b2176

iker-barriocanal approved these changes Oct 30, 2023

View reviewed changes

JoshFerge and others added 2 commits October 30, 2023 10:19

add feedback to from_name in DataCategory

efe9870

Merge branch 'master' into jferg/feedback

037c882

JoshFerge merged commit 20f72be into master Oct 30, 2023
20 checks passed

JoshFerge deleted the jferg/feedback branch October 30, 2023 18:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(feedback): add User Feedbacks as Events #2604

feat(feedback): add User Feedbacks as Events #2604

JoshFerge commented Oct 13, 2023 •

edited

Loading

iker-barriocanal left a comment

iker-barriocanal Oct 17, 2023

iker-barriocanal Oct 17, 2023

jjbayer Oct 19, 2023

JoshFerge Oct 23, 2023

jjbayer Oct 24, 2023

iker-barriocanal Oct 17, 2023

JoshFerge Oct 23, 2023

iker-barriocanal Oct 17, 2023

JoshFerge Oct 23, 2023

jjbayer Oct 19, 2023

JoshFerge commented Oct 23, 2023

iker-barriocanal left a comment

iker-barriocanal Oct 24, 2023

jjbayer Oct 24, 2023

jjbayer Oct 25, 2023

JoshFerge Oct 25, 2023

JoshFerge commented Oct 27, 2023

iker-barriocanal Oct 30, 2023

feat(feedback): add User Feedbacks as Events #2604

feat(feedback): add User Feedbacks as Events #2604

Conversation

JoshFerge commented Oct 13, 2023 • edited Loading

iker-barriocanal 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

JoshFerge commented Oct 23, 2023

iker-barriocanal 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

JoshFerge commented Oct 27, 2023

Choose a reason for hiding this comment

JoshFerge commented Oct 13, 2023 •

edited

Loading