feat(replay): separate feedback from attachments in the same envelope #3403

aliu39 · 2024-04-09T23:26:57Z

The idea is to restructure the code so that UserReportV2's don't end up in the catch-all extract_kafka_messages at the bottom of store_envelope, with attachments

Relates to https://github.com/getsentry/team-replay/issues/393

Dav1dde

Please see if you can add some tests or modify existing tests (if there are any, why didn't they break with that change?).

Also before moving on with this to SaaS it'd be best if you figured out the wire format for the Feedback topic, it's better to avoid supporting multiple formats on the other "end".

relay-server/src/services/store.rs

…/separate-feedback-from-attachments2

aliu39 · 2024-04-15T19:02:12Z

Also before moving on with this to SaaS it'd be best if you figured out the wire format for the Feedback topic, it's better to avoid supporting multiple formats on the other "end".

Thanks for the review! Could you elaborate what you mean by "wire format"? Do you mean the UserReportV2KafkaMessage type?

You're right, I'll make a ticket for adding that message type and remove it from this PR, it's something I forgot to clean that up. After adding coverage I could deploy this with a FF enabled for s4s only.

Dav1dde · 2024-04-16T06:51:14Z

Thanks for the review! Could you elaborate what you mean by "wire format"? Do you mean the UserReportV2KafkaMessage type?

Yes, you're using the Event right now and it looks like you want to move to UserReportV2KafkaMessage, it's better if you do it early, it's gonna be a lot harder to do later if you have to support both versions (we can't instantly switch Relays to a new version so for a certain period of time all downstream consumers have to support both formats).

aliu39 · 2024-04-16T19:26:39Z

Yes, you're using the Event right now and it looks like you want to move to UserReportV2KafkaMessage, it's better if you do it early, it's gonna be a lot harder to do later if you have to support both versions (we can't instantly switch Relays to a new version so for a certain period of time all downstream consumers have to support both formats).

Got it, I'll definitely look into it and give it more thought before finalizing this PR. Thanks for the suggestion!

…/separate-feedback-from-attachments2

… envelope (#69351) Needed for getsentry/relay#3403 For relay, using an option is way simpler than a FF. This option will also be used to test separating the logic for producing feedbacks (`user_report_v2`'s in `store.rs`)

aliu39 · 2024-04-23T00:28:47Z

Also before moving on with this to SaaS it'd be best if you figured out the wire format for the Feedback topic, it's better to avoid supporting multiple formats on the other "end".

I've decided to hold off on making a UserReportV2KafkaMessage type until the new topic/infra has been deployed to all regions in prod. It adds an extra layer of complexity and testing that will delay this feature, which the SDK team needs to GA user feedback in ~1 week.

It probably shouldn't be done until we write a separate ingest strategy for the feedback consumer (right now it shares this with events).

relay-server/src/services/store.rs

JoshFerge · 2024-04-23T00:34:10Z

relay-server/src/services/store.rs

@@ -189,13 +189,15 @@ impl StoreService {
        let retention = envelope.retention();
        let event_id = envelope.event_id();

+        let use_feedback_ingest_v2 = self.global_config.current().options.feedback_ingest_v2;


not sure what this naming implies, maybe just separate_attachments_from_feedback?

Decided to name it this because it also results in some code restructuring / different code path for producing feedback in StoreService.

I agree the naming's a little funky though, will sleep on it + see what Vienna thinks?

I would also prefer aligning the variable name (and the field name feedback_ingest_v2) with either the feature name ("inline attachments") or renaming it to describe what it actually enables.

The flag's docs say it's temporary. Is it only used for testing in development or should we update the docs?

It's used like a feature flag - we'll set it to true in test region(s), test the SDK, and remove this logic + non-v2 code path once satisfied. The flag is just to quickly "revert" in case things start to break

What did you have in mind for updating the docs?

JoshFerge

other than the small nits / naming, lgtm

aliu39 · 2024-04-23T00:46:04Z

relay-server/src/services/store.rs

+        item: &Item,
+        remote_addr: Option<String>,
+    ) -> Result<(), StoreError> {
+        // check rollout rate option (effectively a FF) to determine whether to produce to new infra


jjbayer · 2024-04-23T09:26:17Z

relay-server/src/services/store.rs

@@ -189,13 +189,15 @@ impl StoreService {
        let retention = envelope.retention();
        let event_id = envelope.event_id();

+        let use_feedback_ingest_v2 = self.global_config.current().options.feedback_ingest_v2;


I would also prefer aligning the variable name (and the field name feedback_ingest_v2) with either the feature name ("inline attachments") or renaming it to describe what it actually enables.

jjbayer · 2024-04-23T09:28:42Z

relay-server/src/services/store.rs

+                ItemType::UserReportV2 => {
+                    if use_feedback_ingest_v2 {


With this, you can make sure that there's an error log if there's an unexpected feedback item in the envelope.

Suggested change

ItemType::UserReportV2 => {

if use_feedback_ingest_v2 {

ItemType::UserReportV2 if use_feedback_ingest_v2 => {

jjbayer · 2024-04-23T09:30:12Z

relay-server/src/services/store.rs

+        let topic = if is_rolled_out(organization_id, feedback_ingest_topic_rollout_rate) {
+            KafkaTopic::Feedback
+        } else {
+            KafkaTopic::Events
+        };


Out of scope for this PR, but should we hard-code this to Feedback soon? Or are some installations not ready for that yet? (self-hosted, devenv, ...)

Yes this has to do with ops deployment of the feedback topic -- right now, its only in s4s and not other regions. This check and rollout rate will eventually be removed too

jjbayer · 2024-04-23T09:30:48Z

tests/.idea/workspace.xml

Did you check this in on purpose?

iker-barriocanal · 2024-04-23T11:43:30Z

tests/integration/test_feedback.py

+    if use_feedback_topic:
+        mini_sentry.set_global_config_option("feedback.ingest-topic.rollout-rate", 1.0)
+        feedback_consumer_ = feedback_consumer(timeout=20)
+        other_consumer_ = events_consumer(timeout=20)


Why the _ suffix?

iker-barriocanal · 2024-04-23T11:45:29Z

relay-server/src/services/store.rs

@@ -189,13 +189,15 @@ impl StoreService {
        let retention = envelope.retention();
        let event_id = envelope.event_id();

+        let use_feedback_ingest_v2 = self.global_config.current().options.feedback_ingest_v2;


The flag's docs say it's temporary. Is it only used for testing in development or should we update the docs?

… envelope (#69351) Needed for getsentry/relay#3403 For relay, using an option is way simpler than a FF. This option will also be used to test separating the logic for producing feedbacks (`user_report_v2`'s in `store.rs`)

iker-barriocanal

LGTM! Please see comments below before merging.

iker-barriocanal · 2024-04-25T08:13:54Z

tests/.idea/workspace.xml

Please, remove this file.

iker-barriocanal · 2024-04-25T08:21:04Z

relay-dynamic-config/src/global.rs

+    /// Flag for handling feedback and attachments in the same envelope. Temporary FF for fast-revert
+    /// (will remove after user feedback GA release).


Without context, this comment is not clear to me what the flag should do. What do you think about something like the following?

Suggested change

/// Flag for handling feedback and attachments in the same envelope. Temporary FF for fast-revert

/// (will remove after user feedback GA release).

/// Kill switch to stop producing feedback items separately into Kafka. The topic is controlled by `feedback_ingest_topic_rollout_rate`.

tests/integration/test_feedback.py

iker-barriocanal · 2024-04-25T08:46:48Z

@aliu3ntry please address the suggestions above, I didn't realize the PR had auto-merging enabled.

aliu39 · 2024-04-25T16:06:56Z

Oops, sorry! Reopened at #3486 @iker-barriocanal

Follow-up from #3403. #skip-changelog (doc and renaming)

feat(replay): separate feedback from attachments in the same envelope

706dab1

aliu39 requested a review from a team April 9, 2024 23:27

aliu39 and others added 2 commits April 11, 2024 13:56

Revert changes to KafkaMessage and leave comment

dbfc1dc

Merge branch 'master' into aliu/separate-feedback-from-attachments2

d4a612a

aliu39 marked this pull request as ready for review April 11, 2024 18:12

aliu39 requested a review from a team as a code owner April 11, 2024 18:12

Dav1dde reviewed Apr 12, 2024

View reviewed changes

relay-server/src/services/store.rs Outdated Show resolved Hide resolved

relay-server/src/services/store.rs Outdated Show resolved Hide resolved

relay-server/src/services/store.rs Outdated Show resolved Hide resolved

Dav1dde assigned aliu39 Apr 12, 2024

Merge branch 'master' of https://github.com/getsentry/relay into aliu…

e5f7461

…/separate-feedback-from-attachments2

aliu39 added 2 commits April 19, 2024 13:39

Merge branch 'master' of https://github.com/getsentry/relay into aliu…

03961b4

…/separate-feedback-from-attachments2

Move feedback topic logic to produce_user_report_v2

6fc34ed

aliu39 marked this pull request as draft April 19, 2024 20:47

aliu39 mentioned this pull request Apr 19, 2024

feat(feedback): add option for ingesting feedback+attachments in same envelope getsentry/sentry#69351

Merged

aliu39 mentioned this pull request Apr 19, 2024

feat(feedback): create a KafkaMessage type for user feedback #3467

Closed

Rm kafka type (split to another PR)

c00d82c

aliu39 requested a review from JoshFerge April 22, 2024 19:10

aliu39 and others added 9 commits April 22, 2024 14:25

Merge branch 'master' into aliu/separate-feedback-from-attachments2

117d674

Add UserReportV2KafkaMessage

f0a9f7a

Changelog

5ab4197

Use option flag for old/new code path

af89ee4

Reorder produce_user_report_v2

e53603f

Draft WIP coverage

4dd7b1b

Rm .idea/

1c06509

Remove KafkaMessageType and finish integration test

0fa44f4

Revert consts.py

41f5597

aliu39 marked this pull request as ready for review April 23, 2024 00:09

aliu39 added 2 commits April 22, 2024 17:10

Rm .idea

870f623

Parametrize other tests with inline-attachments option

b2ef5e7

JoshFerge reviewed Apr 23, 2024

View reviewed changes

relay-server/src/services/store.rs Outdated Show resolved Hide resolved

JoshFerge reviewed Apr 23, 2024

View reviewed changes

aliu39 added 2 commits April 22, 2024 17:42

Update flag desc and naming in test

e1f1597

Rm Item case comments

ba5ac3b

aliu39 commented Apr 23, 2024

View reviewed changes

aliu39 requested review from a team and Dav1dde April 23, 2024 00:52

jjbayer reviewed Apr 23, 2024

View reviewed changes

iker-barriocanal reviewed Apr 23, 2024

View reviewed changes

Tweaks for review

9a81a1b

aliu39 enabled auto-merge (squash) April 24, 2024 15:56

aliu39 requested review from jjbayer, iker-barriocanal and a team and removed request for Dav1dde April 24, 2024 16:01

iker-barriocanal approved these changes Apr 25, 2024

View reviewed changes

aliu39 merged commit 2f3c8ff into master Apr 25, 2024
21 checks passed

aliu39 deleted the aliu/separate-feedback-from-attachments2 branch April 25, 2024 08:45

aliu39 mentioned this pull request Apr 25, 2024

ref(feedback): update integration test and doc for #3403 #3486

Merged

aliu39 added a commit that referenced this pull request Apr 26, 2024

ref(feedback): update integration test and doc for #3403 (#3486)

6503900

Follow-up from #3403. #skip-changelog (doc and renaming)

bruno-garcia mentioned this pull request Apr 30, 2024

feat(feedback): Add captureFeedback method getsentry/sentry-javascript#11428

Merged

aliu39 mentioned this pull request May 15, 2024

metrics(processor): emit counter for feedback screenshots #3603

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(replay): separate feedback from attachments in the same envelope #3403

feat(replay): separate feedback from attachments in the same envelope #3403

aliu39 commented Apr 9, 2024 •

edited

Loading

Dav1dde left a comment

aliu39 commented Apr 15, 2024

Dav1dde commented Apr 16, 2024

aliu39 commented Apr 16, 2024

aliu39 commented Apr 23, 2024

JoshFerge Apr 23, 2024

aliu39 Apr 23, 2024

jjbayer Apr 23, 2024

iker-barriocanal Apr 23, 2024

aliu39 Apr 23, 2024

aliu39 Apr 23, 2024

JoshFerge left a comment

aliu39 Apr 23, 2024

jjbayer Apr 23, 2024

jjbayer Apr 23, 2024

jjbayer Apr 23, 2024

aliu39 Apr 23, 2024

jjbayer Apr 23, 2024

aliu39 Apr 23, 2024

iker-barriocanal Apr 23, 2024

iker-barriocanal Apr 23, 2024

iker-barriocanal left a comment

iker-barriocanal Apr 25, 2024

iker-barriocanal Apr 25, 2024

iker-barriocanal commented Apr 25, 2024

aliu39 commented Apr 25, 2024

	ItemType::UserReportV2 => {
	if use_feedback_ingest_v2 {
	ItemType::UserReportV2 if use_feedback_ingest_v2 => {

		/// Flag for handling feedback and attachments in the same envelope. Temporary FF for fast-revert
		/// (will remove after user feedback GA release).

	/// Flag for handling feedback and attachments in the same envelope. Temporary FF for fast-revert
	/// (will remove after user feedback GA release).
	/// Kill switch to stop producing feedback items separately into Kafka. The topic is controlled by `feedback_ingest_topic_rollout_rate`.

feat(replay): separate feedback from attachments in the same envelope #3403

feat(replay): separate feedback from attachments in the same envelope #3403

Conversation

aliu39 commented Apr 9, 2024 • edited Loading

Dav1dde left a comment

Choose a reason for hiding this comment

aliu39 commented Apr 15, 2024

Dav1dde commented Apr 16, 2024

aliu39 commented Apr 16, 2024

aliu39 commented Apr 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

JoshFerge 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

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

iker-barriocanal commented Apr 25, 2024

aliu39 commented Apr 25, 2024

aliu39 commented Apr 9, 2024 •

edited

Loading