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.

Sign up for GitHub

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-1077 Prevent discarding RUM View events #415

Merged
merged 5 commits into from
Feb 22, 2021
Merged

RUMM-1077 Prevent discarding RUM View events #415

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
513fa6f
RUMM-1077 Make the RUMViewEvent mapper return a non-nil RUMViewEvent …
Feb 17, 2021
92688d2
RUMM-1077 Make the RUMViewEvent mapper return a non-nil RUMViewEvent …
Feb 17, 2021
02e6903
Merge branch 'acgh/RUMM-1077-viewevent-data-scrubbing' of https://git…
Feb 18, 2021
a194ed8
RUMM-1077 Update api-surface files
Feb 18, 2021
3625a0c
RUMM-1077 Add reference to the UIKitRUMViewsPredicate API in doc
Feb 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions Sources/Datadog/DatadogConfiguration.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -454,10 +454,10 @@ extension Datadog {
}

/// Sets the custom mapper for `RUMViewEvent`. This can be used to modify RUM View events before they are send to Datadog.
/// - Parameter mapper: the closure taking `RUMViewEvent` as input and expecting `RUMViewEvent` or `nil` as output.
/// The implementation should obtain a mutable version of the `RUMViewEvent`, modify it and return. Returning `nil` will result
/// with dropping the RUM View event entirely, so it won't be send to Datadog.
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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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:

  1. if we mention ViewPredicate here and how it's different than viewMapper, etc. this may be even more confusing in case the user never saw ViewPredicate API before
  2. if we don't mention it, this may be still confusing if the user is already aware of ViewPredicate API

what do you think about that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

public func setRUMViewEventMapper(_ mapper: @escaping (RUMViewEvent) -> RUMViewEvent) -> Builder {
configuration.rumViewEventMapper = mapper
return self
}
Expand Down
2 changes: 1 addition & 1 deletion Sources/Datadog/RUM/Scrubbing/RUMEventsMapper.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

import Foundation

internal typealias RUMViewEventMapper = (RUMViewEvent) -> RUMViewEvent?
internal typealias RUMViewEventMapper = (RUMViewEvent) -> RUMViewEvent
internal typealias RUMErrorEventMapper = (RUMErrorEvent) -> RUMErrorEvent?
internal typealias RUMResourceEventMapper = (RUMResourceEvent) -> RUMResourceEvent?
internal typealias RUMActionEventMapper = (RUMActionEvent) -> RUMActionEvent?
Expand Down
4 changes: 2 additions & 2 deletions Sources/DatadogObjc/DatadogConfiguration+objc.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,10 +231,10 @@ public class DDConfigurationBuilder: NSObject {
}

