Skip to content

feat(slack): notify on disable analytics #54421

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

Merged
merged 8 commits into from
Aug 9, 2023

Conversation

chloeho7
Copy link
Contributor

@chloeho7 chloeho7 commented Aug 8, 2023

Analytics for #53621 and #53533
Closes #53568
Adding to amplitude here: https://github.com/getsentry/etl/pull/1167

@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Aug 8, 2023
@chloeho7 chloeho7 marked this pull request as ready for review August 8, 2023 21:28
@chloeho7 chloeho7 requested a review from a team as a code owner August 8, 2023 21:28
@chloeho7 chloeho7 requested a review from scefali August 8, 2023 21:38

attributes = (
analytics.Attribute("organization_id"),
analytics.Attribute("redis_key"),
Copy link
Member

Choose a reason for hiding this comment

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

Do we need the redis key? If we have the provider and org ID that should be enough for us to tell which integration is disabled and for whom.

type = "integration.disabled.notified"

attributes = (
analytics.Attribute("organization_id"),
Copy link
Contributor

Choose a reason for hiding this comment

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

@chloeho7 can we add a user id so we can send it to Amplitude? It can be the default owner id.

@codecov
Copy link

codecov bot commented Aug 8, 2023

Codecov Report

Merging #54421 (101d7f1) into master (e46ca23) will increase coverage by 0.00%.
Report is 30 commits behind head on master.
The diff coverage is 100.00%.

❗ Current head 101d7f1 differs from pull request most recent head ca168a9. Consider uploading reports for the commit ca168a9 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #54421   +/-   ##
=======================================
  Coverage   79.65%   79.66%           
=======================================
  Files        4989     4989           
  Lines      211491   211571   +80     
  Branches    36048    36061   +13     
=======================================
+ Hits       168471   168542   +71     
- Misses      37834    37840    +6     
- Partials     5186     5189    +3     
Files Changed Coverage Δ
src/sentry/monitors/consumers/monitor_consumer.py 88.88% <ø> (-0.05%) ⬇️
static/app/components/datePageFilter.tsx 94.44% <ø> (-0.30%) ⬇️
...mponents/organizations/pageFilters/persistence.tsx 70.96% <ø> (-0.91%) ⬇️
...mponents/organizations/timeRangeSelector/index.tsx 89.38% <ø> (ø)
static/app/views/issueDetails/groupDetails.tsx 66.66% <ø> (ø)
...entry/api/endpoints/organization_member/details.py 88.81% <100.00%> (+0.23%) ⬆️
...api/serializers/models/organization_member/base.py 98.41% <100.00%> (+0.07%) ⬆️
src/sentry/integrations/analytics.py 100.00% <100.00%> (ø)
src/sentry/integrations/notify_disable.py 79.16% <100.00%> (+1.89%) ⬆️
src/sentry/tasks/weekly_reports.py 77.44% <100.00%> (ø)

... and 25 files with indirect coverage changes


attributes = (
analytics.Attribute("organization_id"),
analytics.Attribute("provider"),
Copy link
Contributor

Choose a reason for hiding this comment

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

@chloeho7 I think we should still have some sort of integration/installation id field and then a type. That way we can do a join on the right table based on the type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the type and parsed integration/installation id from the redis key

attributes = (
analytics.Attribute("organization_id"),
analytics.Attribute("provider"),
analytics.Attribute("user_id"),
Copy link
Contributor

Choose a reason for hiding this comment

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

@chloeho7 let's make this optional because some weird orgs don't have an owner

@chloeho7 chloeho7 enabled auto-merge (squash) August 9, 2023 16:43
@chloeho7 chloeho7 enabled auto-merge (squash) August 9, 2023 18:58
@chloeho7 chloeho7 merged commit b7df770 into master Aug 9, 2023
@chloeho7 chloeho7 deleted the chloe/analytics-slack-disable branch August 9, 2023 19:27
@github-actions github-actions bot locked and limited conversation to collaborators Aug 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add analytics for slack disable integration
3 participants