Skip to content

RUM-3458: Fix time drift in RecordedDataQueueHandler#2075

Merged
jonathanmos merged 1 commit into
developfrom
jmoskovich/rum-3458/fix-time-drift
Jun 20, 2024
Merged

RUM-3458: Fix time drift in RecordedDataQueueHandler#2075
jonathanmos merged 1 commit into
developfrom
jmoskovich/rum-3458/fix-time-drift

Conversation

@jonathanmos

@jonathanmos jonathanmos commented Jun 7, 2024

Copy link
Copy Markdown
Member

What does this PR do?

Fixes an issue in the RecordedDataQueueHandler where the "current" time was being passed in to the loop, but never updated inside it. This could lead to the time drifting when the queue was very long, leading to items being considered out of date.

Motivation

We have telemetry indicating numerous DataQueueItems are being dropped for being out of date.

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

codecov-commenter commented Jun 7, 2024 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.16%. Comparing base (d3f69b6) to head (5e3f8b3).
⚠️ Report is 3307 commits behind head on develop.

Additional details and impacted files 
@@             Coverage Diff             @@
##           develop    #2075      +/-   ##
===========================================
- Coverage    83.19%   83.16%   -0.03%     
===========================================
  Files          495      495              
  Lines        17773    17752      -21     
  Branches      2685     2684       -1     
===========================================
- Hits         14785    14762      -23     
+ Misses        2256     2250       -6     
- Partials       732      740       +8
Files with missing lines Coverage Δ
...nreplay/internal/async/RecordedDataQueueHandler.kt 97.50% <100.00%> (+0.59%) ⬆️
...sionreplay/internal/async/RecordedDataQueueItem.kt 100.00% <100.00%> (ø)
...nreplay/internal/recorder/SessionReplayRecorder.kt 96.80% <100.00%> (+0.19%) ⬆️

... and 29 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jonathanmos
jonathanmos marked this pull request as ready for review June 7, 2024 11:49
@jonathanmos
jonathanmos requested review from a team as code owners June 7, 2024 11:49
@jonathanmos
jonathanmos force-pushed the jmoskovich/rum-3458/fix-time-drift branch 2 times, most recently from e4fa696 to 9799a02 Compare June 10, 2024 15:34
@jonathanmos
jonathanmos requested a review from 0xnm June 11, 2024 10:56
0xnm
0xnm approved these changes Jun 12, 2024
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.

lgtm. Since we care only about item age at this point, using 2 System.nanoTime() instants is fine and result of this call is monotonically increasing.

However, there is a second timestamp involved, which is attached to the queue item and then used later to build wireframes (nextItem.recordedQueuedItemContext.timestamp), so it is better to have another pair of eyes for the review here, because I'm don't have a deep knowledge of the whole processing pipeline.

Comment thread ...src/main/kotlin/com/datadog/android/sessionreplay/internal/recorder/SessionReplayRecorder.kt Outdated
internalLogger = internalLogger,

/**
* TODO RUMM-0000 consider change to LoggingThreadPoolExecutor once V2 is merged.

@0xnm 0xnm Jun 12, 2024

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.

I think it is time to do it: it is a simple change and can be made in another PR

xgouchet
xgouchet suggested changes Jun 13, 2024
View reviewed changes
Comment thread ...src/main/kotlin/com/datadog/android/sessionreplay/internal/recorder/SessionReplayRecorder.kt Outdated
@jonathanmos
jonathanmos force-pushed the jmoskovich/rum-3458/fix-time-drift branch from 9799a02 to 5e3f8b3 Compare June 17, 2024 11:08
@jonathanmos
jonathanmos requested a review from xgouchet June 17, 2024 11:33
@jonathanmos
jonathanmos merged commit a164ce7 into develop Jun 20, 2024
@jonathanmos
jonathanmos deleted the jmoskovich/rum-3458/fix-time-drift branch June 20, 2024 07:09
@xgouchet xgouchet added this to the 2.11.x milestone Jul 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@0xnm 0xnm 0xnm approved these changes
+1 more reviewer
@xgouchet xgouchet xgouchet approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

2.11.x

Development

Successfully merging this pull request may close these issues.

4 participants

@jonathanmos @codecov-commenter @xgouchet @0xnm