Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RUMM-2504: Switch to the SDK v2 storage component #1051

Merged
merged 1 commit into from
Sep 23, 2022

Conversation

0xnm
Copy link
Member

@0xnm 0xnm commented Sep 21, 2022

What does this PR do?

This change does a switch to the Storage implementation written earlier. While it is not used as it should be in the feature configuration per SDK v2 plan, this change helps to minimize the diff of the transition and makes a proof that new component works.

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 a review from a team as a code owner September 21, 2022 09:44
@codecov-commenter
Copy link

codecov-commenter commented Sep 21, 2022

Codecov Report

Merging #1051 (00b79be) into feature/sdkv2 (f913074) will increase coverage by 0.02%.
The diff coverage is 88.24%.

@@                Coverage Diff                @@
##           feature/sdkv2    #1051      +/-   ##
=================================================
+ Coverage          82.83%   82.84%   +0.02%     
=================================================
  Files                306      306              
  Lines              10080    10089       +9     
  Branches            1642     1645       +3     
=================================================
+ Hits                8349     8358       +9     
  Misses              1230     1230              
  Partials             501      501              
Impacted Files Coverage Δ
...id/v2/core/internal/storage/ConsentAwareStorage.kt 96.77% <ø> (-0.03%) ⬇️
.../internal/rum/WebViewRumFilePersistenceStrategy.kt 62.96% <33.33%> (-1.04%) ⬇️
...rnal/persistence/file/batch/BatchFileDataWriter.kt 81.82% <85.71%> (-3.90%) ⬇️
...istence/file/batch/BatchFilePersistenceStrategy.kt 100.00% <100.00%> (ø)
...tadog/android/rum/internal/domain/RumDataWriter.kt 85.71% <100.00%> (+0.53%) ⬆️
.../rum/internal/domain/RumFilePersistenceStrategy.kt 100.00% <100.00%> (ø)
...n/kotlin/com/datadog/android/timber/DatadogTree.kt 50.00% <0.00%> (-5.56%) ⬇️
...g/internal/domain/event/DdSpanToSpanEventMapper.kt 95.92% <0.00%> (-4.08%) ⬇️
...tadog/android/tv/LeanbackViewAttributesProvider.kt 86.21% <0.00%> (-2.68%) ⬇️
...main/kotlin/com/datadog/android/rx/DatadogRxExt.kt 100.00% <0.00%> (ø)
... and 9 more

@xgouchet xgouchet added the size-medium This PR is medium sized label Sep 21, 2022
@0xnm 0xnm force-pushed the nogorodnikov/rumm-2504/switch-to-sdkv2-storage branch from 5659de8 to c059e6b Compare September 21, 2022 12:42
val context = contextProvider.context
val eventId = System.identityHashCode(data).toString()
storage.writeCurrentBatch(context) {
pendingWriteEvents[eventId] = data to byteArray
Copy link
Member

Choose a reason for hiding this comment

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

aren't we concerned that we will keep too much data in HEAP ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question. For now we need to store both event and serialized event mostly because of the RUM, but maybe later on with once RUM will start using write scope to perform writes we can change it.

What is your expectation for the memory pressure during SR? How often and how big are going to be events received? I guess concern is mostly because of the bitmaps?

I would propose to add TODO for now. WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

Yes I am mostly thinking for Bitmaps here, let's benchmark this when we get there and see how it goes.

@0xnm 0xnm force-pushed the nogorodnikov/rumm-2504/switch-to-sdkv2-storage branch from c059e6b to 00b79be Compare September 23, 2022 08:41
@0xnm 0xnm merged commit 757bb25 into feature/sdkv2 Sep 23, 2022
@0xnm 0xnm deleted the nogorodnikov/rumm-2504/switch-to-sdkv2-storage branch September 23, 2022 09:03
@xgouchet xgouchet added this to the 1.16.0 milestone Dec 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size-medium This PR is medium sized
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants