feat(feedback): Emit outcomes for user feedback events

[cmanallen]

Adds outcomes for user feedback events.

Should there be test coverage for this? Will this handle failure outcomes? Are there other locations that need configuration?

[JoshFerge]

@cmanallen only other thing -- we'll want to emit an accepted outcome in our create_feedback function in Sentry. and see if there's anywhere else there we'd want to emit filtered / dropped there.

[jjbayer]

Should there be test coverage for this? Will this handle failure outcomes? Are there other locations that need configuration?

@cmanallen yes! Best write an integration test here with all the outcomes you expect and work your way backward from there (we can assist with emitting the outcomes because it's not always obvious).

What outcomes do you actually expect? User feedback is not sampled or filtered AFAIK, Will there be quotas or rate limits for user feedback? Or do you want to collect outcomes for invalid items?

[cmanallen]

@jjbayer

Will there be quotas or rate limits for user feedback? Or do you want to collect outcomes for invalid items?

Yes to both quotas and rate-limits. We should record an outcome for invalid items.

[jjbayer]

@cmanallen is this PR still relevant?

[cmanallen]

@jjbayer Added a failing test. We just want to emit an outcome when a feedback was rate-limited.

[jjbayer]

Added a failing test. We just want to emit an outcome when a feedback was rate-limited.

@cmanallen Thanks! Unfortunately the rate limiting code is a bit verbose, but you can look at this PR, specifically the files relay-server/src/utils/rate_limits.rs and relay-server/src/utils/managed_envelope.rs to see what changes are needed for a new data type.

[cmanallen]

@jjbayer Added but still failing. Where is the quantity value incremented?

[jjbayer]

This is the name of the event item type, because of

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

Line 30 in 0608f08

#[serde(rename_all = "lowercase")]

[cmanallen]

According to the coverage in test_feedback.py this is the correct type.

https://github.com/getsentry/relay/blob/master/tests/integration/test_feedback.py#L6

[jjbayer]

@cmanallen I fixed the integration test. I gave you bad advice on the _quantity changes, those are actually not necessary because feedback events are a subtype of events. You can revert them.

[cmanallen]

@jjbayer My mistake I didn't realize you had fixed the code on this branch. In my attempt to fix it on my outdated branch I must have made a typo. You can ignore my earlier comments. I'm still a little confused about the event type. I'm not sure why userreportv2 works and feedback doesn't seeing as its used here: https://github.com/getsentry/relay/blob/master/tests/integration/test_feedback.py#L6.

[jjbayer]

I'm still a little confused about the event type. I'm not sure why userreportv2 works and feedback doesn't seeing as its used here: https://github.com/getsentry/relay/blob/master/tests/integration/test_feedback.py#L6.

I think the linked test works because it sends data to the /envelope endpoint with the correct ItemType. The /store endpoint does not have an item type and looks at the event type instead:

relay/tests/integration/fixtures/__init__.py

Lines 306 to 308 in 23cbd98

	def send_user_feedback(self, project_id, payload):
	envelope = Envelope()
	envelope.add_item(Item(PayloadRef(json=payload), type="feedback"))

[cmanallen]

@jjbayer Makes sense. So this rate-limiting outcome will be emitted for requests to the /envelope endpoint?

[jjbayer]

@jjbayer Makes sense. So this rate-limiting outcome will be emitted for requests to the /envelope endpoint?

@cmanallen it will work for the /store endpoint as well, but the /store endpoint requires the userreportv2 event type in the payload.

[cmanallen]

@jjbayer I'm unfamiliar with this product but I'm pretty sure we use the envelope endpoint to submit feedback. As long as it works there with the feedback type I'm happy :D

[cmanallen]

@jjbayer Is this ready (can be approved) or is there more work to do?