Skip to content

Conversation

@0xnm
Copy link
Member

@0xnm 0xnm commented Jun 12, 2025

What does this PR do?

The same as #2718, just branch moved directly to the repo.

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)

@0xnm 0xnm requested review from a team as code owners June 12, 2025 08:16
@0xnm 0xnm force-pushed the xgouchet/RUM-6171/FileObserver branch from 6d02b12 to ecb5cc6 Compare June 12, 2025 08:32
@codecov-commenter
Copy link

codecov-commenter commented Jun 12, 2025

Codecov Report

❌ Patch coverage is 95.91837% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.31%. Comparing base (7606b1a) to head (441a603).

Files with missing lines Patch % Lines
...al/persistence/file/batch/BatchFileOrchestrator.kt 95.92% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #2720      +/-   ##
===========================================
+ Coverage    71.28%   71.31%   +0.03%     
===========================================
  Files          880      880              
  Lines        32360    32372      +12     
  Branches      5457     5461       +4     
===========================================
+ Hits         23066    23083      +17     
- Misses        7736     7745       +9     
+ Partials      1558     1544      -14     
Files with missing lines Coverage Δ
...al/persistence/file/batch/BatchFileOrchestrator.kt 94.61% <95.92%> (+1.71%) ⬆️

... and 36 files with indirect coverage changes

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

mariusc83
mariusc83 previously approved these changes Jun 18, 2025
@kikoveiga kikoveiga force-pushed the xgouchet/RUM-6171/FileObserver branch from ecb5cc6 to 3fbb972 Compare December 9, 2025 23:58
@kikoveiga kikoveiga marked this pull request as draft December 10, 2025 00:40
@datadog-datadog-prod-us1
Copy link

datadog-datadog-prod-us1 bot commented Dec 10, 2025

🎯 Code Coverage
Patch Coverage: 94.12%
Overall Coverage: 66.37% (+0.00%)

View detailed report

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: 441a603 | Docs | Datadog PR Page | Was this helpful? Give us feedback!

@kikoveiga kikoveiga force-pushed the xgouchet/RUM-6171/FileObserver branch from fc7afba to 0e1acc1 Compare December 12, 2025 16:47
@kikoveiga kikoveiga force-pushed the xgouchet/RUM-6171/FileObserver branch 2 times, most recently from afa1dd3 to d5d41e3 Compare December 22, 2025 11:40
@kikoveiga kikoveiga force-pushed the xgouchet/RUM-6171/FileObserver branch from b257b6c to 441a603 Compare December 22, 2025 12:15
@kikoveiga kikoveiga marked this pull request as ready for review December 22, 2025 15:37
Copy link
Member Author

@0xnm 0xnm left a comment

Choose a reason for hiding this comment

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

Looks good! It is worth to do some benchmarks to compare the old and new ways of handling batch files in different scenarios. For example, when there is no batch files, 100 batch files, 10000 batch files.

Comment on lines +220 to +222
@Synchronized
private fun startFileObserverIfNotStarted() {
if (isFileObserverStarted.compareAndSet(false, true)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

In theory CAS operation isFileObserverStarted.compareAndSet(false, true) already guards the logical branch from parallel execution, so @Synchronized is not needed. Do I miss something?


@Suppress("LiftReturnOrAssignment", "ReturnCount")
@Suppress("ReturnCount")
private fun isRootDirValid(): Boolean {
Copy link
Member Author

Choose a reason for hiding this comment

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

This will be called from getRootDir <- ... <- SdkFeature.prepareStorage, meaning it will be called on the caller thread. In most of the cases it will be main thread during app startup, so we need to make sure that these changes don't have significant impact perf-wise at the SDK initialization.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants