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.

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-1791 WebEventConsumers added #683

Merged

Conversation

buranmert
Copy link
Contributor

@buranmert buranmert commented Dec 8, 2021

What and why?

Datadog browser-sdk integration will send log and RUM events to the native SDK and these events will be decorated with the contextual information of the native app.
That way any app with web view can display its users' session in the same timeline, native + webview.

How?

Once the message is passed from browser-sdk, a JS script will catch it and pass it to either WebLogEventConsumer or WebRUMEventConsumer.
Then they will add information to events and persist them in the filesystem where they wait to be uploaded to Datadog.

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

@buranmert buranmert self-assigned this Dec 8, 2021
@buranmert buranmert force-pushed the buranmert/RUMM-1791-event-consumers branch 2 times, most recently from 1fec047 to 6091458 Compare December 10, 2021 13:53
@buranmert buranmert force-pushed the buranmert/RUMM-1791-event-consumers branch from 6091458 to aeab8c4 Compare December 15, 2021 20:03
@buranmert buranmert changed the title RUMM-1791 WebRUMEventConsumer added RUMM-1791 WebEventConsumers added Dec 20, 2021
Optionals to non-optionals, non-optionals to optionals
ashes to ashes, dust to dust...
WKUserContentController_DatadogTests improved
@buranmert buranmert marked this pull request as ready for review December 23, 2021 10:51
@buranmert buranmert requested a review from a team as a code owner December 23, 2021 10:51
@buranmert buranmert force-pushed the buranmert/RUMM-1791-event-consumers branch from fbe03a6 to 5b03879 Compare December 23, 2021 11:08
@buranmert buranmert requested review from ncreated and maxep December 23, 2021 11:21
@buranmert buranmert mentioned this pull request Dec 24, 2021
3 tasks
Copy link
Member

@ncreated ncreated left a comment

Choose a reason for hiding this comment

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

Needs answers on tracking consent for webview events and more logs on eventual inconsistency of the solution. Currently we only addresses the happy scenario (both Logs and RUM enabled, tracking consent granted and receiving only known events).

@buranmert buranmert force-pushed the buranmert/RUMM-1791-event-consumers branch from 9abf1d7 to e10f099 Compare December 28, 2021 15:33
@buranmert buranmert requested a review from ncreated December 28, 2021 15:34
Comment on lines 49 to 53
if eventType == Constants.eventTypeLog {
logEventConsumer.consume(event: wrappedEvent, eventType: eventType)
try logEventConsumer?.consume(event: wrappedEvent, eventType: eventType)
} else {
rumEventConsumer.consume(event: wrappedEvent, eventType: eventType)
try rumEventConsumer?.consume(event: wrappedEvent, eventType: eventType)
}
Copy link
Member

@ncreated ncreated Dec 29, 2021

Choose a reason for hiding this comment

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

Because we pass Browser events to Logging / RUM feature storage, in case of Logging or RUM being disabled we will be sending Browser SDK events to void. This will result with losing observability and entire set of telemetry from webviews, depending on native SDK configuration. This requires additional handling - @mariusc83 @buranmert @xgouchet what's our strategy for this?

I see 3 options:

  • we can send events to their own storage - independent from Logging / RUM feature;
  • we can inform the user on dropping event due to misconfiguration:
if eventType == Constants.eventTypeLog {
   if let logEventConsumer = logEventConsumer {
      try logEventConsumer.consume(event: wrappedEvent, eventType: eventType)
   } else {
      userLogger.warn(/* warn that event will be lost because Logging is disabled in native SDK */)
   }
} else {
   if let rumEventConsumer = rumEventConsumer {
      try rumEventConsumer(event: wrappedEvent, eventType: eventType)
   } else {
      userLogger.warn(/* warn that event will be lost because RUM is disabled in native SDK */)
   }
}
  • we can register selectively for receiving only Logs / only RUM / both Logs and RUM events from Browser SDK.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ℹ️ we decided to go with userLogger.warn option at today's daily

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ncreated should we warn the user every time a new event is received (that might spam their console)?
or only once when WebEventConsumer is created?

WebRUMEvents are decorated as raw JSONs
instead of serializing them into RUMDataModels
@buranmert buranmert requested a review from ncreated December 30, 2021 12:43
Comment on lines +37 to +40
guard let context = context,
context.sessionID != .nullUUID else {
return event
}
Copy link
Member

Choose a reason for hiding this comment

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

"Given native RUM session rejected by sampler, when receiving event from WebView, it is sent anyway" seems to be critical (it impacts user billing) and not very obvious behaviour (in case of RUM disabled we drop events, but we send events no matter of RUM sampling). I don't see it covered in unit tests, meaning we can easily overlook it and change unawarely.

Missing test case added: sampled out native session
@buranmert buranmert requested a review from ncreated December 31, 2021 09:40
@buranmert buranmert merged commit d69ac33 into feature/hybrid-application Jan 3, 2022

private func map(event: JSON, with context: RUMContext?) -> JSON {
guard let context = context,
context.sessionID != .nullUUID else {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

context.sessionID == .nullID when the session is sampled-out

@buranmert buranmert deleted the buranmert/RUMM-1791-event-consumers branch January 3, 2022 11:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants