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.

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

RUM-3574 perf: Start sending batches immediately after feature is initialized #1798

Merged

Conversation

ncreated
Copy link
Member

@ncreated ncreated commented Apr 25, 2024

What and why?

📦⏱️ This PR removes the initial delay for sending batches. Now, first upload will be triggered soon after feature is initialized.

The motivation is to maximise a chance of sending data if application is impacted by early crash.
Related to DataDog/dd-sdk-flutter#573

How?

Removing initial delay from DataUploadWorker.

Before:

----[I]------------[U1]----------------[U2]-->
     ^ SDK init
                    ^ initial upload
                                         ^ next upload

Now:

----[I][U1]----------------[U2]-->
     ^ SDK init
        ^ initial upload
                             ^ next upload

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 for Core, RUM, Trace, Logs, CR and WVT
  • Run unit tests for Session Replay
  • Run integration tests
  • Run smoke tests
  • Run tests for tools/

@ncreated ncreated force-pushed the ncreated/RUM-3574/start-sending-batches-on-sdk-init branch from 25e7c0c to 7afd568 Compare April 25, 2024 12:59
@ncreated ncreated marked this pull request as ready for review April 25, 2024 13:00
@ncreated ncreated requested review from a team as code owners April 25, 2024 13:00
@datadog-datadog-prod-us1
Copy link

datadog-datadog-prod-us1 bot commented Apr 25, 2024

Datadog Report

Branch report: ncreated/RUM-3574/start-sending-batches-on-sdk-init
Commit report: 1f60725
Test service: dd-sdk-ios

✅ 0 Failed, 2767 Passed, 0 Skipped, 6m 34.99s Wall Time
🔻 Test Sessions change in coverage: 8 decreased, 5 increased

🔻 Code Coverage Decreases vs Default Branch (8)

This report shows up to 5 code coverage decreases.

  • test DatadogCoreTests tvOS 79.31% (-0.31%) - Details
  • test DatadogLogsTests tvOS 45.09% (-0.28%) - Details
  • test DatadogLogsTests iOS 45.04% (-0.28%) - Details
  • test DatadogCrashReportingTests iOS 27.68% (-0.2%) - Details
  • test DatadogRUMTests tvOS 80.83% (-0.2%) - Details

@ncreated ncreated self-assigned this Apr 25, 2024
@ncreated ncreated marked this pull request as draft April 25, 2024 15:47
@ncreated
Copy link
Member Author

Back to draft as the CI got flaky on BG tasks tests in DataUploadWorker.

by ensuring there is data available before worker is initialized. This is
because we now start sending data soon after worker init.
@ncreated ncreated marked this pull request as ready for review April 25, 2024 17:55
maciejburda
maciejburda previously approved these changes Apr 26, 2024
@ganeshnj
Copy link
Contributor

I'm curious about the impact when a notification triggers app load and we will send anything pending all at the same time.

@ncreated
Copy link
Member Author

ncreated commented May 6, 2024

I'm curious about the impact when a notification triggers app load and we will send anything pending all at the same time.

As we discussed, we only change the delay of initial upload, so practically there won't be a change to the load spike in this scenario - it will be only shifted earlier in time.

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.

👍

@ncreated ncreated merged commit 0a9bfae into develop May 7, 2024
6 checks passed
@maciejburda maciejburda mentioned this pull request May 8, 2024
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants