RUM-1040 Stop Core Instance #1541

maxep · 2023-11-07T12:57:30Z

What and why?

Add public interface for stopping core instance of the SDK. Stopping the core will deallocate all Features and their storage & upload units.

How?

In parity with Android, the stopInstance static method will stop and unregister all feature of the core. Their storage and upload process will be deallocated so no event-write-context and upload will execute.

Note

This change was a good opportunity to streamline flushing mechanism of the core (e.g. #1522). But the number of GDC queues involved in the core and Features made the task very tedious with high risk of introducing flakiness. I believe we should continue decreasing the number of queues, by using exclusion-queue as described in WWDC 2017 - Modernizing Grand Central Dispatch Usage, with clear and efficient queue hierarchy. We should do so after building continuous benchmark to make sure we don't decrease performances.

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 · 2023-11-07T13:11:05Z

Datadog Report

Branch report: maxep/RUM-1040/stop-core-instance
Commit report: 451d541

✅ dd-sdk-ios: 0 Failed, 0 New Flaky, 2415 Passed, 0 Skipped, 4m 44.84s Wall Time

maciejburda

LGTM

DatadogCore/Tests/Datadog/DatadogCore/DatadogCoreTests.swift

datadog-datadog-prod-us1 · 2023-11-15T11:23:51Z

Datadog Report

Branch report: maxep/RUM-1040/stop-core-instance
Commit report: a768e3e

✅ dd-sdk-ios: 0 Failed, 0 New Flaky, 2419 Passed, 0 Skipped, 4m 49.62s Wall Time

datadog-datadog-prod-us1 · 2024-01-09T19:05:31Z

Datadog Report

Branch report: maxep/RUM-1040/stop-core-instance
Commit report: 6066c10
Test service: dd-sdk-ios

✅ 0 Failed, 2545 Passed, 0 Skipped, 25m 10s Wall Time
🔻 Test Sessions change in coverage: 6 decreased, 5 increased, 3 no change

🔻 Code Coverage Decreases vs Default Branch (6)

This report shows up to 5 code coverage decreases.

test DatadogLogsTests iOS 44.74% (Δ-0.15%) - Details
test DatadogLogsTests tvOS 44.80% (Δ-0.15%) - Details
test DatadogInternalTests tvOS 81.21% (Δ-0.06%) - Details
test DatadogWebViewTrackingTests iOS 23.05% (Δ-0.02%) - Details
test DatadogCrashReportingTests tvOS 29.03% (Δ-0.02%) - Details

maxep · 2024-01-10T10:01:52Z

This PR is finally ready for review, it has increased in size due to the added integration tests.

maxep · 2024-01-10T10:04:56Z

instrumented-tests/http-server-mock/python/start_mock_server.py

+    def do_DELETE(self):
+        """
+        Routes all incoming DELETE requests
+        """
+        self.__route([
+            (r"/requests$", self.__DELETE_requests),
+        ])
+


I'm adding this endpoint to remove recorded requests so we can clear the mock server before restarting the core instance.

IntegrationTests/Runner/AppDelegate.swift

maxep · 2024-01-10T10:25:05Z

IntegrationTests/Runner/Scenarios/URLSession/URLSessionScenarios.swift

@@ -39,11 +39,6 @@ private class CompositedURLSessionDelegate: NSObject, URLSessionTaskDelegate, UR
    }
 }

