Expose Telemetry and addLongTask for internal use by fuzzybinary · Pull Request #983 · DataDog/dd-sdk-android

fuzzybinary · 2022-07-12T15:36:11Z

What does this PR do?

This exposes some internal Datadog classes and methods for use by cross platform frameworks.

We expose internal methods through the _InternalProxy class, which is documented for internal use, and the _internal static member on the Datadog object. We keep Telemetry and RUM as separate proxies, in case we have to split them as v2 takes shape.

As part of exposing telemetry I also needed a way to report errors that are not Throwable, so I modified the event to replace the throwable member with stack and kind, and moved the creation of those strings farther up the chain (they were created on event write prior).

Additional Notes

Please double check all my logic on the switch from throwable to stack and kind

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)

xgouchet · 2022-07-13T06:48:13Z

+        var stack: String? = null
+        var kind: String? = null
+        throwable?.let {
+            stack = it.loggableStackTrace()
+            kind = it.javaClass.canonicalName ?: it.javaClass.simpleName
+        }
+        handleEvent(RumRawEvent.SendTelemetry(TelemetryType.ERROR, message, stack, kind))


Suggested change

var stack: String? = null

var kind: String? = null

throwable?.let {

stack = it.loggableStackTrace()

kind = it.javaClass.canonicalName ?: it.javaClass.simpleName

}

handleEvent(RumRawEvent.SendTelemetry(TelemetryType.ERROR, message, stack, kind))

val stack: String? = throwable?.loggableStackTrace()

var kind: String? = throwable?.javaClass?.canonicalName ?: throwable?.javaClass?.simpleName

handleEvent(RumRawEvent.SendTelemetry(TelemetryType.ERROR, message, stack, kind))

xgouchet · 2022-07-13T06:49:55Z

