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-3284 Extension background upload #1803

Merged
merged 6 commits into from
May 8, 2024

Conversation

maciejburda
Copy link
Member

@maciejburda maciejburda commented Apr 26, 2024

What and why?

This PR makes extension targets use beginActivity(options:reason:) and endActivity(_:) APIs when backgroundTasksEnabled configuration is enabled. This allows slightly extending the lifetime of the extension in order to upload all the necessary data.

https://developer.apple.com/documentation/foundation/processinfo/1415995-beginactivity
https://developer.apple.com/documentation/foundation/processinfo/1411321-endactivity

It is recommended to use this configuration of the SDK when initialising from extensions:

Datadog.initialize(
    with: Datadog.Configuration(
        clientToken: clientToken,
        env: environment,
        site: .us1,
        backgroundTasksEnabled: true // This will give more time for extensions to upload SDK data
    ),
    trackingConsent: .granted,
    instanceName: "UniqueExtensionName" // This line is important when using the SDK in multiple extensions
)

How?

This PR adds extension compatible mechanism for background upload as well as mentions the recommended configuration setup when using the SDK from within extensions. Currently adopted mechanism only works from app targets.

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/

@datadog-datadog-prod-us1
Copy link

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

Datadog Report

Branch report: maciey/RUM-3284-widget-crash-prevention
Commit report: a14f506
Test service: dd-sdk-ios

✅ 0 Failed, 3044 Passed, 0 Skipped, 3m 57.51s Total Time
🔻 Test Sessions change in coverage: 10 decreased, 4 increased

🔻 Code Coverage Decreases vs Default Branch (10)

This report shows up to 5 code coverage decreases.

  • test DatadogCoreTests tvOS 79.26% (-0.37%) - Details
  • test DatadogLogsTests tvOS 45.1% (-0.27%) - Details
  • test DatadogLogsTests iOS 45.05% (-0.27%) - Details
  • test DatadogRUMTests tvOS 80.84% (-0.19%) - Details
  • test DatadogCrashReportingTests tvOS 27.72% (-0.18%) - Details

@maciejburda maciejburda marked this pull request as ready for review April 26, 2024 16:05
@maciejburda maciejburda requested review from a team as code owners April 26, 2024 16:05
fuzzybinary
fuzzybinary previously approved these changes Apr 29, 2024
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.

I have couple of question on the approach here.

  • When a batch is written - there can only be one and only one writer, because the way we name it. Once the file is written to the disk it is immutable.
    • in this process, the lock must be on the file, not on the directory, if we need.
  • Now reading the file from multiple sources must not make a difference.

While working on a personal project, I realized the programming model of widgets is quite different from lifecycle point of view. Hence I wonder if it even makes sense to upload batches from extension itself.

https://developer.apple.com/documentation/widgetkit/making-network-requests-in-a-widget-extension

@maciejburda maciejburda force-pushed the maciey/RUM-3284-widget-crash-prevention branch from 6f17a40 to 2ae71ba Compare May 6, 2024 15:13
@maciejburda
Copy link
Member Author

maciejburda commented May 6, 2024

I have couple of question on the approach here.

  • When a batch is written - there can only be one and only one writer, because the way we name it. Once the file is written to the disk it is immutable.

    • in this process, the lock must be on the file, not on the directory, if we need.
  • Now reading the file from multiple sources must not make a difference.

While working on a personal project, I realized the programming model of widgets is quite different from lifecycle point of view. Hence I wonder if it even makes sense to upload batches from extension itself.

https://developer.apple.com/documentation/widgetkit/making-network-requests-in-a-widget-extension

Problem is we don't have access to app's directory from the extension. For example I get path like this:
App:

file:///Users/maciej.burda/Library/Developer/CoreSimulator/Devices/7A836CF3-9C5A-4B90-BA73-76D0F1B5EB87/
data/Containers/Data/Application/93B8D5CA-40AD-4AAD-9B2F-C5F8DEBAB04E/Library/Caches/com.datadoghq/v2/f5da9c34155155fa15554148a8a2d145170e0a082ddf9cba39ea3a198b2bb947/rum/v2/

Extension:

file:///Users/maciej.burda/Library/Developer/CoreSimulator/Devices/5A69FAE7-6D74-49CC-803B-B7E64C895AB2/
data/Library/Caches/com.datadoghq/v2/f5da9c34155155fa15554148a8a2d145170e0a082ddf9cba39ea3a198b2bb947/logging/v2/

For sharing the storage we could think of using app groups for instance, but this requires bigger planning and probably an RFC.

This PR helps but doesn't fully resolves the 0xdead10cc which happens when file handle is open, while extension is being suspended. BG tasks will increase the amount of time we have to perform all the SDK work.

DatadogCore/Sources/Core/Storage/Directories.swift Outdated Show resolved Hide resolved
Comment on lines +84 to +87
internal func beginBackgroundTask() {
endBackgroundTask()
currentActivity = processInfo.beginActivity(options: [.background], reason: "Datadog SDK background upload")
}
Copy link
Member

Choose a reason for hiding this comment

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

question/ Because beginBackgroundTask() is called right before the SDK starts uploading data, how does it correspond to the explanation of 0xdead10cc?

(...) Make this request well before starting to write to the file in order to complete those operations and relinquish the lock before the app suspends.

Apple suggestion is to wrap "file write" operations with .beginActivity() and .endActivity(). What we seem to do instead is wrapping data uploads. Considering that upload starts after the file was read and it has nothing to do with file writes (which are decoupled SDK routine), I'm not seeing clearly how this solves the 0xdead10cc problem.

Shouldn't we instead use .beginActivity() and .endActivity() when writing to files, instead of when performing data upload 🤔💭?

@maciejburda maciejburda changed the title RUM-3284 Widget crash prevention RUM-3284 Extension background upload May 8, 2024
@maciejburda maciejburda force-pushed the maciey/RUM-3284-widget-crash-prevention branch from 1b1e04d to 1c2c466 Compare May 8, 2024 11:21
@maciejburda
Copy link
Member Author

As described in #1690 we ruled out some potential root causes in the SDK code.

Now, this PR is still relevant, but it acts more as a fix of background tasks for extension targets. I updated the description accordingly. Please review again :)

@maciejburda maciejburda requested a review from ncreated May 8, 2024 11:31
ncreated
ncreated previously approved these changes May 8, 2024
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.

Looks good 👌, thanks for doing an extra iteration on this 💪

@maciejburda maciejburda merged commit a0e9c69 into develop May 8, 2024
8 checks passed
@maciejburda maciejburda deleted the maciey/RUM-3284-widget-crash-prevention branch May 8, 2024 13:48
@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