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-2029 Reduce the number of intermediate view events in RUM payloads #815

Merged
merged 8 commits into from
Apr 14, 2022

Conversation

ncreated
Copy link
Member

@ncreated ncreated commented Apr 12, 2022

What and why?

📦 This PR reduces the number of view events written to RUM batches. With this change, the SDK will send smaller RUM payloads with the same amount of information.

Before, the SDK was issuing a new view update on each significant event spotted in RUM, e.g. new resource stopped or next action tracked. It was allowing similar updates to be written into single RUM payload. In following example, A.1., A.2 and A.3 updates are sent in the same batch, even though their informaton is contained in A.4:

bad

The same applies to B.1. in the second batch.

Current architecture makes it difficult to solve this problem entirely (avoid sending A.1 and B.1). While waiting for V2, this PR applies a partial improvement, which should bring significant benefit with not much cost.

Now, the SDK will skip sending updates which happen too frequently. A basic heuristic is implemented:

  • send (accept) view update if it's the first or last update within a view;
  • send intermediate view update only if it happened more than 30s after last accepted update.

This removes A.2 and A.3 updates from the sample payload:
better

How?

I added basic RUMViewUpdatesThrottler which implements described heuristic:

internal protocol RUMViewUpdatesThrottlerType {
    func accept(event: RUMViewEvent) -> Bool
}

It is used in RUMViewScope and samples view events before passing to the writer.

This change has significant impact on RUMMonitorTests.swift, because some of its tests depend on the exact number of view updates. This problem is solved by injecting NoOpRUMViewUpdatesThrottler() into tested monitor. This required a bit of refactoring in RUMMonitor.initialize() sequence - done in two separate commits for easy code review.

Alternatively, instead of using no-op sampler, we could refactor these unit tests to not depend on the number of view updates - in a similar fashion that we did for integration tests in #811. However, knowing that we will soon change RUMMonitorTests significantly for V2, I decided to go with the no-op approach, which seems easier.

Review checklist

  • Feature or bugfix MUST have appropriate tests (unit, integration)
  • Make sure each commit and the PR mention the Issue number or JIRA reference
  • Add CHANGELOG entry for user facing change.

to unlock the possibility of partial mocking `RUMMonitor's` downstream logic.
…ure`

to isolate `RUMDependencies` creation into separate method. This way we can
partially replace some dependencies in certain tests.
by mocking RUM view updates sampler with no-op implementation.
@ncreated ncreated self-assigned this Apr 12, 2022
@ncreated ncreated changed the title RUMM-2029 Reduce number of RUM view updates RUMM-2029 Reduce the number of intermediate view events in RUM payloads Apr 13, 2022
@ncreated ncreated marked this pull request as ready for review April 13, 2022 09:28
@ncreated ncreated requested a review from a team as a code owner April 13, 2022 09:29
Copy link
Member

@maxep maxep left a comment

Choose a reason for hiding this comment

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

This looks great!! very nice clean up and refactor as well 👏
I left a suggestion about naming.


import Foundation

internal protocol RUMViewUpdatesSamplerType {
Copy link
Member

Choose a reason for hiding this comment

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

Can we rename this to throttler instead of sampler? it can be confused with sampler we apply to session.

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 point 👍. Renamed to "throttler" with accept(event:) API:

internal protocol RUMViewUpdatesThrottlerType {
    func accept(event: RUMViewEvent) -> Bool
}

@ncreated ncreated merged commit 1803150 into develop Apr 14, 2022
@ncreated ncreated deleted the ncreated/RUMM-2029-reduce-number-of-view-updates branch April 14, 2022 15:42
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.

2 participants