-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(issue summary) Only show possible cause if confident and novel #84349
base: master
Are you sure you want to change the base?
feat(issue summary) Only show possible cause if confident and novel #84349
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## master #84349 +/- ##
=======================================
Coverage 87.62% 87.62%
=======================================
Files 9601 9601
Lines 543278 543278
Branches 21286 21286
=======================================
Hits 476034 476034
Misses 66891 66891
Partials 353 353 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks fine, but wanna add a test?
title: t('Possible cause'), | ||
insight: data?.possibleCause, | ||
icon: <IconFocus size="sm" />, | ||
showWhenLoading: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@roaga double-checking that changing showWhenLoading
from true
to false
makes sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah that makes sense, but actually now that I see this, I'm not sure about the logic. You need the scores from the generated summary to determine the loading logic, but you don't have the scores until loading is done. Are you sure this works correctly when you tested it locally?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok nvm, this is fine
…ry/thresholds-possible-cause-frontend
no review notes, but this is awesome. great work |
title: t('Possible cause'), | ||
insight: data?.possibleCause, | ||
icon: <IconFocus size="sm" />, | ||
showWhenLoading: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok nvm, this is fine
Background
possible_cause_confidence
increases when there's less speculation (which correlates w/ slightly more accurate causes)possible_cause_novelty
increases when there's more novelty / less redundancy wrtwhats_wrong
thresholds led to 50% of possible causes getting dropped on our autofix sentry issues (notebook here)
Backend changes
corresponding backend change to sentry: #84346
corresponding backend change to seer: getsentry/seer#1788
without the sentry backend change (there are no
data.scores
), default to current behavior: always show possible causewith the change, only show it if both scores are greater than the threshold
(note: this example is actually considered novel and confident-enough. I hardcoded the threshold to test the behavior)