sentry icon indicating copy to clipboard operation
sentry copied to clipboard

feat(integration): Give each action a UUID

Open ykamo001 opened this issue 1 year ago • 3 comments

Give each Rule Action a UUID so it's identifiable in future work. Currently we only store related information about what the action should do, but not how to differentiate it uniquely across the platform between one another. For example, a singular Rule could have 3 actions, each one describing that it should be a Slack notification to a different channel. To differentiate, you would have to check the channel id against each other. This is even harder when you need to compare against other rule action types (Discord, email, etc). To understand which action has what side effect, this will get us closer to that visibility.

Why is this in the serializer?

We use the validated data to create and update actions, which are subsets of a Rule. This makes sure all locations where we need validated data are going to get a UUID. We could optionally move this to the "save" method in the Rule model, but felt like this is not the responsibility of the Rule model to ensure.

Resolves: https://github.com/getsentry/sentry/issues/64916

ykamo001 avatar Feb 08 '24 23:02 ykamo001

Codecov Report

Attention: 10 lines in your changes are missing coverage. Please review.

Comparison is base (5516f1a) 81.38% compared to head (acc1ab3) 81.40%. Report is 60 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #64922      +/-   ##
==========================================
+ Coverage   81.38%   81.40%   +0.01%     
==========================================
  Files        5243     5252       +9     
  Lines      231458   232279     +821     
  Branches    45417    45611     +194     
==========================================
+ Hits       188372   189078     +706     
- Misses      37231    37310      +79     
- Partials     5855     5891      +36     
Files Coverage Δ
src/sentry/api/serializers/rest_framework/rule.py 72.41% <100.00%> (+3.18%) :arrow_up:
src/sentry/testutils/cases.py 85.95% <ø> (+0.10%) :arrow_up:
src/sentry/api/endpoints/project_rules.py 94.33% <89.69%> (-1.64%) :arrow_down:

... and 53 files with indirect coverage changes

codecov[bot] avatar Feb 08 '24 23:02 codecov[bot]

Changes to add uuid to action makes sense to me. Please add a test for the bug you're fixing.

We're not going to fix the bug in this change, as the side effects and lift to fix it is going to be a lot (in terms of updating all the tests and potentially fixing UI flows). We identified where the bug is, and how to fix it, but will tackle it in this ticket later: https://github.com/getsentry/sentry/issues/65028

ykamo001 avatar Feb 12 '24 20:02 ykamo001

Got it. Can you help me understand the value that the refactory provides here?

sentaur-athena avatar Feb 12 '24 21:02 sentaur-athena

Got it. Can you help me understand the value that the refactory provides here?

Discussed this offline

ykamo001 avatar Feb 12 '24 22:02 ykamo001

You will also need to make sure that this is handled correctly during import. We used to follow this procedure here: https://www.notion.so/sentry/On-the-Subject-of-Self-Hosted-Migrations-2433c8da9ec14775b144945853294a3e?pvs=4#0ce1fd4efa4c4ccfbdacd617cb5b813f

Not sure if it's still the process, but if it is, then you'll need to add this uuid generation to https://github.com/getsentry/getsentry/blob/2d08bc5967e2b9125185e0eb05a5910804909e59/bin/issue-alert-actions-migrate.py#L99

I think self-hosted has done some work around this process, so not sure if things have changed since I last looked into it

Good eye, thank you for the call out! Added that generation and work to the import side just in case: https://github.com/getsentry/getsentry/pull/12918/files#diff-6e4fb98a0c4c3c943a829cd081d206611293eda7ff7c7951653486cce51cb20fR36

ykamo001 avatar Feb 13 '24 00:02 ykamo001