Skip to content

ref(weekly reports): Rearrange files & functions to be reused#65559

Merged
ceorourke merged 6 commits intomasterfrom
ceorourke/reorganize-weekly-reports
Feb 22, 2024
Merged

ref(weekly reports): Rearrange files & functions to be reused#65559
ceorourke merged 6 commits intomasterfrom
ceorourke/reorganize-weekly-reports

Conversation

@ceorourke
Copy link
Copy Markdown
Member

Offshoot of #65501 that moves weekly_reports.py into a summaries directory and puts classes and functions to be reused by the daily summary into a utils file.

@codecov
Copy link
Copy Markdown

codecov bot commented Feb 21, 2024

Codecov Report

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

Comparison is base (b1d4c07) 84.16% compared to head (8afdd60) 84.17%.
Report is 8 commits behind head on master.

❗ Current head 8afdd60 differs from pull request most recent head 799f629. Consider uploading reports for the commit 799f629 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #65559   +/-   ##
=======================================
  Coverage   84.16%   84.17%           
=======================================
  Files        5267     5268    +1     
  Lines      235578   235569    -9     
  Branches    40778    40769    -9     
=======================================
- Hits       198285   198282    -3     
+ Misses      37074    37068    -6     
  Partials      219      219           
Files Coverage Δ
src/sentry/conf/server.py 90.00% <ø> (ø)
src/sentry/utils/sdk.py 79.19% <ø> (ø)
...c/sentry/web/frontend/debug/debug_weekly_report.py 36.53% <100.00%> (+2.57%) ⬆️
src/sentry/tasks/summaries/weekly_reports.py 92.41% <96.66%> (ø)
src/sentry/tasks/summaries/utils.py 81.53% <81.53%> (ø)

... and 6 files with indirect coverage changes

@ceorourke
Copy link
Copy Markdown
Member Author

Opened https://github.com/getsentry/getsentry/pull/13021 to address the getsentry failures

Copy link
Copy Markdown
Member

@wedamija wedamija left a comment

Choose a reason for hiding this comment

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

I don't think this can pass in getsentry. You might need some temporary code in place to handle the refactor as part of https://github.com/getsentry/getsentry/pull/13021/files

Something like

try: 
    from sentry.tasks.summaries.weekly_reports import prepare_organization_report
except ImportError:
    # import old path

Copy link
Copy Markdown
Contributor

@nhsiehgit nhsiehgit left a comment

Choose a reason for hiding this comment

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

nice!

Ok - so mostly moving things around, except for project_key_errors and project_key_performance_issues which now return values rather than modifying in place.

added a quick comment, but can we also do that for project_key_transactions?

"project_key_errors.results",
extra={"project_id": project.id, "num_key_errors": len(key_errors)},
)
project_key_transactions(ctx, project)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this one still modifying in place?
can we update to return also?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It is - I didn't update it because I don't have plans to use this function again, but I can for consistency.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Updated - I had to break it out into 2 functions FYI

Copy link
Copy Markdown
Contributor

@nhsiehgit nhsiehgit left a comment

Choose a reason for hiding this comment

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

nice 🎉

mmm...

Maybe in the future i would also break this into separate PRs too

one purely a move things around (since there's a lot of line changes)
and another to actually modify the behavior

@ceorourke ceorourke merged commit 2b68c6d into master Feb 22, 2024
@ceorourke ceorourke deleted the ceorourke/reorganize-weekly-reports branch February 22, 2024 00:33
snigdhas pushed a commit that referenced this pull request Feb 22, 2024
Offshoot of #65501 that moves
`weekly_reports.py` into a `summaries` directory and puts classes and
functions to be reused by the daily summary into a utils file.
@sentry
Copy link
Copy Markdown
Contributor

sentry bot commented Feb 23, 2024

Suspect Issues

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

  • ‼️ OperationalError: QueryCanceled('canceling statement due to user request\n') sentry.tasks.summaries.weekly_reports.prepare_o... View Issue

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

ceorourke added a commit that referenced this pull request Feb 27, 2024
2nd chunk of #65501 now that the
files have been rearranged in
#65559

This PR reuses a lot of the weekly report functions to get per project
data on the current day's event count vs. 14 day average (I have ideas
for optimizing this, but for the internal MVP it isn't necessary), the
day's top 3 error and performance issues (by event count), issues that
escalated or regressed that day, and today's releases and new issues in
those releases.

A future PR will use this data and build a Slack notification.
@github-actions github-actions bot locked and limited conversation to collaborators Mar 12, 2024
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.

3 participants