RUMM-3151 feat: reduce number of view updates by looking ahead duplicate events…

[ganeshnj]

What and why?

There are several view updates happens which are not needed. These view updates are reduced by the backend anyway, but we could reduce the number of view updates by filtering them out from the payload.

How?

While saving RUMViewEvent to disk, we save the metadata along with it. This allows us to access key information without decoding the whole event. While preparing the request body, we filter out the events which are duplicate and will be sent in the same batch.

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 changes

Custom CI job configuration (optional)

• Run unit tests
• Run integration tests
• Run smoke tests

[datadog-datadog-prod-us1]

Datadog Report

Branch report: ganeshnj/feat/reduce-view-updates
Commit report: 00a1e06

✅ dd-sdk-ios: 0 Failed, 0 New Flaky, 2191 Passed, 0 Skipped, 15m 9.22s Wall Time

[maciejburda]

Did a first pass. Looking good. Some minor suggestions 🙌

[maxep]

Looks very good, nice work!

I've left a blocking change about separating RUM specifics from DatadogInternal to facilitate merging with v2 👍

[ncreated]

Looks definitely good 👌👍, but I left few points with one blocker.

[maxep]

Looks great !!

[ncreated]

Nice work @ganeshnj 👌. Before we can go with merge:

• Please update the PR and branch name to our convention (notably, we must reference to RUMM-3151 for future visibility of "why?" we decided on that change)
• What about the second part from Definition Of Done in RUMM-3151? Without dropping the old "view update throttling" logic we are not solving the "1ns views problem" stated in the spec. With this PR we drop unnecessary updates, but we still don't let the valid ones in. It is still an increment what we have here, so we can tackle this part separately (it will imply quite impactful change in tests). Happy to chat about this to give you more context 🙌.

[ncreated]

nit/ Because we pass metadata in write<E: Encodable, M: Encodable>(event: E, metadata: M) the feature doesn't know the actual detail of metadata: Data encoding. We should clearly state in this comment that metadata: Data? is JSON encoded. Actually, the same comment should be applied to data: Data field.

[ganeshnj]

Actually I disagree with it, the JSON part comes from the FileWriter

Here Event is just an intermediately representation, which starts from FileWriter because we writing the metadata in JSON. (this can be changed to anything per needs).

https://github.com/DataDog/dd-sdk-ios/blob/develop/Sources/Datadog/DatadogCore/Storage/Writing/FileWriter.swift#L50

Then, it is feature specific filters, in this case RUMViewEventsFilter

internal struct RUMViewEventsFilter {
    let decoder: JSONDecoder

    init(decoder: JSONDecoder = JSONDecoder()) {
        self.decoder = decoder
    }

which knows metadata is JSON (FileWriter is using JSONEncoder). As we discussed, how Event is decoded is totally transparent to the Core logic of the SDK.

[maxep]

Currently, responsibilities are not properly assigned: Core owns the encoding while Features are responsible of decoding. JSON encoding/decoding is only a convention between the core and Features, so I think it should be mentioned in comments.

Ultimately, I think we should transfer responsibility of encoding to the Features, so they are free to use the serialisation of their choices (JSON, TLV, protobuf, ...). Not a change request of this PR but something to consider in the future.

[ganeshnj]

Nice work @ganeshnj 👌. Before we can go with merge:

• Please update the PR and branch name to our convention (notably, we must reference to RUMM-3151 for future visibility of "why?" we decided on that change)
• What about the second part from Definition Of Done in RUMM-3151? Without dropping the old "view update throttling" logic we are not solving the "1ns views problem" stated in the spec. With this PR we drop unnecessary updates, but we still don't let the valid ones in. It is still an increment what we have here, so we can tackle this part separately (it will imply quite impactful change in tests). Happy to chat about this to give you more context 🙌.

I realized the branch name as soon as I put the PR for review, the problem with branch name, I have to re-do the PR.

For the 1ns issue - I want to tackle in a separate PR and not convolute this PR (keep it filtering specific). Let's chat about 1ns thing, will ping you on side.

[ncreated]

🏆