Skip to content

RUM-321: Introduce view event filtering in upload pipeline, remove view event throttling in write pipeline#1678

Merged
0xnm merged 4 commits into
developfrom
nogorodnikov/rum-321/reduce-view-events-in-upload-pipeline
Oct 26, 2023
Merged

RUM-321: Introduce view event filtering in upload pipeline, remove view event throttling in write pipeline#1678
0xnm merged 4 commits into
developfrom
nogorodnikov/rum-321/reduce-view-events-in-upload-pipeline

Conversation

@0xnm

@0xnm 0xnm commented Oct 19, 2023

Copy link
Copy Markdown
Member

What does this PR do?

This is the counterpart of DataDog/dd-sdk-ios#1328 for Android.

This PR filters out RUM View events during the upload process following the logic that batch should have only one RUM view event for a given ID and this event should have maximum documentVersion property value. To support that we introduce RUM View event metadata which will be attached to every view event written and read back during the upload cycle.

View throttling logic in RumViewScope is removed.

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 October 19, 2023 12:31
Comment on lines +35 to +38
NullPointerException::class,
ClassCastException::class,
IllegalStateException::class,
NumberFormatException::class

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Note

It might be relevant to add as a comment what situation those exception could rise from?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I changed the way we are doing the parsing, so that it looks now the same as for other RUM events and the only exception thrown is JsonParseException. Let me know what you think.

mariusc83
mariusc83 previously approved these changes Oct 23, 2023
@codecov-commenter

codecov-commenter commented Oct 23, 2023

Copy link
Copy Markdown

Codecov Report

Merging #1678 (4a3f24c) into develop (4744e9d) will increase coverage by 0.01%.
The diff coverage is 86.02%.

@@             Coverage Diff             @@
##           develop    #1678      +/-   ##
===========================================
+ Coverage    83.70%   83.71%   +0.01%     
===========================================
  Files          459      462       +3     
  Lines        15770    15843      +73     
  Branches      2360     2363       +3     
===========================================
+ Hits         13200    13262      +62     
+ Misses        1945     1943       -2     
- Partials       625      638      +13     
Files Coverage Δ
...lin/com/datadog/android/rum/internal/RumFeature.kt 92.51% <100.00%> (-0.54%) ⬇️
...um/internal/domain/event/RumEventMetaSerializer.kt 100.00% <100.00%> (ø)
...id/rum/internal/domain/event/RumViewEventFilter.kt 100.00% <100.00%> (ø)
.../android/rum/internal/domain/scope/RumViewScope.kt 95.06% <ø> (-0.19%) ⬇️
...adog/android/rum/internal/net/RumRequestFactory.kt 97.92% <100.00%> (+0.09%) ⬆️
.../internal/domain/event/RumEventMetaDeserializer.kt 92.31% <92.31%> (ø)
...tadog/android/rum/internal/domain/RumDataWriter.kt 88.10% <87.50%> (+1.89%) ⬆️
.../android/rum/internal/domain/event/RumEventMeta.kt 68.75% <68.75%> (ø)

... and 13 files with indirect coverage changes

@0xnm
0xnm force-pushed the nogorodnikov/rum-321/reduce-view-events-in-upload-pipeline branch from de31563 to 4a3f24c Compare October 25, 2023 09:06
@0xnm
0xnm merged commit ad4d00a into develop Oct 26, 2023
@0xnm
0xnm deleted the nogorodnikov/rum-321/reduce-view-events-in-upload-pipeline branch October 26, 2023 09:47
@xgouchet xgouchet added this to the 2.3.0 milestone Dec 13, 2023
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.

4 participants