RUMM-2607: Use event write context for Logs by 0xnm · Pull Request #1103 · DataDog/dd-sdk-android

0xnm · 2022-10-25T09:33:13Z

What does this PR do?

This change brings the usage of event write context (EWC) to the Logs feature, by doing the following:

Logs feature now does the call to withWriteContext to get DatadogContext and the binary batch writer.
There is a step to make Logs feature extracted into a dedicated module: Crash reports feature and NDK crash reporting don't depend on the Logs Feature explicitly now, but they send the necessary request to write crash log via the message bus.
Since many values to create LogEvent is now taken from DatadogContext which is not available during the feature construction time, many values are moved out from the DatadogLogGenerator constructor.
If LogEvent should be created and written in the synchronous manner, caller should pass the syncWrite parameter with the event.
DataWriter for v2 now should accept 2 arguments for the write method - event which should be serialized, and EventBatchWriter which will write serialized event and provided by the withWriteContext call.

There is some duplication between NetworkInfo and UserInfo classes from v2 and network model packages, it will essentially go away later (we won't expose network models in the public API).

Review checklist (to be filled by reviewers)

Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
Make sure you discussed the feature or bugfix with the maintaining team in an Issue
Make sure each commit and the PR mention the Issue number (cf the CONTRIBUTING doc)

codecov-commenter · 2022-10-25T10:59:06Z

Codecov Report

❌ Patch coverage is 89.86175% with 22 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.46%. Comparing base (9b73f57) to head (cd86390).

⚠️ Current head cd86390 differs from pull request most recent head 169cbec

Please upload reports for the commit 169cbec to get more accurate results.

Files with missing lines	Patch %	Lines
...in/com/datadog/android/log/internal/LogsFeature.kt	85.96%	2 Missing and 6 partials ⚠️
...og/android/v2/core/internal/NoOpContextProvider.kt	0.00%	6 Missing ⚠️
...g/android/log/internal/logger/DatadogLogHandler.kt	86.96%	1 Missing and 2 partials ⚠️
.../android/error/internal/DatadogExceptionHandler.kt	84.62%	1 Missing and 1 partial ⚠️
.../android/v2/log/internal/storage/LogsDataWriter.kt	60.00%	2 Missing ⚠️
.../src/main/kotlin/com/datadog/android/log/Logger.kt	93.33%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@                Coverage Diff                @@
##           feature/sdkv2    #1103      +/-   ##
=================================================
- Coverage          82.69%   82.46%   -0.23%     
=================================================
  Files                350      351       +1     
  Lines              11301    11362      +61     
  Branches            1846     1874      +28     
=================================================
+ Hits                9345     9369      +24     
- Misses              1394     1424      +30     
- Partials             562      569       +7

Files with missing lines	Coverage Δ
...n/com/datadog/android/core/internal/CoreFeature.kt	`91.09% <ø> (-0.35%)`	⬇️
...in/com/datadog/android/core/internal/SdkFeature.kt	`88.18% <ø> (ø)`
...adog/android/error/internal/CrashReportsFeature.kt	`100.00% <100.00%> (ø)`
...android/log/internal/domain/DatadogLogGenerator.kt	`98.20% <100.00%> (-0.76%)`	⬇️
...atadog/android/log/internal/domain/LogGenerator.kt	`80.00% <ø> (-3.33%)`	⬇️
...g/android/rum/internal/domain/scope/RumEventExt.kt	`93.75% <100.00%> (ø)`
...android/rum/internal/ndk/DatadogNdkCrashHandler.kt	`87.89% <100.00%> (+0.06%)`	⬆️
.../com/datadog/android/v2/api/context/NetworkInfo.kt	`100.00% <100.00%> (ø)`
.../kotlin/com/datadog/android/v2/core/DatadogCore.kt	`88.30% <100.00%> (+0.53%)`	⬆️
...android/v2/core/internal/DatadogContextProvider.kt	`83.58% <100.00%> (+0.77%)`	⬆️
... and 6 more

... and 10 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

mariusc83 · 2022-10-25T12:23:03Z

+
+    @AnyThread
+    override fun onReceive(event: Any) {
+        if (event !is Map<*, *>) {


mariusc83 · 2022-10-25T12:32:11Z


+    @Suppress("ComplexMethod")
+    private fun sendCrashLog(data: Map<*, *>) {
+        val threadName = data["threadName"] as? String


should we add these keys in static constants or maybe not as they will not be visible from the other modules anyway ?

They are not in the constants on purpose, because caller (crash reporting/NDK crash reporting) and consumer (logs) will be in the different modules without any dependency. We have a task in the backlog to document messages format though.

Maybe later will create some common constants in the core module, will see.

mariusc83 · 2022-10-25T12:32:34Z

+
+        @Suppress("UnsafeThirdPartyFunctionCall")
+        val lock =
+            if (syncWrite) CountDownLatch(1) else null


nice workaround

mariusc83 · 2022-10-25T12:59:17Z

+        datadogContext: DatadogContext,
+        networkInfo: NetworkInfo?
+    ): LogEvent.Network {
+        // TODO RUMM-0000 use V2 (RUM should write V2 version)


I am not sure I get this comment ?

for NDK crash reporting we are saving to the disk UserInfo and NetworkInfo as models from com.datadog.android.core.model package. So later on we deserialized them back in the DatadogNdkCrashHandler and create LogEvent, but outside this flow, say using the normal Logger we are taking UserInfo and NetworkInfo from DatadogContext, and these ones are living in com.datadog.android.v2.api.context, they are different classes.

I see, tks for the explanation ;)

mariusc83 · 2022-10-25T13:09:23Z

+                )
+
+                @Suppress("ThreadSafety") // called in a worker thread context
+                dataWriter.write(eventBatchWriter, log)


why not using the eventBatchWriter directly?

Because of the assertion. EventBatchWriter is taking ByteArray, I cannot use LogEventAssert with it directly (without de-serializing back). So just for the convenience.

xgouchet

LGTM, added one comment

xgouchet added the size-huge This PR is huge sized label Oct 25, 2022

0xnm force-pushed the nogorodnikov/rumm-2607/use-eventwritecontext-for-logs branch from ca2db72 to cd86390 Compare October 25, 2022 10:35

0xnm changed the base branch from feature/sdkv2 to nogorodnikov/merge_develop_into_sdkv2_25102022 October 25, 2022 10:36

Base automatically changed from nogorodnikov/merge_develop_into_sdkv2_25102022 to feature/sdkv2 October 25, 2022 10:41

0xnm marked this pull request as ready for review October 25, 2022 12:01

0xnm requested a review from a team as a code owner October 25, 2022 12:01

mariusc83 reviewed Oct 25, 2022

View reviewed changes

mariusc83 approved these changes Oct 25, 2022

View reviewed changes

xgouchet approved these changes Oct 27, 2022

View reviewed changes

Comment thread dd-sdk-android/src/main/kotlin/com/datadog/android/log/internal/LogsFeature.kt Outdated

RUMM-2607: Use event write context for Logs

169cbec

0xnm force-pushed the nogorodnikov/rumm-2607/use-eventwritecontext-for-logs branch from cd86390 to 169cbec Compare October 27, 2022 11:44

0xnm merged commit 67cc7c3 into feature/sdkv2 Oct 27, 2022

0xnm deleted the nogorodnikov/rumm-2607/use-eventwritecontext-for-logs branch October 27, 2022 12:36

This was referenced Oct 31, 2022

RUMM-2602: Use event write context in RUM #1117

Merged

RUMM-2700: Use event write context in WebView Logs #1121

Merged

xgouchet added this to the 1.16.0 milestone Dec 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

RUMM-2607: Use event write context for Logs#1103

RUMM-2607: Use event write context for Logs#1103
0xnm merged 1 commit into
feature/sdkv2from
nogorodnikov/rumm-2607/use-eventwritecontext-for-logs

0xnm commented Oct 25, 2022 •

edited

Loading

Uh oh!

codecov-commenter commented Oct 25, 2022 •

edited

Loading

Uh oh!

mariusc83 Oct 25, 2022

Uh oh!

mariusc83 Oct 25, 2022

Uh oh!

0xnm Oct 25, 2022

Uh oh!

mariusc83 Oct 25, 2022

Uh oh!

mariusc83 Oct 25, 2022

Uh oh!

0xnm Oct 25, 2022

Uh oh!

mariusc83 Oct 25, 2022

Uh oh!

mariusc83 Oct 25, 2022

Uh oh!

0xnm Oct 25, 2022

Uh oh!

xgouchet left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

0xnm commented Oct 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Review checklist (to be filled by reviewers)

Uh oh!

codecov-commenter commented Oct 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xgouchet left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

0xnm commented Oct 25, 2022 •

edited

Loading

codecov-commenter commented Oct 25, 2022 •

edited

Loading