feat(stats): Add quantity to TrackOutcome and new attachment outcomes #942
Conversation
| Some(envelope) => { | ||
| envelope_summary.replace(EnvelopeSummary::compute(&envelope)); | ||
| Ok(envelope) | ||
| } |
There was a problem hiding this comment.
Is this the latest place where we need to update the envelope summary?
(I love how it can be written like this in the Rust syntax, btw.)
There was a problem hiding this comment.
Similar to the case in endpoints/common.rs, you'll have to make another update after calling ProcessEnvelope below once rate limiting can emit outcomes. For now, a comment will suffice.
Technically, that would also apply to the replace you added here. However, it is fine to do it in this instance. Prior to invoking the HandleEnvelope handler, we split envelopes into event-related and event-unrelated items.
|
I think the main code is good to move forward. The integration tests are passing but the new outcomes seem not to be exercised. I'm continuing to muddle around with that but let me know if you can suggest an easy addition. |
relay-server/src/endpoints/common.rs
Outdated
| @@ -446,6 +446,8 @@ where | |||
| Some(envelope) => envelope, | |||
| None => return Err(BadStoreRequest::RateLimited(checked.rate_limits)), | |||
| }; | |||
| // TODO: Ensure that outcomes are emitted correctly for rate-limited attachments | |||
| // so that it is safe to update envelope_summary here. | |||
There was a problem hiding this comment.
Technically, we will have to update the envelope summary here. We chose not to do this just yet, until the rate limiting code inside CheckEnvelope can emit its own outcomes.
| Some(envelope) => { | ||
| envelope_summary.replace(EnvelopeSummary::compute(&envelope)); | ||
| Ok(envelope) | ||
| } |
There was a problem hiding this comment.
Similar to the case in endpoints/common.rs, you'll have to make another update after calling ProcessEnvelope below once rate limiting can emit outcomes. For now, a comment will suffice.
Technically, that would also apply to the replace you added here. However, it is fine to do it in this instance. Prior to invoking the HandleEnvelope handler, we split envelopes into event-related and event-unrelated items.
relay-server/src/actors/events.rs
Outdated
| @@ -1483,12 +1483,13 @@ impl Handler<HandleEnvelope> for EventManager { | |||
|
|
|||
| let scoping = Rc::new(RefCell::new(envelope.meta().get_partial_scoping())); | |||
| let is_received = Rc::new(AtomicBool::from(false)); | |||
| let envelope_summary = Rc::new(RefCell::new(EnvelopeSummary::empty())); | |||
There was a problem hiding this comment.
You should be able to get rid of the event_category variable as a result of this, and use envelope_summary in the or_else error handler.
|
Integration tests are updated. Earlier failures in @jan-auer I think this is good to merge. 🤞 I've addressed all your inline comments and the behavior is correct AFAICT, but I await another check from you. I also took a pass at adding a changelog entry but I'm concerned the phrasing might be slightly wrong; please revise if so. |
| @@ -464,11 +464,15 @@ def test_minidump_ratelimit( | |||
|
|
|||
| # First minidump returns 200 but is rate limited in processing | |||
| relay.send_minidump(project_id=project_id, files=attachments) | |||
| outcomes_consumer.assert_rate_limited("static_disabled_quota", category="error") | |||
| outcomes_consumer.assert_rate_limited( | |||
| "static_disabled_quota", categories=["error", "attachment"] | |||
There was a problem hiding this comment.
This test unveils that there is a mismatch between what EnvelopeLimiter does and how we generate outcomes. I'll follow up with a more detailed description.
There was a problem hiding this comment.
We will go with the current state and fine-tune this in the next stage.
RyanSkonnord
changed the title
* master: feat(stats): Add quantity to TrackOutcome and new attachment outcomes (#942)
Add a quantity attribute to outcomes. For attachments, the quantity is
the total attachment size in bytes. For all other categories, the
quantity is the event count (by default, 1).
If a rejected envelope has attachments, emit a second outcome with the
attachment category in addition to the event outcome.