@objc
public func setRUMViewEventMapper(_ mapper: @escaping (DDRUMViewEvent) -> DDRUMViewEvent?) {
public func setRUMViewEventMapper(_ mapper: @escaping (DDRUMViewEvent) -> DDRUMViewEvent) {
_ = sdkBuilder.setRUMViewEventMapper { swiftEvent in
let objcEvent = DDRUMViewEvent(swiftModel: swiftEvent)
return mapper(objcEvent)?.swiftModel
return mapper(objcEvent).swiftModel
}
}

Expand Down
10 changes: 2 additions & 8 deletions Tests/DatadogTests/Datadog/RUM/Scrubbing/RUMEventsMapperTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,17 +88,13 @@ class RUMEventsMapperTests: XCTestCase {
}

func testGivenMappersEnabled_whenDroppingEvents_itReturnsNil() {
let originalViewEvent: RUMViewEvent = .mockRandom()
let originalErrorEvent: RUMErrorEvent = .mockRandom()
let originalResourceEvent: RUMResourceEvent = .mockRandom()
let originalActionEvent: RUMActionEvent = .mockRandom()

// Given
let mapper = RUMEventsMapper(
viewEventMapper: { viewEvent in
XCTAssertEqual(viewEvent, originalViewEvent, "Mapper should be called with the original event.")
return nil
},
viewEventMapper: nil,
errorEventMapper: { errorEvent in
XCTAssertEqual(errorEvent, originalErrorEvent, "Mapper should be called with the original event.")
return nil
Expand All @@ -114,13 +110,11 @@ class RUMEventsMapperTests: XCTestCase {
)

// When
let mappedViewEvent = mapper.map(event: RUMEvent<RUMViewEvent>.mockWith(model: originalViewEvent))?.model
let mappedErrorEvent = mapper.map(event: RUMEvent<RUMErrorEvent>.mockWith(model: originalErrorEvent))?.model
let mappedResourceEvent = mapper.map(event: RUMEvent<RUMResourceEvent>.mockWith(model: originalResourceEvent))?.model
let mappedActionEvent = mapper.map(event: RUMEvent<RUMActionEvent>.mockWith(model: originalActionEvent))?.model

// Then
XCTAssertNil(mappedViewEvent, "Mapper should return nil.")
XCTAssertNil(mappedErrorEvent, "Mapper should return nil.")
XCTAssertNil(mappedResourceEvent, "Mapper should return nil.")
XCTAssertNil(mappedActionEvent, "Mapper should return nil.")
Expand Down Expand Up @@ -163,7 +157,7 @@ class RUMEventsMapperTests: XCTestCase {

// When
let mapper = RUMEventsMapper(
viewEventMapper: { _ in nil },
viewEventMapper: nil,
errorEventMapper: { _ in nil },
resourceEventMapper: { _ in nil },
actionEventMapper: { _ in nil }
Expand Down
4 changes: 1 addition & 3 deletions Tests/DatadogTests/DatadogObjc/DDConfigurationTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ class DDConfigurationTests: XCTestCase {
objcBuilder.set(rumSessionsSamplingRate: 42.5)
XCTAssertEqual(objcBuilder.build().sdkConfiguration.rumSessionsSamplingRate, 42.5)

objcBuilder.setRUMViewEventMapper { _ in nil }
objcBuilder.setRUMViewEventMapper { $0 }
XCTAssertNotNil(objcBuilder.build().sdkConfiguration.rumViewEventMapper)

objcBuilder.setRUMResourceEventMapper { _ in nil }
Expand Down Expand Up @@ -212,14 +212,12 @@ class DDConfigurationTests: XCTestCase {
environment: "tests"
)

objcBuilder.setRUMViewEventMapper { _ in nil }
objcBuilder.setRUMResourceEventMapper { _ in nil }
objcBuilder.setRUMActionEventMapper { _ in nil }
objcBuilder.setRUMErrorEventMapper { _ in nil }

let configuration = objcBuilder.build().sdkConfiguration

XCTAssertNil(configuration.rumViewEventMapper?(.mockRandom()))
XCTAssertNil(configuration.rumResourceEventMapper?(.mockRandom()))
XCTAssertNil(configuration.rumActionEventMapper?(.mockRandom()))
XCTAssertNil(configuration.rumErrorEventMapper?(.mockRandom()))
Expand Down
7 changes: 6 additions & 1 deletion api-surface-objc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ public class DDConfigurationBuilder: NSObject
public func trackUIKitRUMViews()
public func trackUIKitRUMViews(using predicate: DDUIKitRUMViewsPredicate)
public func trackUIKitActions()
public func setRUMViewEventMapper(_ mapper: @escaping (DDRUMViewEvent) -> DDRUMViewEvent?)
public func setRUMViewEventMapper(_ mapper: @escaping (DDRUMViewEvent) -> DDRUMViewEvent)
public func setRUMResourceEventMapper(_ mapper: @escaping (DDRUMResourceEvent) -> DDRUMResourceEvent?)
public func setRUMActionEventMapper(_ mapper: @escaping (DDRUMActionEvent) -> DDRUMActionEvent?)
public func setRUMErrorEventMapper(_ mapper: @escaping (DDRUMErrorEvent) -> DDRUMErrorEvent?)
Expand Down Expand Up @@ -186,6 +186,7 @@ public class DDRUMViewEventView: NSObject
@objc public var action: DDRUMViewEventViewAction
@objc public var crash: DDRUMViewEventViewCrash?
@objc public var cumulativeLayoutShift: NSNumber?
@objc public var customTimings: [String: NSNumber]?
@objc public var domComplete: NSNumber?
@objc public var domContentLoaded: NSNumber?
@objc public var domInteractive: NSNumber?
Expand All @@ -200,6 +201,7 @@ public class DDRUMViewEventView: NSObject
@objc public var loadingTime: NSNumber?
@objc public var loadingType: DDRUMViewEventViewLoadingType
@objc public var longTask: DDRUMViewEventViewLongTask?
@objc public var name: String?
@objc public var referrer: String?
@objc public var resource: DDRUMViewEventViewResource
@objc public var timeSpent: NSNumber
Expand Down Expand Up @@ -350,6 +352,7 @@ public class DDRUMResourceEventRUMUser: NSObject
@objc public var name: String?
public class DDRUMResourceEventView: NSObject
@objc public var id: String
@objc public var name: String?
@objc public var referrer: String?
@objc public var url: String
public class DDRUMActionEvent: NSObject
Expand Down Expand Up @@ -428,6 +431,7 @@ public class DDRUMActionEventRUMUser: NSObject
@objc public var name: String?
public class DDRUMActionEventView: NSObject
@objc public var id: String
@objc public var name: String?
@objc public var referrer: String?
@objc public var url: String
public class DDRUMErrorEvent: NSObject
Expand Down Expand Up @@ -529,6 +533,7 @@ public class DDRUMErrorEventRUMUser: NSObject
@objc public var name: String?
public class DDRUMErrorEventView: NSObject
@objc public var id: String
@objc public var name: String?
@objc public var referrer: String?
@objc public var url: String
public class DDRUMView: NSObject
Expand Down
7 changes: 6 additions & 1 deletion api-surface-swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ public class Datadog
public func set(rumSessionsSamplingRate: Float) -> Builder
public func trackUIKitRUMViews(using predicate: UIKitRUMViewsPredicate = DefaultUIKitRUMViewsPredicate()) -> Builder
public func trackUIKitActions(_ enabled: Bool = true) -> Builder
public func setRUMViewEventMapper(_ mapper: @escaping (RUMViewEvent) -> RUMViewEvent?) -> Builder
public func setRUMViewEventMapper(_ mapper: @escaping (RUMViewEvent) -> RUMViewEvent) -> Builder
public func setRUMResourceEventMapper(_ mapper: @escaping (RUMResourceEvent) -> RUMResourceEvent?) -> Builder
public func setRUMActionEventMapper(_ mapper: @escaping (RUMActionEvent) -> RUMActionEvent?) -> Builder
public func setRUMErrorEventMapper(_ mapper: @escaping (RUMErrorEvent) -> RUMErrorEvent?) -> Builder
Expand Down Expand Up @@ -233,6 +233,7 @@ public struct RUMViewEvent: RUMDataModel
public let action: Action
public let crash: Crash?
public let cumulativeLayoutShift: Double?
public let customTimings: [String: Int64]?
public let domComplete: Int64?
public let domContentLoaded: Int64?
public let domInteractive: Int64?
Expand All @@ -247,6 +248,7 @@ public struct RUMViewEvent: RUMDataModel
public let loadingTime: Int64?
public let loadingType: LoadingType?
public let longTask: LongTask?
public var name: String?
public var referrer: String?
public let resource: Resource
public let timeSpent: Int64
Expand Down Expand Up @@ -362,6 +364,7 @@ public struct RUMResourceEvent: RUMDataModel
case synthetics = "synthetics"
public struct View: Codable
public let id: String
public var name: String?
public var referrer: String?
public var url: String
public struct RUMActionEvent: RUMDataModel
Expand Down Expand Up @@ -415,6 +418,7 @@ public struct RUMActionEvent: RUMDataModel
case synthetics = "synthetics"
public struct View: Codable
public let id: String
public var name: String?
public var referrer: String?
public var url: String
public struct RUMErrorEvent: RUMDataModel
Expand Down Expand Up @@ -483,6 +487,7 @@ public struct RUMErrorEvent: RUMDataModel
case synthetics = "synthetics"
public struct View: Codable
public let id: String
public var name: String?
public var referrer: String?
public var url: String
public struct RUMConnectivity: Codable
Expand Down