RUM-6039 Fix StrictMode warning regarding I/O disk operation on main thread

[mariusc83]

What does this PR do?

Fixes: #2229

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)

[mariusc83]

I wonder why do we need to create the dirs on the main thread, maybe the owner of this code can give more info while reviewing this code. Tks !!.

[0xnm]

you are the original owner of this logic :) aa1ff53#diff-11c9203b7b745ca318ece104c990b0d0d70353ecee0d6be0ec15574c31a2844aR52

[mariusc83]

yes and no, I didn't wrote that function allowThreadDiskRead, I think @xgouchet did ;)

[mariusc83]

Check line 54 and 56 ;)

[0xnm]

I'm referencing the code to create directories, not to allow I/O. This code to call directories creation on the main thread was since the creation of NDK module.

[mariusc83]

yup I am getting to understand why, this is going inside the NDK but I don't see a reason why we should just not pass the path to the ndk and create the file inside when we actually write the first content. What do you think ? Not sure if it is a change to do it now or maybe just keep it as it is ?

[0xnm]

This path is also used in DatadogNdkCrashHandler, so we need to make sure that any change we do to this logic is compatible.

We maybe indeed can create it only when crash happens, the downside is that we will lose the time on the folder creation during the crash itself, when time is precious, but maybe the time penalty of such call is negligible.

[mariusc83]

yes that's the reason why I am afraid as when the signal is handle you are in a different state it is not the same as in the JVM crash state. You have so little time to do something then the process is killed so you need to be very efficient there, I think for this main reason I prefer in this case to have that folder created already.

[0xnm]

Side note: I'm not even sure what is the point of having directory name here. With what we currently have it will be essentially (value of operationName):writeCurrentBatchrum-pending-v2 or writeCurrentBatchrum-v2.

Assuming that tracking consent information is probably useless, we are interested mostly in the feature name, which is a part of MetricsDispatcher already, so can be removed from here and be an additional attribute of all the events sent. But this performance metric is not using MetricsDispatcher, but InternalLogger, which is confusing.

It would be much more sense to have operation name = writeCurrentBatch and feature name attached as an attribute.

[mariusc83]

That's the thing I asked on the original thread in Slack but no - one answered so I said to go with this and discuss it here maybe.

[mariusc83]

maybe we could just use feature name here....it will remove a lot of boiler code, what do you all think ? The only thing is that we will miss the folder type ? Granted/Pending. For that we can also pass the tracking consent in the operationName.

[0xnm]

I would avoid growing constructor much. Also what is the point of having tracking consent here?

[0xnm]

Maybe add enrich MetricsDispatcher? Implementation already has everything needed: logger and feature name,

[mariusc83]

Waiting for more opinions here before getting into it but I think it is the right call instead of using the root dir name.

[mariusc83]

the only thing is to not affect the metric which is based on the operationName in the dashboards

[0xnm]

It is our internal metric, so we can break it at any time.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 55.55556% with 4 lines in your changes missing coverage. Please review.

Project coverage is 69.93%. Comparing base (a115df3) to head (1947770).
Report is 14 commits behind head on develop.

Files with missing lines	Patch %	Lines
...n/kotlin/com/datadog/android/core/StrictModeExt.kt	0.00%	4 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #2284      +/-   ##
===========================================
- Coverage    70.03%   69.93%   -0.10%     
===========================================
  Files          732      732              
  Lines        27228    27204      -24     
  Branches      4580     4571       -9     
===========================================
- Hits         19069    19024      -45     
- Misses        6883     6895      +12     
- Partials      1276     1285       +9     

Files with missing lines	Coverage Δ
...d/core/internal/persistence/ConsentAwareStorage.kt	97.62% <100.00%> (+1.19%)	⬆️
...core/internal/persistence/file/FileOrchestrator.kt	100.00% <ø> (ø)
...ence/file/advanced/ConsentAwareFileOrchestrator.kt	96.77% <100.00%> (+0.11%)	⬆️
...al/persistence/file/batch/BatchFileOrchestrator.kt	94.08% <100.00%> (+0.04%)	⬆️
.../persistence/file/single/SingleFileOrchestrator.kt	80.00% <100.00%> (+1.43%)	⬆️
...dog/android/ndk/internal/NdkCrashReportsFeature.kt	69.64% <100.00%> (ø)
...n/kotlin/com/datadog/android/core/StrictModeExt.kt	50.00% <0.00%> (-50.00%)	⬇️

... and 34 files with indirect coverage changes