RUMM-3151 feat: reduce number of view updates by looking ahead duplicate events…#1328
Conversation
32f1649 to
6c541e5
Compare
Datadog ReportBranch report: ✅ |
maciejburda
left a comment
maciejburda
left a comment
There was a problem hiding this comment.
Did a first pass. Looking good. Some minor suggestions 🙌
…m payload ### 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.
4b1a196 to
84beb87
Compare
maxep
left a comment
maxep
left a comment
There was a problem hiding this comment.
Looks very good, nice work!
I've left a blocking change about separating RUM specifics from DatadogInternal to facilitate merging with v2 👍
ncreated
left a comment
ncreated
left a comment
There was a problem hiding this comment.
Looks definitely good 👌👍, but I left few points with one blocker.
ncreated
left a comment
ncreated
left a comment
There was a problem hiding this comment.
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 "
1nsviews 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 🙌.
| /// Metadata associated with the event. | ||
| /// Metadata is optional and may be `nil` but of very small size. | ||
| /// This allows us to skip resource intensive operations in case such | ||
| /// as filtering of the events. | ||
| public let metadata: Data? |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
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.
There was a problem hiding this comment.
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.
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. |
ganeshnj
changed the title
60fc50e to
00a1e06
Compare
4 participants
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
Custom CI job configuration (optional)