+        @StringForgery kind: String
+    ) {
+        // Given
+        val proxy = _InternalProxy(GlobalRum.get() as? AdvancedRumMonitor)


Suggested change

val proxy = _InternalProxy(GlobalRum.get() as? AdvancedRumMonitor)

val proxy = _InternalProxy(rumMonitor.mockInstance)

xgouchet · 2022-07-13T06:50:02Z

+        @StringForgery message: String
+    ) {
+        // Given
+        val proxy = _InternalProxy(GlobalRum.get() as? AdvancedRumMonitor)


Suggested change

val proxy = _InternalProxy(GlobalRum.get() as? AdvancedRumMonitor)

val proxy = _InternalProxy(rumMonitor.mockInstance)

xgouchet · 2022-07-13T06:50:04Z

+        @StringForgery target: String
+    ) {
+        // Given
+        val proxy = _InternalProxy(GlobalRum.get() as? AdvancedRumMonitor)


You shouldn't rely on static/global states in your tests.

Suggested change

val proxy = _InternalProxy(GlobalRum.get() as? AdvancedRumMonitor)

val proxy = _InternalProxy(rumMonitor.mockInstance)

0xnm · 2022-07-13T06:43:27Z

+)
+class _InternalProxy {
+    class _TelemetryProxy {
+        private val telemetry: Telemetry = Telemetry()


we can also directly reference com.datadog.android.core.internal.utils.telemetry property I think (but in both cases the question of mocking for unit-tests arise).

Do you think we should inject Telemetry instead?

you can use default arguments in the constructor and provide them explicitly in the test.

0xnm · 2022-07-13T07:23:36Z

+     *
+     * @see _InternalProxy
+     */
+    @JvmStatic


I don't think we need this annotation here, because this property is supposed to be called only by us, so no interop with Java is needed (both Flutter and RN use Kotlin for the native modules).

fuzzybinary · 2022-07-13T17:10:33Z

Given some things we found in the iOS PR, I've restructured this to separate the "core" internals (only telemetry at the moment) and RUM Internals. This simplifies testing as well as avoids some issues with lazy initializers having unwelcome side affects.

codecov-commenter · 2022-07-15T06:49:01Z

Codecov Report

❌ Patch coverage is 77.41935% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.98%. Comparing base (009bd1a) to head (2242bdb).
⚠️ Report is 3637 commits behind head on develop.

Files with missing lines	Patch %	Lines
.../android/rum/internal/monitor/DatadogRumMonitor.kt	42.86%	2 Missing and 2 partials ⚠️
.../main/kotlin/com/datadog/android/_InternalProxy.kt	88.89%	1 Missing ⚠️
...om/datadog/android/telemetry/internal/Telemetry.kt	0.00%	1 Missing ⚠️
...ndroid/telemetry/internal/TelemetryEventHandler.kt	87.50%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #983      +/-   ##
===========================================
- Coverage    83.08%   82.98%   -0.10%     
===========================================
  Files          268      270       +2     
  Lines         9218     9241      +23     
  Branches      1485     1486       +1     
===========================================
+ Hits          7658     7668      +10     
- Misses        1152     1162      +10     
- Partials       408      411       +3

Files with missing lines	Coverage Δ
...oid/src/main/kotlin/com/datadog/android/Datadog.kt	`81.51% <100.00%> (+0.16%)`	⬆️
.../main/kotlin/com/datadog/android/rum/RumMonitor.kt	`77.78% <ø> (ø)`
...otlin/com/datadog/android/rum/_RumInternalProxy.kt	`100.00% <100.00%> (ø)`
...g/android/rum/internal/domain/scope/RumRawEvent.kt	`82.43% <100.00%> (+0.12%)`	⬆️
.../main/kotlin/com/datadog/android/_InternalProxy.kt	`88.89% <88.89%> (ø)`
...om/datadog/android/telemetry/internal/Telemetry.kt	`66.67% <0.00%> (-13.33%)`	⬇️
...ndroid/telemetry/internal/TelemetryEventHandler.kt	`91.94% <87.50%> (ø)`
.../android/rum/internal/monitor/DatadogRumMonitor.kt	`92.86% <42.86%> (-2.23%)`	⬇️

... and 5 files with indirect coverage changes

🚀 New features to boost your workflow:

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

0xnm · 2022-07-15T08:00:35Z

+        @StringForgery message: String
+    ) {
+        // Given
+        val mockTelemetry = Mockito.mock(Telemetry::class.java)


nit: we can do import org.mockito.Mockito.mock and just use mock call directly without Mockito. prefix as it is done in the codebase.

0xnm · 2022-07-15T08:03:30Z

+        @StringForgery target: String
+    ) {
+        // Given
+        val proxy = _RumInternalProxy(rumMonitor.mockInstance)


we can simply mock<AdvancedRumMonitor>() here, don't think we need to use GlobalRumMonitorTestConfiguration.

We expose internal methods through the _InternalProxy class, which is documented for internal use, and the `_internal` static member on the Datadog object. We keep Telemetry and RUM as separate proxies, in case we have to split them as v2 takes shape. Exposing Telemetry requires adding a way for reporting errors that are not Throwable (for use in cross platform frameworks), so I modified the event to take `stack` and `kind` as strings instead.

fuzzybinary · 2022-07-15T13:50:37Z

@0xnm updated and rebased on develop for a final review.

0xnm

lgtm!

fuzzybinary requested a review from a team as a code owner July 12, 2022 15:36

fuzzybinary force-pushed the jward/expose-telemetry branch 3 times, most recently from 0041f01 to 121e55e Compare July 12, 2022 17:34

xgouchet suggested changes Jul 13, 2022

View reviewed changes

0xnm requested changes Jul 13, 2022

View reviewed changes

fuzzybinary requested review from 0xnm and xgouchet July 13, 2022 17:09

xgouchet added the size-medium This PR is medium sized label Jul 13, 2022

fuzzybinary force-pushed the jward/expose-telemetry branch from 660ef06 to 02e4afb Compare July 13, 2022 19:03

xgouchet approved these changes Jul 15, 2022

View reviewed changes

0xnm requested changes Jul 15, 2022

View reviewed changes

fuzzybinary added 5 commits July 15, 2022 09:49

Fix detekt by suppressing warnings about naming.

17078e7

Refactor RUM internal Proxy into its own object.

c5ae6a5

Add _InternalProxy classes to NotTestableApis

65fed2c

Cleanup tests from PR feedback

2242bdb

fuzzybinary force-pushed the jward/expose-telemetry branch from 043616a to 2242bdb Compare July 15, 2022 13:49

fuzzybinary requested a review from 0xnm July 15, 2022 13:50

0xnm approved these changes Jul 15, 2022

View reviewed changes

fuzzybinary merged commit 4fc552f into develop Jul 15, 2022

fuzzybinary deleted the jward/expose-telemetry branch July 15, 2022 14:17

xgouchet added this to the 1.14.0 milestone Dec 13, 2023

	val proxy = _InternalProxy(GlobalRum.get() as? AdvancedRumMonitor)
	val proxy = _InternalProxy(rumMonitor.mockInstance)

Uh oh!

Conversation

fuzzybinary commented Jul 12, 2022

What does this PR do?

Additional Notes

Review checklist (to be filled by reviewers)

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!

fuzzybinary commented Jul 13, 2022

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fuzzybinary commented Jul 15, 2022

Uh oh!

0xnm left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-commenter commented Jul 15, 2022 •

edited

Loading