RUMM-1077 Prevent discarding RUM View events #415
Conversation
…to disallow dropping view events
There was a problem hiding this comment.
Looks good 👌.
Could you also add a change suggestion in the PR with RUM docs update? We should add a note there, that returning nil is impossible for view events.
…to disallow dropping view events
…hub.com/DataDog/dd-sdk-ios into acgh/RUMM-1077-viewevent-data-scrubbing
| public func setRUMViewEventMapper(_ mapper: @escaping (RUMViewEvent) -> RUMViewEvent?) -> Builder { | ||
| /// - Parameter mapper: the closure taking `RUMViewEvent` as input and expecting `RUMViewEvent` as output. | ||
| /// The implementation should obtain a mutable version of the `RUMViewEvent`, modify it and return it. | ||
| /// Note that the mapper intentionally prevents from returning a `nil` to drop the `RUMViewEvent` entirely, this ensures that all `RUMViewEvent` are sent to Datadog. |
There was a problem hiding this comment.
if i were a user, that could be a bit confusing to me
i have ViewPredicate where I can keep or drop views and then i also have viewEventMapper which intentionally prevents me from dropping them.
i have a dilemma here:
- if we mention
ViewPredicatehere and how it's different thanviewMapper, etc. this may be even more confusing in case the user never sawViewPredicateAPI before - if we don't mention it, this may be still confusing if the user is already aware of
ViewPredicateAPI
what do you think about that?
There was a problem hiding this comment.
Those are good points and that can look confusing.
My understanding is that the ViewPredicate gives an opportunity to drop views already, and downstream the RUMViewEventMapper will be fed with events based on views that have been already filtered.
Before that PR, we give the opportunity (again) to drop view related data at the RUMViewEventMapper, I think it can create a confusing pipeline (the other way around regarding your concerns).
My point is that ViewPredicate let's the user code filter out views, there will be no events for already filtered out views and RUMViewEventMapper will only receive events for views that have been chosen already.
Does that make sense?
That being said, maybe it would help to mention the UIKitRUMViewsPredicate here for context.
There was a problem hiding this comment.
I'd vote for mentioning the UIKitRUMViewsPredicate in there.
Using this scrubbing API to ignore particular UIViewControllers from being tracked is a misuse of the SDK APIs. It means the user doesn't understand the SDK auto instrumentation & scrubbing APIs well and we should guide him towards the success.
That said, this change to "view events cannot be dropped" makes the contract between auto instrumentation & scrubbing APIs even more solid and puts the user on pathway to success.
What and why?
My understanding is that the view event opens a parent-child relationship with the subsequent Error/Resource/Action events, and dropping a view event would cause "leaks" of Error/Resource/Action events.
Thus a solution is to forbid dropping view events while keeping the ability to redact their properties and drop "children" events.
How?
In this PR we change the
RUMViewEventMapperAPI to statically prevent from droppingRUMViewEventby requiring a non-optional return type.As this is statically enforced, we also remove a few checks in existing test cases as they would not compile nor make sense anymore.
Review checklist