feat(client-reports): Protocol support for client reports #1074
Conversation
|
This assumption is likely very wrong now: fn started(&mut self, context: &mut Self::Context) {
// Set the mailbox size to the size of the envelope buffer. This is a rough estimate but
// should ensure that we're not dropping outcomes unintentionally.
let mailbox_size = self.config.envelope_buffer_size() as usize;
context.set_mailbox_capacity(mailbox_size); |
| @@ -1451,6 +1454,11 @@ impl Config { | |||
| self.values.limits.max_attachments_size.as_bytes() | |||
| } | |||
|
|
|||
| /// Returns the maxmium combined size of client reports in bytes. | |||
There was a problem hiding this comment.
Not sure if we want this config flag, but if we do, we probably need to document it.
| @@ -84,7 +84,7 @@ pub struct TrackOutcome { | |||
| /// The event's data category. | |||
| pub category: DataCategory, | |||
| /// The number of events or total attachment size in bytes. | |||
| pub quantity: usize, | |||
| pub quantity: u32, | |||
There was a problem hiding this comment.
I'm changing this to u32 because we do not actually permit anything larger than a u32 on the kafka topic or it will fatally fail in the consumer.
| quantity: limit.quantity, | ||
| // XXX: on the limiter we have quantity of usize, but in the protocol | ||
| // and data store we're limited to u32. | ||
| quantity: limit.quantity as u32, |
There was a problem hiding this comment.
Since I only changed it on the protocol to a u32 we have a few of these now which are just suboptimal casts.
There was a problem hiding this comment.
I have some concerns about how DataCategory=0 is redefined and also wonder why you define it that way in Relay if it's never going to be emitted or consumed over the wire from/to Relay.
I also am scared of building something here that fundamentally cannot be rate-limited. We can have a killswitch in Relay to drop those client reports, but they have to pass through Relay to be detected, can't be dropped on LB and SDKs will not honor 429 on them even if we decide to emit any, as far as I understand.
There was a problem hiding this comment.
I think my comment about validation being in internal/processing Relays vs Snuba still applies. Again I would like us to consider doing this validation in Relay, we can still have the same exact validation we have in Snuba running in processing relays exclusively.
We have the same pattern for most kinds of data validation, and where we don't (enums in eg event schema) we actually know it to be a problem.
We discussed some stuff on the call about truncation and there was the valid argument that all relays will still have to truncate values, but I think that's fine and doesn't change anything here.
)" This reverts commit bf9710d.
This adds support for client reports from the SDKs. This feature is primarily used to allow client SDKs to emit outcomes.
The way this is currently implemented is that we add a new envelope item type called
client_reportwhich contains these outcomes. The envelope item type is intentionally kept flexible for future expansion as we already discussed emitting other internal debug information upstream which is unrelated to outcomes (eg: number of breadcrumbs discarded and configuration errors).Because of how SDKs are currently implemented we now reserve
internalas data category for such metric items. Relay however will never emit such a data category as we do not want to communicate actual rate limits for such envelope items.From the relay perspective any outcome reason that an SDK emits is acceptable and upstreamed. It's up to the snuba consumer to whitelist these outcomes to avoid writing bad data into the kafka topic. This way SDKs can send more outcomes even if relays have not been updated.
Refs INGEST-343