Skip to content
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(slack): Show culprit in notifications #72980

Merged
merged 13 commits into from
Jun 26, 2024
Merged

Conversation

iamrajjoshi
Copy link
Member

Just wrapping @olagjo's PR #72587 around a FF so we can quickly turn it on/off.

olagjo and others added 2 commits June 13, 2024 17:34
Relates to/part of #30164 (which has absorbed #26006).
This adds a context-like culprit-block directly under the title,
which mirrors how the culprit is presented in the web UI.
Copy link

sentry-io bot commented Jun 18, 2024

🔍 Existing Issues For Review

Your pull request is modifying functions with the following pre-existing issues:

📄 File: src/sentry/integrations/slack/message_builder/issues.py

Function Unhandled Issue
build TypeError: 'NoneType' object cannot be interpreted as an integer sentry.tasks.activity.send_activi...
Event Count: 7
build Actor.InvalidActor: Cannot find a User with id=2493315 sentry.tasks.digests.deliver_...
Event Count: 3

Did you find this useful? React with a 👍 or 👎

@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Jun 18, 2024
@iamrajjoshi iamrajjoshi self-assigned this Jun 18, 2024
@iamrajjoshi iamrajjoshi requested a review from a team June 18, 2024 23:06
@iamrajjoshi iamrajjoshi marked this pull request as ready for review June 18, 2024 23:06
@iamrajjoshi iamrajjoshi requested a review from a team as a code owner June 18, 2024 23:06
Copy link

codecov bot commented Jun 18, 2024

Codecov Report

Attention: Patch coverage is 85.71429% with 3 lines in your changes missing coverage. Please review.

Project coverage is 78.04%. Comparing base (a741388) to head (b95b736).

Current head b95b736 differs from pull request most recent head e0c0027

Please upload reports for the commit e0c0027 to get more accurate results.

Additional details and impacted files
@@             Coverage Diff             @@
##           master   #72980       +/-   ##
===========================================
+ Coverage   46.59%   78.04%   +31.45%     
===========================================
  Files        1873     6624     +4751     
  Lines      120667   295748   +175081     
  Branches    21721    50943    +29222     
===========================================
+ Hits        56224   230831   +174607     
+ Misses      63382    58621     -4761     
- Partials     1061     6296     +5235     
Files Coverage Δ
src/sentry/features/temporary.py 100.00% <100.00%> (ø)
src/sentry/testutils/cases.py 85.76% <92.30%> (+54.86%) ⬆️
...entry/integrations/slack/message_builder/issues.py 85.39% <71.42%> (+67.10%) ⬆️

... and 6017 files with indirect coverage changes

iamrajjoshi and others added 2 commits June 20, 2024 22:36
Co-authored-by: Yash Kamothi <yash.kamothi@gmail.com>
Copy link
Member

@cathteng cathteng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we keep testing for slack messages without culprit? right now it seems like we've removed coverage for no culprit messages

src/sentry/testutils/cases.py Outdated Show resolved Hide resolved
Co-authored-by: Cathy Teng <70817427+cathteng@users.noreply.github.com>
@olagjo
Copy link
Contributor

olagjo commented Jun 25, 2024

should we keep testing for slack messages without culprit? right now it seems like we've removed coverage for no culprit messages

There are still tests where there are no culprit blocks – however, in all of them it's currently because the feature flag is off.
We can turn the feature flag on on e.g.:
tests/sentry/integrations/slack/test_message_builder.py::BuildGroupAttachmentReplaysTest::test_build_replay_issue
Locally for me, this test still passes without outputting a culprit block, even with the feature flag on (and this should hopefully fix the coverage as well!)

@iamrajjoshi iamrajjoshi enabled auto-merge (squash) June 26, 2024 20:20
@iamrajjoshi iamrajjoshi merged commit d13db8c into master Jun 26, 2024
47 checks passed
@iamrajjoshi iamrajjoshi deleted the culprit-in-slack branch June 26, 2024 20:57
@olagjo
Copy link
Contributor

olagjo commented Jun 27, 2024

Yay!
Can we get this enabled on our organization hyreas? :D

@iamrajjoshi
Copy link
Member Author

Yay! Can we get this enabled on our organization hyreas? :D

i added the org in the LA group!

Copy link

sentry-io bot commented Jun 30, 2024

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

  • ‼️ KeyError: 'event_metadata' pytest.runtest.protocol tests/sentry/integratio... View Issue

Did you find this useful? React with a 👍 or 👎

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants