metrics(processor): emit counter for feedback screenshots #3603
Conversation
aliu39
changed the title
There was a problem hiding this comment.
The metric should probably live in the processor not in the store, in a feedback specific code path which has access to the entire envelope.
The information you want to be tracking here is way to high cardinality.
Note: as an informative metric adding this to Relay is fine, but building processes etc. on top is a bad idea.
Wdym by this? |
|
Informational yes, automatically including it in reports and building other processes ontop of the metric. |
There was a problem hiding this comment.
The only alternative is to add a field to the msg payload, which we'd prefer not to do.
-
Why not actually?
-
Do you want to measure this number before or after quotas are applied? If before, moving this metric to the envelope processor would make sense.
-
It probably makes sense to also count feedback items without attachments, i.e. "measure baseline". Could be a single metric tagged by
has_attachment: true/false.
relay-server/src/services/store.rs
Outdated
| @@ -347,6 +349,10 @@ impl StoreService { | |||
| } | |||
| } | |||
|
|
|||
| if has_feedback && !attachments.is_empty() { | |||
| metric!(counter(RelayCounters::FeedbackAttachments) += attachments.len() as i64); | |||
There was a problem hiding this comment.
What exactly do we want to count here, the number of feedback envelopes that have an attachment, or the total number of attachments? Can a single feedback envelope contain multiple screenshots?
There was a problem hiding this comment.
Thanks so much for the considerations! I'm thinking it might be better to add a metadata field to the payload instead. Will close and come back to this if not
There was a problem hiding this comment.
What exactly do we want to count here, the number of feedback envelopes that have an attachment, or the total number of attachments? Can a single feedback envelope contain multiple screenshots?
We'd just be counting the total number of attachments for now. Currently we only support 1 ss/attachment per feedback, don't expect this to change for a while
It probably makes sense to also count feedback items without attachments, i.e. "measure baseline". Could be a single metric tagged by has_attachment: true/false.
Atm we're using the event.produced counter, filter by topic, for this
Co-authored-by: Joris Bayer <joris.bayer@sentry.io>
What processes are we building on top of it? For context: We'd like to monitor usage of it |
| @@ -720,6 +723,8 @@ impl CounterMetric for RelayCounters { | |||
| #[cfg(feature = "processing")] | |||
| RelayCounters::TransactionsFromSpans => "transactions_from_spans", | |||
| RelayCounters::MissingDynamicSamplingContext => "missing_dynamic_sampling_context", | |||
| #[cfg(feature = "processing")] | |||
| RelayCounters::FeedbackAttachments => "processing.feedback_attachments", | |||
aliu39
changed the title
aliu39
changed the title
| @@ -420,6 +420,26 @@ pub fn has_unprintable_fields(event: &Annotated<Event>) -> bool { | |||
| } | |||
| } | |||
|
|
|||
| //TODO: comment | |||
| //TODO: add test: all incr/no incr cases, envelope unchanged | |||
| pub fn emit_feedback_metrics(envelope: &Envelope) -> i64 { | |||
There was a problem hiding this comment.
Why does this function return the count? To make it testable?
There was a problem hiding this comment.
Removed the return value since we're ok with no tests
|
@aliu3ntry you only made the call to the function conditional on the feature, but not the function itself (it still exists so all it references also must exist). I pushed a commit which removes all of the conditionals, don't think this is necessary here. |
|
Ok - thanks! |
Follow up from #3403
A useful metric for our product team to gauge the success of user feedback screenshots, after the new release of Javascript SDK v8. Unfortunately we have no way to track this in the Sentry backend, since the two event types split into separate pipelines after Relay. The only alternative is to add a field to the msg payload, which we'd prefer not to do.
6/3 update: Bruno spoke w/SDK team and they thought it made more sense to emit the metric here. We want to monitor the num of attachments sent in the same envelope as feedback (aka user report v2).
#skip-changelog