-/// An example of instrumenting existing `URLSessionDelegate` with `DDURLSessionDelegate` through inheritance.
-private class CustomURLSessionDelegate: NSObject, URLSessionDataDelegate {


Moving to its own file with internal accessor for re-use in stop-core-instance test.

CHANGELOG.md

maciejburda · 2024-01-10T11:20:11Z

DatadogCore/Sources/Datadog.swift

@@ -489,13 +497,10 @@ public enum Datadog {
 #endif

    internal static func internalFlushAndDeinitialize(instanceName: String = CoreRegistry.defaultInstanceName) {
-        assert(CoreRegistry.instance(named: instanceName) is DatadogCore, "SDK must be first initialized.")


no assert anymore?

I don't remember why I removed it TBH! probably in an attempt to flush before stopping the core. Now, I don't see why we assert initialisation here 🤔

DatadogCore/Sources/Datadog.swift

ganeshnj

This looks good, can we do some testing that no objects are being leaked when the core instance is stopped. Ideally we want to have memory graph something like

     ...................
..../                   \......
 start               stop

ncreated · 2024-01-11T10:25:59Z

DatadogCore/Tests/Datadog/DatadogCore/DatadogCoreTests.swift

+        scope.eventWriteContext { context, writer in
+            writer.write(value: FeatureMock.Event(event: "should not be sent"))
+        }
+
+        core.stop()


question/ This looks flaky ❄️ , doesn't it? If it takes more than "min file age" from writing an event to stopping the core, data will be uploaded. What is the rationale here?

Indeed, I can remove the first EWC. Rationale was to assert that no upload is triggered after stopping the core, even with event in the pipe.

IntegrationTests/IntegrationScenarios/Scenarios/Core/StopCoreScenarioTests.swift

ncreated · 2024-01-11T10:47:40Z

instrumented-tests/http-server-mock/python/start_mock_server.py

+    def do_DELETE(self):
+        """
+        Routes all incoming DELETE requests
+        """
+        self.__route([
+            (r"/requests$", self.__DELETE_requests),
+        ])
+


ncreated · 2024-01-11T10:53:35Z

This looks good, can we do some testing that no objects are being leaked when the core instance is stopped. Ideally we want to have memory graph something like
     ...................
..../                   \......
 start               stop

I agree with @ganeshnj ☝️ that we must test SDK allocations. However, I don't think the stop() model proposed in this PR delivers this promise. It is rather "hard stop" implementation that only deallocates DatadogCore instance without awaiting pending asynchronous operations that may be still keeping it. Correct me if I'm wrong @maxep

maxep · 2024-01-11T15:08:44Z

@ncreated, @ganeshnj That's a good call!
It is indeed a hard stop that deallocates the core and its features, it does not wait for async operations (same in Android btw), see the note in the PR description. I will run some profiling 👍

ncreated

Well done 👌. Great work on fixing the memory profile 💪.

ncreated · 2024-01-15T08:43:52Z

DatadogCore/Sources/Datadog.swift

+        let core = CoreRegistry.unregisterInstance(named: instanceName) as? DatadogCore
+        core?.stop()


question/ Why not making stop() a requirement in DatadogCoreProtocol? I have no opinion on this (both may be fine), just wondering on what was the rationale.

The stop is only required from high level apis, there is no usage from Features so there no need for making it part of the protocol. The requirement might change in the future tho!

maxep self-assigned this Nov 7, 2023

maxep requested a review from a team as a code owner November 7, 2023 12:57

maciejburda previously approved these changes Nov 8, 2023

View reviewed changes

DatadogCore/Tests/Datadog/DatadogCore/DatadogCoreTests.swift Show resolved Hide resolved

maxep marked this pull request as draft November 13, 2023 09:34

maxep added the do-not-merge/WIP label Nov 13, 2023

maxep force-pushed the maxep/RUM-1040/stop-core-instance branch from 451d541 to a768e3e Compare November 15, 2023 10:00

maxep force-pushed the maxep/RUM-1040/stop-core-instance branch from a768e3e to 4774bc5 Compare January 8, 2024 16:28

maxep removed the do-not-merge/WIP label Jan 8, 2024

maxep marked this pull request as ready for review January 8, 2024 16:29

maxep requested a review from a team as a code owner January 8, 2024 16:29

maxep dismissed maciejburda’s stale review via 04f24d0 January 8, 2024 16:39

maxep force-pushed the maxep/RUM-1040/stop-core-instance branch 2 times, most recently from 9534e0c to 33373b7 Compare January 9, 2024 18:37

maxep force-pushed the maxep/RUM-1040/stop-core-instance branch from 33373b7 to bbbe421 Compare January 10, 2024 09:12

maxep requested a review from maciejburda January 10, 2024 10:01

maxep commented Jan 10, 2024

View reviewed changes

IntegrationTests/Runner/AppDelegate.swift Outdated Show resolved Hide resolved

maxep commented Jan 10, 2024

View reviewed changes

maxep force-pushed the maxep/RUM-1040/stop-core-instance branch from bbbe421 to 495c05b Compare January 10, 2024 11:07

maciejburda reviewed Jan 10, 2024

View reviewed changes

maxep force-pushed the maxep/RUM-1040/stop-core-instance branch from 495c05b to 1f31222 Compare January 10, 2024 12:56

ganeshnj reviewed Jan 11, 2024

View reviewed changes

ncreated reviewed Jan 11, 2024

View reviewed changes

RUM-1040 Stop core instance

ce479f1

maxep added 3 commits January 11, 2024 14:12

RUM-1040 Add integration test scenario

63d2377

Update CHANGELOG.md

07a9f23

Update documentation

3324a70

maxep force-pushed the maxep/RUM-1040/stop-core-instance branch from 27efbdc to 3324a70 Compare January 11, 2024 13:12

maxep added 4 commits January 11, 2024 17:27

Fix retain cycle in context provider

94180ad

Fix leak in VitalRefreshRateReader

8f4a0dc

Update StopCoreScenarioTests.swift

447d2a0

Update DatadogCoreTests.swift

6066c10

maxep requested review from ncreated, maciejburda and ganeshnj January 12, 2024 10:44

ncreated approved these changes Jan 15, 2024

View reviewed changes

ganeshnj approved these changes Jan 15, 2024

View reviewed changes

maxep merged commit 6b0928a into develop Jan 15, 2024
9 checks passed

maxep deleted the maxep/RUM-1040/stop-core-instance branch January 15, 2024 11:18

ncreated mentioned this pull request Jan 25, 2024

Release 2.7.0 #1648

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RUM-1040 Stop Core Instance #1541

RUM-1040 Stop Core Instance #1541

maxep commented Nov 7, 2023 •

edited

datadog-datadog-prod-us1 bot commented Nov 7, 2023 •

edited

maciejburda left a comment

datadog-datadog-prod-us1 bot commented Nov 15, 2023 •

edited

datadog-datadog-prod-us1 bot commented Jan 9, 2024 •

edited

maxep commented Jan 10, 2024

maxep Jan 10, 2024

ncreated Jan 11, 2024

maxep Jan 10, 2024

maciejburda Jan 10, 2024

maxep Jan 10, 2024

ganeshnj left a comment

ncreated Jan 11, 2024

maxep Jan 11, 2024 •

edited

ncreated Jan 11, 2024

ncreated commented Jan 11, 2024 •

edited

maxep commented Jan 11, 2024

ncreated left a comment

ncreated Jan 15, 2024

maxep Jan 15, 2024

		let core = CoreRegistry.unregisterInstance(named: instanceName) as? DatadogCore
		core?.stop()

RUM-1040 Stop Core Instance #1541

RUM-1040 Stop Core Instance #1541

Conversation

maxep commented Nov 7, 2023 • edited

What and why?

How?

Note

Review checklist

Custom CI job configuration (optional)

datadog-datadog-prod-us1 bot commented Nov 7, 2023 • edited

Datadog Report

maciejburda left a comment

Choose a reason for hiding this comment

datadog-datadog-prod-us1 bot commented Nov 15, 2023 • edited

Datadog Report

datadog-datadog-prod-us1 bot commented Jan 9, 2024 • edited

Datadog Report

🔻 Code Coverage Decreases vs Default Branch (6)

maxep commented Jan 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ganeshnj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxep Jan 11, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncreated commented Jan 11, 2024 • edited

maxep commented Jan 11, 2024

ncreated left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxep commented Nov 7, 2023 •

edited

datadog-datadog-prod-us1 bot commented Nov 7, 2023 •

edited

datadog-datadog-prod-us1 bot commented Nov 15, 2023 •

edited

datadog-datadog-prod-us1 bot commented Jan 9, 2024 •

edited

maxep Jan 11, 2024 •

edited

ncreated commented Jan 11, 2024 •

edited