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

REPLAY-1963 Add background task coordinator #1412

Merged
merged 15 commits into from
Sep 12, 2023
Merged

REPLAY-1963 Add background task coordinator #1412

merged 15 commits into from
Sep 12, 2023

Conversation

maciejburda
Copy link
Member

@maciejburda maciejburda commented Aug 11, 2023 •
edited
Loading

What and why?

Adds BackgroundTaskCoordinator which is abstraction that allows creating UIBackgroundTask if UIKit is available.
This is utilised in DataUploadWorker to register background tasks for upload works.

With this change we receive additional 30 seconds of upload time after app goes to background.

It's a starting point for other uploader improvements that will be explained in the RFC.

Review checklist

  • Feature or bugfix MUST have appropriate tests (unit, integration)
  • Make sure each commit and the PR mention the Issue number or JIRA reference
  • Add CHANGELOG entry for user facing changes

Custom CI job configuration (optional)

  • Run unit tests
  • Run integration tests
  • Run smoke tests

@maciejburda maciejburda marked this pull request as ready for review August 16, 2023 12:54
@maciejburda maciejburda requested a review from a team as a code owner August 16, 2023 12:54
@datadog-datadog-prod-us1
Copy link

datadog-datadog-prod-us1 bot commented Aug 16, 2023 •
edited
Loading

Datadog Report

Branch report: maciey/REPLAY-1963-background-task-uploader
Commit report: abff044

✅ dd-sdk-ios: 0 Failed, 0 New Flaky, 118 Passed, 0 Skipped, 2m 40.45s Wall Time

ncreated
ncreated requested changes Aug 16, 2023
View reviewed changes
Copy link
Member

@ncreated ncreated left a comment

Choose a reason for hiding this comment

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

The idea of using OS' background API is IMO good 👍. I noted few problems with the implementation.

@maciejburda maciejburda force-pushed the maciey/REPLAY-1963-background-task-uploader branch from 168cb7f to 7972ba6 Compare August 17, 2023 15:03
@maciejburda maciejburda requested a review from ncreated August 17, 2023 15:33
@maciejburda maciejburda force-pushed the maciey/REPLAY-1963-background-task-uploader branch 2 times, most recently from 25b8f89 to abff044 Compare August 21, 2023 09:00
ganeshnj
ganeshnj reviewed Aug 23, 2023
View reviewed changes
Copy link
Contributor

@ganeshnj ganeshnj left a comment

Choose a reason for hiding this comment

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

Looks good, just a question on cleanup.

maxep
maxep reviewed Aug 23, 2023
View reviewed changes
Copy link
Member

@maxep maxep left a comment

Choose a reason for hiding this comment

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

Nice work, it's simple and well abstracted 👏
I left some minor comments/suggestions

@maciejburda maciejburda force-pushed the maciey/REPLAY-1963-background-task-uploader branch from abff044 to 51231a1 Compare August 24, 2023 10:54
ncreated
ncreated previously approved these changes Aug 24, 2023
View reviewed changes
Copy link
Member

@ncreated ncreated left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks ok 👍.

What about observability of batches that are sent (deleted) while the app is in background? The metrics we added recently include is_background attribute that was made for this. Do we have task to cover it?

@maciejburda maciejburda force-pushed the maciey/REPLAY-1963-background-task-uploader branch from 51231a1 to ca19162 Compare August 25, 2023 09:52
@datadog-datadog-prod-us1
Copy link

datadog-datadog-prod-us1 bot commented Aug 25, 2023 •
edited
Loading

Datadog Report

Branch report: maciey/REPLAY-1963-background-task-uploader
Commit report: b536b3e

✅ dd-sdk-ios: 0 Failed, 0 New Flaky, 118 Passed, 0 Skipped, 2m 49.08s Wall Time

@maciejburda maciejburda force-pushed the maciey/REPLAY-1963-background-task-uploader branch from b536b3e to 1075eed Compare August 28, 2023 11:08
@maciejburda maciejburda force-pushed the maciey/REPLAY-1963-background-task-uploader branch from 43bd9f4 to 79785d7 Compare September 11, 2023 11:51
ncreated
ncreated requested changes Sep 12, 2023
View reviewed changes
Copy link
Member

@ncreated ncreated left a comment

Choose a reason for hiding this comment

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

LG in overall 👌. I left few small change requests, no major blockers.

ncreated
ncreated previously approved these changes Sep 12, 2023
View reviewed changes
@maciejburda maciejburda merged commit 9dd3f69 into develop Sep 12, 2023
@maciejburda maciejburda deleted the maciey/REPLAY-1963-background-task-uploader branch September 12, 2023 12:32
This was referenced Sep 12, 2023
Merged
Merged
@maxep maxep mentioned this pull request Sep 26, 2023
Merged
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ncreated ncreated ncreated approved these changes

@maxep maxep Awaiting requested review from maxep

@ganeshnj ganeshnj Awaiting requested review from ganeshnj

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@maciejburda @ncreated @maxep @ganeshnj