Skip to content

RUMM-2630 RUM Configuration Telemetry#1118

Merged
xgouchet merged 13 commits into
developfrom
xgouchet/RUMM-2630/configuration_telemetry
Nov 7, 2022
Merged

RUMM-2630 RUM Configuration Telemetry#1118
xgouchet merged 13 commits into
developfrom
xgouchet/RUMM-2630/configuration_telemetry

Conversation

@xgouchet

@xgouchet xgouchet commented Nov 2, 2022

Copy link
Copy Markdown
Contributor

What does this PR do?

Implement a RUM Configuration Telemetry Event.

@xgouchet
xgouchet requested a review from a team as a code owner November 2, 2022 10:04
@xgouchet
xgouchet force-pushed the xgouchet/RUMM-2630/configuration_telemetry branch from 0b82881 to 87001e1 Compare November 2, 2022 13:53
@xgouchet xgouchet added the size-large This PR is large sized label Nov 2, 2022
0xnm
0xnm approved these changes Nov 2, 2022
View reviewed changes

@0xnm 0xnm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

looks nice! I've added some comments, but they are not blocking.

Comment thread detekt.yml
# endregion
# region Java Concurrency
- "java.lang.Thread.constructor(java.lang.Runnable, kotlin.String):java.lang.NullPointerException,java.lang.SecurityException,java.lang.IllegalArgumentException"
- "java.lang.Thread.constructor(java.lang.Runnable, kotlin.String):java.lang.NullPointerException,java.lang.SecurityException,java.lang.IllegalArgumentException"

@0xnm 0xnm Nov 2, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

seems like a duplicate

Comment thread detekt.yml
@@ -1061,10 +1062,11 @@ datadog:
- "kotlin.collections.MutableMap.remove(com.datadog.android.rum.internal.vitals.VitalListener)"
- "kotlin.collections.MutableMap.remove(kotlin.String)"
- "kotlin.collections.MutableSet.add(com.datadog.android.telemetry.internal.TelemetryEventHandler.EventIdentity)"

@0xnm 0xnm Nov 2, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

minor: this line can be removed, there is no such class anymore

Comment thread ...-android/src/test/kotlin/com/datadog/android/telemetry/internal/TelemetryEventHandlerTest.kt Outdated
argumentCaptor<TelemetryConfigurationEvent> {
verify(mockWriter).write(capture())
assertConfigEventMatchesRawEvent(firstValue, configRawEvent, rumContext)
assertThat(firstValue)

@0xnm 0xnm Nov 2, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

should it have the continuation or be removed?

@xgouchet xgouchet Nov 4, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Indeed it can be removed

@fuzzybinary

fuzzybinary commented Nov 2, 2022

Copy link
Copy Markdown
Member

What do we think the process will be for the Cross Platform frameworks to set their configuration properties? It looks like the current event just takes the Configuration which won't have some of the cross platform information available.

mariusc83
mariusc83 approved these changes Nov 3, 2022
View reviewed changes

@mariusc83 mariusc83 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Good job. LGTM !!

@xgouchet

xgouchet commented Nov 7, 2022

Copy link
Copy Markdown
Contributor Author

@fuzzybinary

What do we think the process will be for the Cross Platform frameworks to set their configuration properties? It looks like the current event just takes the Configuration which won't have some of the cross platform information available.

The idea will be to embed some configuration attributes in the additionalConfig property, and read them in the TelemetryHandler (e.g.: trackFlutterPerformance = configuration.additionalConfig["_dd.configuration.trackFlutterPerformance"])

@fuzzybinary

fuzzybinary commented Nov 7, 2022

Copy link
Copy Markdown
Member

@xgouchet

The idea will be to embed some configuration attributes in the additionalConfig property, and read them in the TelemetryHandler (e.g.: trackFlutterPerformance = configuration.additionalConfig["_dd.configuration.trackFlutterPerformance"])

I took a different approach on iOS, using a mapper style API instead. The problem is, we don't know the value of a lot of theses during the first call for initialization, so we can't send them in the additional config.

See https://github.com/DataDog/dd-sdk-ios/pull/1039/files#diff-dbaa857f0082fd2cf6795c86b222655eb9d9aafe9e5efd8a5d472891bb5c0e8fR177

@xgouchet
xgouchet force-pushed the xgouchet/RUMM-2630/configuration_telemetry branch from 6815500 to d5f7c65 Compare November 7, 2022 14:25
@xgouchet

xgouchet commented Nov 7, 2022

Copy link
Copy Markdown
Contributor Author

@fuzzybinary
Indeed that would be a possible approach too. I think it'll be easy to do one or the other in another PR when needed.

@xgouchet
xgouchet force-pushed the xgouchet/RUMM-2630/configuration_telemetry branch from d5f7c65 to 1291472 Compare November 7, 2022 15:46
@xgouchet
xgouchet force-pushed the xgouchet/RUMM-2630/configuration_telemetry branch from 1291472 to dfbb12c Compare November 7, 2022 17:35
@codecov-commenter

codecov-commenter commented Nov 7, 2022

Copy link
Copy Markdown

Codecov Report

Merging #1118 (dfbb12c) into develop (8ba3c1f) will decrease coverage by 0.16%.
The diff coverage is 67.26%.

@@             Coverage Diff             @@
##           develop    #1118      +/-   ##
===========================================
- Coverage    83.24%   83.07%   -0.16%     
===========================================
  Files          273      274       +1     
  Lines         9372     9459      +87     
  Branches      1504     1539      +35     
===========================================
+ Hits          7801     7858      +57     
- Misses        1153     1156       +3     
- Partials       418      445      +27
Impacted Files Coverage Δ
...ndroid/telemetry/internal/TelemetryEventHandler.kt 69.64% <50.00%> (-22.42%) ⬇️
...oid/src/main/kotlin/com/datadog/android/Datadog.kt 81.95% <60.87%> (-0.01%) ⬇️
...n/kotlin/com/datadog/android/DatadogInterceptor.kt 87.50% <100.00%> (+0.29%) ⬆️
...atadog/android/core/configuration/Configuration.kt 98.76% <100.00%> (+1.26%) ⬆️
...c/main/kotlin/com/datadog/android/rum/GlobalRum.kt 70.27% <100.00%> (+0.83%) ⬆️
...ndroid/rum/internal/domain/event/RumEventMapper.kt 94.23% <100.00%> (+0.23%) ⬆️
...id/rum/internal/domain/event/RumEventSerializer.kt 99.09% <100.00%> (+0.02%) ⬆️
...um/internal/domain/event/RumEventSourceProvider.kt 100.00% <100.00%> (ø)
...g/android/rum/internal/domain/scope/RumRawEvent.kt 82.58% <100.00%> (+0.11%) ⬆️
.../android/rum/internal/monitor/DatadogRumMonitor.kt 93.33% <100.00%> (+0.72%) ⬆️
... and 11 more

@xgouchet
xgouchet merged commit b260f33 into develop Nov 7, 2022
@xgouchet
xgouchet deleted the xgouchet/RUMM-2630/configuration_telemetry branch November 7, 2022 18:30
@0xnm 0xnm mentioned this pull request Nov 8, 2022
Merged
3 tasks
@xgouchet xgouchet added this to the 1.15.0 milestone Dec 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mariusc83 mariusc83 mariusc83 approved these changes
@0xnm 0xnm 0xnm approved these changes
@plousada plousada plousada approved these changes

Assignees

No one assigned

Labels

size-large This PR is large sized

Projects

None yet

Milestone

1.15.0

Development

Successfully merging this pull request may close these issues.

6 participants

@xgouchet @fuzzybinary @codecov-commenter @mariusc83 @0xnm @plousada