fix(feedback): enforce a max message size from all sources#79326
Conversation
aliu39
changed the title
github-actions
Bot
added
the
Scope: Backend
| "feedback.large_message", | ||
| tags={ | ||
| "is_spam": is_message_spam, | ||
| "pow2_size_bucket": 2 ** math.ceil(math.log2(len(feedback_message))), |
There was a problem hiding this comment.
Limits the granularity of this tag. I'd rather do this for viewing in datadog, instead of logging each msg size.
There was a problem hiding this comment.
cant we just use a distribution metric type?
There was a problem hiding this comment.
Actually I'll add a log too so we can see the org/project id.
|
This PR has a migration; here is the generated SQL for --
-- Alter field comments on userreport
--
-- (no-op) |
… in one file + rename/comment
| register( | ||
| "feedback.message.max-size", | ||
| type=Int, | ||
| default=4096, |
There was a problem hiding this comment.
Do you need an option if the max-length is also in the schema?
There was a problem hiding this comment.
The max length schema is for legacy feedbacks only, aka user reports. User reports are shimmed to feedback issues, but not vice versa. IMO it makes sense to have a separate option for new feedback.
In the long-term it makes more sense to enforce a maximum on the upstream envelopes, but I need more time to look into that. This just plugs all holes for now, resolving that sentry issue. We can gather some metrics and have a flexible limit for now, using the option. Wdyt?
There was a problem hiding this comment.
In the long-term it makes more sense to enforce a maximum on the upstream envelopes, but I need more time to look into that. This just plugs all holes for now, resolving that sentry issue.
That makes sense to me.
aliu39
requested review from
JoshFerge and
markstory
and removed request for
JoshFerge
JoshFerge
left a comment
JoshFerge
left a comment
There was a problem hiding this comment.
some feedback: this PR could have been broken up for easier reviewing, but overall looks good. 👍🏼
Got it, thanks for lmk! |
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## master #79326 +/- ##
==========================================
+ Coverage 78.33% 78.35% +0.01%
==========================================
Files 7125 7123 -2
Lines 314677 314900 +223
Branches 51431 51464 +33
==========================================
+ Hits 246515 246739 +224
+ Misses 61698 61690 -8
- Partials 6464 6471 +7 |
Closes #76298 Closes [SENTRY-3B86](https://sentry.sentry.io/issues/5552524761/) Decided on a limit of 4096, generously below the LLM request, postgres, and kafka size limits described in the ticket. Messages that are too large will be truncated (or rejected, for crash report modal) and skip spam detection, auto-marking as spam. Includes a small refactor of `create_feedback.py`, moving stuff around and commenting a bit. ~~Renamed `auto_ignore_spam_feedback` to `set_feedback_ignored`.~~
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
4 participants
Closes #76298
Closes SENTRY-3B86
Decided on a limit of 4096, generously below the LLM request, postgres, and kafka size limits described in the ticket. Messages that are too large will be truncated (or rejected, for crash report modal) and skip spam detection, auto-marking as spam.
Includes a small refactor of
create_feedback.py, moving stuff around and commenting a bit.Renamedauto_ignore_spam_feedbacktoset_feedback_ignored.