GH1618 Simplify (internal) flushing logic

[ncreated]

What and why?

🧰 The internal Datadog.internalFlushAndDeinitialize() does not work without setting DD_SDK_COMPILED_FOR_TESTING compiler flag. To simplify the reasoning about internal "flush", I propose to remove this macro from downstream logic. It will be still used to hide the public API behind this feature, see:

dd-sdk-ios/DatadogCore/Sources/Datadog.swift

Lines 485 to 489 in a43c63d

	#if DD_SDK_COMPILED_FOR_TESTING
	public static func flushAndDeinitialize(instanceName: String = CoreRegistry.defaultInstanceName) {
	internalFlushAndDeinitialize(instanceName: instanceName)
	}
	#endif

Solves #1618

How?

The "flush" logic was protected twice:

• first, the public API was hidden behind DD_SDK_COMPILED_FOR_TESTING flag,
• second, part of its implementation was conditionally enabled on DD_SDK_COMPILED_FOR_TESTING.

The second occurrence was redundant and it is removed.

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]

Datadog Report

Branch report: ncreated/GH1618/allow-flushing-SDK-using-internal-API
Commit report: c37be37
Test service: dd-sdk-ios

✅ 0 Failed, 2479 Passed, 0 Skipped, 5m 8.75s Wall Time
🔻 Test Sessions change in coverage: 9 decreased, 3 increased, 1 no change

🔻 Code Coverage Decreases vs Default Branch (9)

This report shows up to 5 code coverage decreases.

• test DatadogLogsTests tvOS 44.78% (Δ-0.17%) - Details
• test DatadogLogsTests iOS 44.74% (Δ-0.15%) - Details
• test DatadogInternalTests tvOS 81.21% (Δ-0.06%) - Details
• test DatadogInternalTests iOS 81.20% (Δ-0.03%) - Details
• test DatadogRUMTests iOS 79.39% (Δ-0.02%) - Details

[maciejburda]

Are we sure about this? When we do this "open" batch is going to be included.

This function is called in the DataUploadWorker as well (it happens in line 77: fileReader.readFiles(limit: maxBatchesPerUpload)).

[ncreated]

This logic is conditioned by ignoreFilesAgeWhenReading flag enabled from core.flushAndTearDown() which is only available when DD_SDK_COMPILED_FOR_TESTING macro is set:

dd-sdk-ios/DatadogCore/Sources/Datadog.swift

Lines 485 to 489 in 3686259

	#if DD_SDK_COMPILED_FOR_TESTING
	public static func flushAndDeinitialize(instanceName: String = CoreRegistry.defaultInstanceName) {
	internalFlushAndDeinitialize(instanceName: instanceName)
	}
	#endif

so I don't see how this may interrupt DataUploadWorker in production apps. The entire concept of "ignoring files age" was introduced for testing purpose (we want to force-send all data in tests). Does it sound ok to you @maciejburda?

Ultimately, I don't like using macros in this way as they shred the codebase. Since it was introduced a long time ago however, we managed to keep the impact of DD_SDK_COMPILED_FOR_TESTING very minimal. A better approach would be to abstract certain components and inject different implementation in tests - it implies a much bigger effort, but I hope we do it one day.

[maciejburda]

Not sure what am I missing, but the modified function getReadableFiles(excludingFilesNamed excludedFileNames: Set<String> = [], limit: Int = .max) is being called by the DataUploadWorker dependency: FileReader.

Removing this #if makes the file orchestrator ignore file age for producation code as well, no?

[ncreated]

The logic we enable here is conditioned by ignoreFilesAgeWhenReading flag 🙂:

if ignoreFilesAgeWhenReading {
   return filesFromOldest
      .prefix(limit)
      .map { $0.file }
}

And this is only set to true in our tests 💡.

[maciejburda]

Ah, I get it now! Thanks for clarifying :)

[maxep]

👍