RUM-1844: Add Method Call Telemetry #1940
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jonathanmos
changed the title
jonathanmos
force-pushed
the
jmoskovich/rum-1844/method-call-telemetry
branch
from
2eccdbe to
ba05245
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1940 +/- ##
===========================================
- Coverage 83.55% 83.17% -0.38%
===========================================
Files 478 481 +3
Lines 17576 17643 +67
Branches 2672 2682 +10
===========================================
- Hits 14684 14673 -11
- Misses 2164 2239 +75
- Partials 728 731 +3
|
jonathanmos
commented
Mar 27, 2024
...otlin/com/datadog/android/sessionreplay/internal/recorder/telemetry/MethodCalledTelemetry.kt
Outdated
Show resolved
Hide resolved
jonathanmos
commented
Mar 27, 2024
...ain/kotlin/com/datadog/android/sessionreplay/internal/recorder/telemetry/TelemetryWrapper.kt
Outdated
Show resolved
Hide resolved
maciejburda
approved these changes
Mar 27, 2024
There was a problem hiding this comment.
Compared to iOS implementation and documentation (internal).
LGTM
xgouchet
requested changes
Mar 27, 2024
...ain/kotlin/com/datadog/android/sessionreplay/internal/recorder/telemetry/TelemetryWrapper.kt
Outdated
Show resolved
Hide resolved
...ain/kotlin/com/datadog/android/sessionreplay/internal/recorder/telemetry/TelemetryWrapper.kt
Outdated
Show resolved
Hide resolved
...otlin/com/datadog/android/sessionreplay/internal/recorder/telemetry/MethodCalledTelemetry.kt
Outdated
Show resolved
Hide resolved
xgouchet
requested changes
Mar 27, 2024
.../src/main/kotlin/com/datadog/android/sessionreplay/internal/recorder/telemetry/MetricBase.kt
Outdated
Show resolved
Hide resolved
...otlin/com/datadog/android/sessionreplay/internal/recorder/telemetry/MethodCalledTelemetry.kt
Outdated
Show resolved
Hide resolved
mariusc83
reviewed
Mar 28, 2024
...otlin/com/datadog/android/sessionreplay/internal/recorder/telemetry/MethodCalledTelemetry.kt
Outdated
Show resolved
Hide resolved
.../src/main/kotlin/com/datadog/android/sessionreplay/internal/recorder/telemetry/MetricBase.kt
Outdated
Show resolved
Hide resolved
jonathanmos
force-pushed
the
jmoskovich/rum-1844/method-call-telemetry
branch
from
413769b to
0069f66
Compare
xgouchet
approved these changes
Apr 17, 2024
jonathanmos
force-pushed
the
jmoskovich/rum-1844/method-call-telemetry
branch
from
7c1d16c to
bbcafbe
Compare
xgouchet
reviewed
Apr 18, 2024
buildSrc/src/main/kotlin/com/datadog/gradle/plugin/jsonschema/TypeDefinition.kt
Show resolved
Hide resolved
xgouchet
approved these changes
Apr 22, 2024
jonathanmos
force-pushed
the
jmoskovich/rum-1844/method-call-telemetry
branch
from
459f83d to
50ab1fc
Compare
jonathanmos
force-pushed
the
jmoskovich/rum-1844/method-call-telemetry
branch
from
50ab1fc to
c9854fa
Compare
mariusc83
approved these changes
Apr 29, 2024
jonathanmos
force-pushed
the
jmoskovich/rum-1844/method-call-telemetry
branch
from
c9854fa to
d951c21
Compare
xgouchet
approved these changes
May 2, 2024
0xnm
reviewed
May 3, 2024
| * @param samplingRate value between 0-100 for sampling the event. | ||
| * @return a PerformanceMetric object that can later be used to send telemetry, or null if sampled out | ||
| */ | ||
| fun startPerformanceMeasure( |
There was a problem hiding this comment.
why this one is a part of the FeatureScope? by looking on the implementation it has nothing to do with the Feature - it takes InternalLogger, which is bound to the SDK core and the rest is just the arguments passed to the method. Is it implemented the same way on iOS?
| /** | ||
| * Base class for performance metric events. | ||
| */ | ||
| interface PerformanceMetric { |
There was a problem hiding this comment.
these metrics are going to our telemetry, so it makes sense to put @InternalApi annotation here.
| * @param logger - an instance of the internal logger. | ||
| * @param startTime - the time when the metric is instantiated, to be used as the start point for the measurement. | ||
| */ | ||
| class MethodCalledTelemetry( |
There was a problem hiding this comment.
this one can be internal class if I'm not wrong, it is not used outside the package, it seems? And instances anyway are created via PerformanceMetric#startMetric
Comment on lines
+40
to
+41
| internalSdkCore: InternalSdkCore? = Datadog.getInstance() as? InternalSdkCore, | ||
| private val deviceInfo: DeviceInfo? = internalSdkCore?.getDatadogContext()?.deviceInfo |
There was a problem hiding this comment.
why this is needed, if inside handleEvent method there is already DatadogContext provided as a part of withWriteContext lambda?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Adds telemetry for view tree traversal time
Device and OS properties were added to debug and error telemetry. This required adding a fix for TypeDefinition to correctly merge additionalProperties (since the "telemetry" property now exists in the common schema). Also, I've opened a pr for rum-events-format to modify the schema there
Motivation
What inspired you to submit this pull request?
Additional Notes
Anything else we should know when reviewing?
Review checklist (to be filled by reviewers)