RUM-1187 Add more information into the batch telemetry #1641

mariusc83 · 2023-09-26T11:35:56Z

What does this PR do?

Following the latest discussions regarding the problem with the batch telemetry data for Android SDK we decided to add more information into the related logs:

thread name (might be useful for potential concurrency debug)
the batch file name
the tracking consent according with the batch origin folder

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)

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 · 2023-09-26T11:51:52Z

Codecov Report

Merging #1641 (d64d0d6) into develop (876f439) will increase coverage by 0.03%.
Report is 23 commits behind head on develop.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop    #1641      +/-   ##
===========================================
+ Coverage    83.79%   83.82%   +0.03%     
===========================================
  Files          455      455              
  Lines        15684    15697      +13     
  Branches      2333     2336       +3     
===========================================
+ Hits         13142    13158      +16     
- Misses        1918     1924       +6     
+ Partials       624      615       -9

Files	Coverage Δ
...id/core/internal/metrics/BatchMetricsDispatcher.kt	`98.53% <100.00%> (+0.32%)`	⬆️
...rsistence/file/advanced/FeatureFileOrchestrator.kt	`100.00% <100.00%> (ø)`

... and 14 files with indirect coverage changes

...oid-core/src/main/kotlin/com/datadog/android/core/internal/metrics/BatchMetricsDispatcher.kt

jonathanmos · 2023-09-27T07:44:05Z

...oid-core/src/main/kotlin/com/datadog/android/core/internal/metrics/BatchMetricsDispatcher.kt

