feat(escalating-v2): Emit escalating metrics #54240
Conversation
github-actions
bot
added
the
Scope: Backend
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #54240 +/- ##
==========================================
+ Coverage 79.74% 79.75% +0.01%
==========================================
Files 4995 5001 +6
Lines 212184 212322 +138
Branches 36168 36188 +20
==========================================
+ Hits 169205 169339 +134
+ Misses 37772 37765 -7
- Partials 5207 5218 +11
|
|
@snigdhas let me look into these, though I'm not super familiar with Relay's tests |
|
One note in terms of the rollout of this feature, can we put it behind a feature flag? |
|
@getsentry/ingest could one of y'all help figure out the tests that are failing here? I'm not sure it's specifically related to the changes in this PR - I think the tests may need to be changed a bit to support the generic metrics pipeline. |
|
@getsentry/owners-ingest The tests are not failing now because the feature flag is not enabled, but can we have some help with the test failures? specifically https://github.com/getsentry/sentry/actions/runs/5798927319/job/15717780956? |
|
@snigdhas I think I might have managed to fix those tests. I think it might still be good to have someone from the relevant team to take a look and give this their approval I still need to merge the change in the backend option in getsentry before this can be merged / will do another pass before I approve on my end. |
|
Seems like tests are passing now. I think it's a good idea to have any relevant team(s) -- Issues, Ingest -- take a look at the way the tests have been changed and get approval from them as well before merging. |
snigdhas
force-pushed
the
snigdha/emit-metrics
branch
from
dd20295
to
d71691a
Compare
|
(Just fyi I will need to merge https://github.com/getsentry/getsentry/pull/11420 before this can be merged) |
| @@ -457,7 +461,7 @@ def test_sends_issue_notification(self, record_analytics): | |||
| assert isinstance(msg.alternatives[0][0], str) | |||
| assert "Hello world</pre>" in msg.alternatives[0][0] | |||
|
|
|||
| attachment, text = get_attachment() | |||
| attachment, text = get_attachment(1) | |||
There was a problem hiding this comment.
Do we know if the response_num needs to be passed in explicitly like this still? This was a different attempt to fix the tests. From my understanding, the passthru should be sufficient for fixing the tests and we no longer need this other change.
i.e. Do all tests still pass if we don't have response_num passed in like this?
These changes from #52774 were reverting - undoing the revert.
Update the ESCALATING_ISSUES use case during post_process as we see new events come in.