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

Jump to bottom

RUM-984 fix: Drop WebView RUM events when mobile session is not sampled #1502

Merged
merged 3 commits into from
Oct 12, 2023
Merged

RUM-984 fix: Drop WebView RUM events when mobile session is not sampled #1502

merged 3 commits into from
Oct 12, 2023

Conversation

ncreated
Copy link
Member

@ncreated ncreated commented Sep 29, 2023 •
edited
Loading

What and why?

📦 This PR fixes the problem of sampling being not respected for WebView events. With this change, if mobile RUM session is not sampled, all WebView events will be dropped. Previously, these events were sent with their original (web) session ID.

How?

Extra context for code reviewers: Browser RUM events are received from WebView in DatadogWebViewTracking. Then, they are sent to DatadogRUM through message bus using [String: Any] baggage. RUM replaces application.id and session.id in these events to "link" them to mobile RUM session.

No change to DatadogWebViewTracking.

DatadogRUM now checks if current session is sampled (determined by presence of "rum" baggage in DatadogContext):

  • if yes → web event is linked to mobile session and sent;
  • if not → web event is dropped.

Changes to DatadogRUMTests:

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
  • Add CHANGELOG entry for user facing changes

Custom CI job configuration (optional)

  • Run unit tests for Core, RUM, Trace, Logs, CR and WVT
  • Run unit tests for Session Replay
  • Run integration tests
  • Run smoke tests
  • Run tests for tools/

@ncreated ncreated self-assigned this Sep 29, 2023
@ncreated ncreated force-pushed the ncreated/RUM-984/sample-webview-rum-events branch from 527103a to 62f02eb Compare September 29, 2023 09:40
@datadog-datadog-prod-us1
Copy link

datadog-datadog-prod-us1 bot commented Sep 29, 2023 •
edited
Loading

Datadog Report

Branch report: ncreated/RUM-984/sample-webview-rum-events
Commit report: 62f02eb

✅ dd-sdk-ios: 0 Failed, 0 New Flaky, 2616 Passed, 0 Skipped, 29m 57.88s Wall Time

@ncreated ncreated changed the title RUM-984 Drop WebView RUM events when mobile session is not sampled RUM-984 fix: Drop WebView RUM events when mobile session is not sampled Sep 29, 2023
@ncreated ncreated marked this pull request as ready for review September 29, 2023 10:21
@ncreated ncreated requested a review from a team as a code owner September 29, 2023 10:21
maciejburda
maciejburda previously approved these changes Sep 29, 2023
View reviewed changes
Copy link
Member

@maciejburda maciejburda left a comment

Choose a reason for hiding this comment

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

Great work, especially on tests side.

LGTM

DatadogCore/Tests/Datadog/InternalProxyTests.swift
@@ -11,7 +11,7 @@ import DatadogInternal
@testable import DatadogCore

class InternalProxyTests: XCTestCase {
let telemetry = TelemetryMock()
let telemetry = TelemetryReceiverMock()
Copy link
Member

Choose a reason for hiding this comment

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

what's the difference here? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

The TelemetryMock was conforming to two distinct protocols: Telemetry and FeatureMessageReceiver. Both have different purpose, so it didn't make sense to have a single mock implementation. I made a split:

  • TelemetryMock: Telemetry
  • TelemetryReceiverMock: FeatureMessageReceiver

With no split, I almost ended up re-implementing this mock because it wasn't obvious at first glance how to spy core telemetry in tests.

DatadogCore/Tests/Matchers/JSONDataMatcher.swift
@@ -6,6 +6,7 @@

import Foundation
import XCTest
import TestUtilities
Copy link
Member

Choose a reason for hiding this comment

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

nice improvement moving these to TestUtilities! :)

@ncreated ncreated force-pushed the ncreated/RUM-984/sample-webview-rum-events branch 2 times, most recently from 74551a9 to b304245 Compare October 11, 2023 14:43
@datadog-datadog-prod-us1
Copy link

datadog-datadog-prod-us1 bot commented Oct 11, 2023 •
edited
Loading

Datadog Report

Branch report: ncreated/RUM-984/sample-webview-rum-events
Commit report: bac1cbe

✅ dd-sdk-ios: 0 Failed, 0 New Flaky, 2657 Passed, 0 Skipped, 28m 36.44s Wall Time

@ncreated ncreated force-pushed the ncreated/RUM-984/sample-webview-rum-events branch from b304245 to bac1cbe Compare October 11, 2023 15:32
@ncreated ncreated requested review from maxep and maciejburda October 12, 2023 07:06
maxep
maxep approved these changes Oct 12, 2023
View reviewed changes
Copy link
Member

@maxep maxep left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for re-working the tests 🙏

@ncreated ncreated merged commit bc9150c into develop Oct 12, 2023
@ncreated ncreated deleted the ncreated/RUM-984/sample-webview-rum-events branch October 12, 2023 08:02
@ncreated ncreated mentioned this pull request Oct 17, 2023
Merged
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maxep maxep maxep approved these changes

@maciejburda maciejburda Awaiting requested review from maciejburda

Assignees

@ncreated ncreated

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ncreated @maxep @maciejburda