@@ -218,6 +238,15 @@ internal class BatchMetricsDispatcher(
        /* The value for the type of the metric.*/
        internal const val BATCH_CLOSED_TYPE_VALUE = "batch closed"

+        /* The value of the tracking consent according with this file origin.*/


Suggested change

/* The value of the tracking consent according with this file origin.*/

/* The tracking consent according to the file origin.*/

jonathanmos · 2023-09-27T07:46:54Z

...oid-core/src/main/kotlin/com/datadog/android/core/internal/metrics/BatchMetricsDispatcher.kt

+        internal const val FILE_NAME = "name"
+
+        /* The thread name from which the current metric was sent.*/
+        internal const val THREAD_NAME = "thread"


minor: are these key names taken from a telemetry rfc doc? If we have flexibility it may be better to have more explicit map keys (e.g. it looks like we're sending a semiambiguous name in the map, where it might be preferable to send filename, and to a lesser extent for thread/threadname). wdyt?

wanted to use a shorter key for having lighter logs but yeah...3 chars will not kill

0xnm

lgtm! I've added one suggestions

0xnm · 2023-09-27T07:55:43Z

...oid-core/src/main/kotlin/com/datadog/android/core/internal/metrics/BatchMetricsDispatcher.kt

    // endregion

    companion object {

+        internal val IS_GRANTED_DIR_REG_EX = Regex("[a-z]+-v[0-9]+")


should we move this to the FeatureFileOrchestrator? At least this way if we update naming convention for the folders we won't forget to update this one as well.

actually this is not good enough :)...need to find a better one. We also have session-replay-v1 or ndk_crash_reports_intermediary-v1

0xnm · 2023-09-27T09:11:36Z

...otlin/com/datadog/android/core/internal/persistence/file/advanced/FeatureFileOrchestrator.kt

-        internal const val PENDING_DIR = "%s-pending-v$VERSION"
+        internal const val PENDING_DIR_SUFFIX = "-pending-v$VERSION"
+        internal const val PENDING_DIR = "%s$PENDING_DIR_SUFFIX"


it seems that this change can be reverted

xgouchet · 2023-09-27T11:20:59Z

...otlin/com/datadog/android/core/internal/persistence/file/advanced/FeatureFileOrchestrator.kt

@@ -64,6 +64,10 @@ internal class FeatureFileOrchestrator(
    )

    companion object {
+        private const val BASE_DIR_NAME_REG_EX = "([a-z]+[-|_])+"


Do we have a list of base dire name used? It seems this regex is allowing a lot more than it should (e.g.: abcdef|)

mariusc83 self-assigned this Sep 26, 2023

mariusc83 force-pushed the mconstantin/rum-1095/fix-batch-close-telemetry-data branch from 2450355 to 876f439 Compare September 26, 2023 11:51

mariusc83 force-pushed the mconstantin/rum-1187/add-more-data-for-batch-close-delete-telemetry branch 2 times, most recently from cbdb6b4 to e359fe4 Compare September 26, 2023 12:02

mariusc83 marked this pull request as ready for review September 26, 2023 12:03

mariusc83 requested a review from a team as a code owner September 26, 2023 12:03

Base automatically changed from mconstantin/rum-1095/fix-batch-close-telemetry-data to develop September 26, 2023 13:01

0xnm reviewed Sep 26, 2023

View reviewed changes

...oid-core/src/main/kotlin/com/datadog/android/core/internal/metrics/BatchMetricsDispatcher.kt Outdated Show resolved Hide resolved

...oid-core/src/main/kotlin/com/datadog/android/core/internal/metrics/BatchMetricsDispatcher.kt Show resolved Hide resolved

mariusc83 force-pushed the mconstantin/rum-1187/add-more-data-for-batch-close-delete-telemetry branch from e359fe4 to 5790972 Compare September 27, 2023 07:24

mariusc83 requested a review from 0xnm September 27, 2023 07:28

jonathanmos previously approved these changes Sep 27, 2023

View reviewed changes

0xnm previously approved these changes Sep 27, 2023

View reviewed changes

mariusc83 dismissed stale reviews from 0xnm and jonathanmos via 75bd1eb September 27, 2023 08:53

mariusc83 force-pushed the mconstantin/rum-1187/add-more-data-for-batch-close-delete-telemetry branch 2 times, most recently from 75bd1eb to 938ab50 Compare September 27, 2023 09:02

0xnm previously approved these changes Sep 27, 2023

View reviewed changes

mariusc83 requested a review from jonathanmos September 27, 2023 09:27

RUM-1187 Add more information into the batch telemetry

d64d0d6

mariusc83 dismissed 0xnm’s stale review via d64d0d6 September 27, 2023 09:28

mariusc83 force-pushed the mconstantin/rum-1187/add-more-data-for-batch-close-delete-telemetry branch from 938ab50 to d64d0d6 Compare September 27, 2023 09:28

0xnm approved these changes Sep 27, 2023

View reviewed changes

jonathanmos approved these changes Sep 27, 2023

View reviewed changes

mariusc83 merged commit 2043399 into develop Sep 27, 2023

mariusc83 deleted the mconstantin/rum-1187/add-more-data-for-batch-close-delete-telemetry branch September 27, 2023 11:18

xgouchet requested changes Sep 27, 2023

View reviewed changes

xgouchet added this to the 2.2.0 milestone Dec 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RUM-1187 Add more information into the batch telemetry #1641

RUM-1187 Add more information into the batch telemetry #1641

mariusc83 commented Sep 26, 2023 •

edited

Loading

codecov-commenter commented Sep 26, 2023 •

edited

Loading

jonathanmos Sep 27, 2023

jonathanmos Sep 27, 2023

mariusc83 Sep 27, 2023

0xnm left a comment

0xnm Sep 27, 2023

mariusc83 Sep 27, 2023

0xnm Sep 27, 2023

xgouchet Sep 27, 2023

	/* The value of the tracking consent according with this file origin.*/
	/* The tracking consent according to the file origin.*/

RUM-1187 Add more information into the batch telemetry #1641

RUM-1187 Add more information into the batch telemetry #1641

Conversation

mariusc83 commented Sep 26, 2023 • edited Loading

What does this PR do?

Motivation

Additional Notes

Review checklist (to be filled by reviewers)

codecov-commenter commented Sep 26, 2023 • edited Loading

Codecov Report

jonathanmos Sep 27, 2023

Choose a reason for hiding this comment

jonathanmos Sep 27, 2023

Choose a reason for hiding this comment

mariusc83 Sep 27, 2023

Choose a reason for hiding this comment

0xnm left a comment

Choose a reason for hiding this comment

0xnm Sep 27, 2023

Choose a reason for hiding this comment

mariusc83 Sep 27, 2023

Choose a reason for hiding this comment

0xnm Sep 27, 2023

Choose a reason for hiding this comment

xgouchet Sep 27, 2023

Choose a reason for hiding this comment

mariusc83 commented Sep 26, 2023 •

edited

Loading

codecov-commenter commented Sep 26, 2023 •

edited

Loading