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.

Sign up for GitHub

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

Jump to bottom

Use batch requests for daily summary job #72

Merged
merged 8 commits into from
Apr 13, 2020
Merged

Use batch requests for daily summary job #72

merged 8 commits into from
Apr 13, 2020

Conversation

mickmister
Copy link
Contributor

Summary

The daily summary job now uses msgraph JSON batching to limit the number of requests made. Order of operations:

  • Fetch DailySummaryIndex from store
  • Filter out users who should not receive their summary
  • Perform API requests with 20 users per request
  • Send each user their summary

Ticket Link

Fixes #48

@mickmister mickmister added 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels Apr 2, 2020
@mickmister mickmister requested review from levb and larkox April 2, 2020 05:54
@codecov-io
Copy link

codecov-io commented Apr 2, 2020 •
edited
Loading

Codecov Report

Merging #72 into master will increase coverage by 1.60%.
The diff coverage is 36.36%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #72      +/-   ##
==========================================
+ Coverage   26.20%   27.80%   +1.60%     
==========================================
  Files          57       57              
  Lines        2015     2057      +42     
==========================================
+ Hits          528      572      +44     
+ Misses       1432     1420      -12     
- Partials       55       65      +10
Impacted Files Coverage Δ
server/command/daily_summary.go 0.00% <0.00%> (ø)
server/jobs/daily_summary_job.go 0.00% <0.00%> (ø)
server/mscalendar/calendar.go 0.00% <0.00%> (ø)
server/remote/msgraph/get_default_calendar_view.go 0.00% <0.00%> (ø)
server/utils/tz/conversion.go 0.00% <0.00%> (ø)
server/mscalendar/availability.go 53.84% <25.00%> (-2.79%) ⬇️
server/mscalendar/daily_summary.go 41.30% <71.73%> (+27.91%) ⬆️
server/remote/msgraph/get_schedule_batched.go 51.66% <100.00%> (ø)
server/mscalendar/filters.go 42.50% <0.00%> (ø)
server/mscalendar/user.go 20.00% <0.00%> (+2.10%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d09debb...c75cbb2. Read the comment docs.

larkox
larkox reviewed Apr 2, 2020
View reviewed changes
server/command/daily_summary.go Outdated Show resolved Hide resolved
server/mscalendar/daily_summary_job.go Outdated Show resolved Hide resolved
server/mscalendar/daily_summary.go Outdated Show resolved Hide resolved
server/mscalendar/daily_summary.go Outdated Show resolved Hide resolved
@mickmister
PR feedback:
f17d273 
Remove '"summary all" command
remove one-liner function call
fix error message return
@mickmister mickmister requested a review from larkox April 6, 2020 13:14
larkox
larkox previously approved these changes Apr 6, 2020
View reviewed changes
Copy link
Contributor

@larkox larkox left a comment

Choose a reason for hiding this comment

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

LGTM

levb
levb previously approved these changes Apr 8, 2020
View reviewed changes
@levb levb removed the 2: Dev Review Requires review by a core committer label Apr 8, 2020
@levb levb requested a review from DHaussermann April 8, 2020 22:17
DHaussermann
DHaussermann previously approved these changes Apr 10, 2020
View reviewed changes
Copy link

@DHaussermann DHaussermann left a comment

Choose a reason for hiding this comment

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

Thanks @mickmister Please merge this PR.

I will do regression testing around this along with #74 in master.

@DHaussermann DHaussermann added 4: Reviews Complete All reviewers have approved the pull request QA Review Done PR has been approved by QA and removed 3: QA Review Requires review by a QA tester labels Apr 10, 2020
@levb
Copy link
Contributor

levb commented Apr 11, 2020

@mickmister Can you please resolve the conflict?

@mickmister mickmister dismissed stale reviews from DHaussermann and levb via 0461ccb April 12, 2020 02:22
@mickmister mickmister added the AutoMerge Used by Mattermod to merge PR automatically label Apr 12, 2020
@mattermod
Copy link
Contributor

Will try to auto merge this PR once all tests and checks are passing. This might take up to an hour.

@mickmister mickmister requested a review from levb April 12, 2020 04:33
@mickmister mickmister removed the AutoMerge Used by Mattermod to merge PR automatically label Apr 13, 2020
levb
levb approved these changes Apr 13, 2020
View reviewed changes
server/jobs/daily_summary_job.go
env.Logger.Debugf("Daily summary job beginning")

err = mscal.ProcessAllDailySummary()
err := mscalendar.New(env, "").ProcessAllDailySummary(time.Now())
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: should this be BotUserID?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@levb Yes it is implicitly BotUserID. The New function falls back to the BotUserID when an empty string is passed in. It's convenient but I think I prefer the explicit way. Leaving it as is, as we have done this elsewhere and we this is already QA reviewed etc

@levb levb added this to the 0.1.0 milestone Apr 13, 2020
@mickmister mickmister merged commit fdd0a1f into master Apr 13, 2020
@mickmister mickmister deleted the daily-summary-batch branch April 13, 2020 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@levb levb levb approved these changes

@DHaussermann DHaussermann DHaussermann left review comments

@larkox larkox larkox left review comments

Assignees
No one assigned
Labels
4: Reviews Complete All reviewers have approved the pull request QA Review Done PR has been approved by QA
Projects
None yet
Milestone
v1.0.0
Development

Successfully merging this pull request may close these issues.

Use batch requests in the Daily Summary job to improve performance
6 participants
@mickmister @codecov-io @levb @mattermod @larkox @DHaussermann