-
Notifications
You must be signed in to change notification settings - Fork 134
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-2745 Decouple Webview Tracking #1086
Conversation
Datadog ReportBranch report: ✅ |
internal final class GlobalRUMCommandSubscriber: RUMCommandSubscriber { | ||
func process(command: RUMCommand) { | ||
let subscriber = Global.rum as? RUMCommandSubscriber | ||
subscriber?.process(command: command) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a fan of this, but it helps for injecting a RUMCommandSubscriber
instance for testing. Ultimately, The Global.rum
and this class will be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe worth documenting the usage and plan in the header comment?
internal typealias JSON = [String: Any] | ||
|
||
/// Receiver to consume a RUM event coming from Browser SDK. | ||
internal final class WebViewEventReceiver { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic is migrated from WebRUMEventConsumer
.
907fda8
to
618ec72
Compare
4f2a1a1
to
fad9c7b
Compare
618ec72
to
cf8c6ae
Compare
fad9c7b
to
fc17e83
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
internal final class GlobalRUMCommandSubscriber: RUMCommandSubscriber { | ||
func process(command: RUMCommand) { | ||
let subscriber = Global.rum as? RUMCommandSubscriber | ||
subscriber?.process(command: command) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe worth documenting the usage and plan in the header comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well done 🎯. Happy to see remaining V1 context gone 💪 and the use of V2 things for further separation of Features.
Everything makes total sense 👍 and AnyCodable
pays-off well. I left few minor comments, but approve it anyway.
/// `DatadogV1Context` can be safely captured during component initialization. It will never change during component's lifespan, meaning that: | ||
/// - exposed static configuration won't change; | ||
/// - bundled provider references won't change (although the value they provide will be different over time). | ||
internal struct DatadogV1Context { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🍾 Great to see it gone 👋🙂.
Sources/Datadog/FeaturesIntegration/WebView/WebEventBridge.swift
Outdated
Show resolved
Hide resolved
Tests/DatadogTests/Datadog/Logging/LoggingMessageReceiverTests.swift
Outdated
Show resolved
Hide resolved
Datadog ReportBranch report: ✅ |
What and why?
The web-view tracking no longer depends on the
DatadogV1Context
and will send raw logs and events through the message-bus.The
DatadogV1Context
can now be removed as well as its dependencies:CarrierInfoProvider
NetworConnectionInfoProvider
AppVersionProvider
DateCorrector
How?
The bridge used to inject native context in Browser events, this responsibility moves to the Feature message receiver. The
WebEventBridge
now send raw Browser events on the bus, the Log and RUM receiver will be able to inject current context (e.g ddTags, or RUM context) before writing it tocore
.Review checklist
Custom CI job configuration (optional)