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

RUM-7795 Anonymous RUM Identifier #2172

Open
wants to merge 20 commits into
base: develop
Choose a base branch
from

Conversation

maciejburda
Copy link
Member

@maciejburda maciejburda commented Jan 10, 2025

What and why?

Adds capability of tracking anonymous id (enabled by default). This data allows linking sessions coming from the same device.

How?

Following proposal from this RFC (internal) it reuses data store mechanism, and based on configuration it generates and reuses stored identifier.

This identifier is attached to UserInfo object (schema already updated), which is enriching other SDK events.

NOTE: The identifier is read asynchronously from a file, which means RUM events created immediately after calling RUM.enable() will not include it. However, this is not an issue since the anonymous_id belongs to the Session entity. As long as the anonymous_id is present in at least in one event, it is sufficient for linking.

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
  • Add Objective-C interface for public APIs (see our guidelines [internal]) and run make api-surface)

@maciejburda maciejburda changed the title Maciey/rum 7795/anonymous RUM-7795 Anonymous RUM Identifier Jan 10, 2025
@datadog-datadog-prod-us1
Copy link

datadog-datadog-prod-us1 bot commented Jan 10, 2025

Datadog Report

Branch report: maciey/RUM-7795/anonymous-id
Commit report: 777da2e
Test service: dd-sdk-ios

✅ 0 Failed, 1013 Passed, 2754 Skipped, 47.38s Total duration (1m 45.5s time saved)

@maciejburda maciejburda marked this pull request as ready for review January 10, 2025 16:29
@maciejburda maciejburda requested review from a team as code owners January 10, 2025 16:29
mariedm
mariedm previously approved these changes Jan 10, 2025
Copy link
Member

@mariedm mariedm left a comment

Choose a reason for hiding this comment

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

LGMT! Only a few minor comments.

simaoseica-dd
simaoseica-dd previously approved these changes Jan 10, 2025
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.

It looks great overall 👌 - I only left suggestions on naming and documenting the public API.

Requesting a change tho, to add integration unit tests for this feature. Using real instance of the SDK + proxy and asserting that anonymous ID is generated and inserted into events of two RUM sessions started in two instances of the SDK. I can help and pair on this 🙌

@maciejburda maciejburda dismissed stale reviews from mariedm and simaoseica-dd via 7ca3267 January 14, 2025 14:02
Copy link
Contributor

@simaoseica-dd simaoseica-dd left a comment

Choose a reason for hiding this comment

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

Can the the anonymous id naming be consistent?
There is AnonymousIdentifier, AnonymousId and AnonymousID in the code.

@maciejburda
Copy link
Member Author

@simaoseica-dd Thanks for the review. I addressed all the comments.

For naming I went for full AnonymousIdentifier in class / protocol naming anonymousId for properties / functions.

Let me know if it looks more consistent.

@datadog-datadog-prod-us1
Copy link

datadog-datadog-prod-us1 bot commented Jan 24, 2025

Datadog Report

Branch report: maciey/RUM-7795/anonymous-id
Commit report: b6df357
Test service: dd-sdk-ios

✅ 0 Failed, 627 Passed, 3177 Skipped, 27.43s Total duration (2m 2.73s time saved)

simaoseica-dd
simaoseica-dd previously approved these changes Jan 24, 2025
Copy link
Contributor

@simaoseica-dd simaoseica-dd left a comment

Choose a reason for hiding this comment

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

LGTM! 👌

self?.featureScope.set(anonymousId: anonymousId)
} else {
let anonymousId = self?.uuidGenerator.generateUnique().toRUMDataFormat
self?.featureScope.rumDataStore.setValue(anonymousId, forKey: .anonymousId)
Copy link
Member

Choose a reason for hiding this comment

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

We don't need to update the rumDataStore as well if the anonymousId already exists?

Copy link
Contributor

Choose a reason for hiding this comment

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

let anonymousId = anonymousId ?? self?.uuidGenerator.generateUnique().toRUMDataFormat
And then the rumDataStore.setValue and featureScope.set ?

@@ -31,5 +31,6 @@ class RUMConfigurationTests: XCTestCase {
XCTAssertNil(config.longTaskEventMapper)
XCTAssertNil(config.onSessionStart)
XCTAssertNil(config.customEndpoint)
XCTAssertEqual(config.trackAnonymousUser, true)
Copy link
Member

Choose a reason for hiding this comment

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

nit

Suggested change
XCTAssertEqual(config.trackAnonymousUser, true)
XCTAssertTrue(config.trackAnonymousUser)

Copy link
Contributor

@simaoseica-dd simaoseica-dd left a comment

Choose a reason for hiding this comment

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

Some small nits
👍


assertAnonymousIdentifier(isSet: true, in: session2)
let anonymousId2 = session2.views.last?.viewEvents.first?.usr?.anonymousId
XCTAssertEqual(anonymousId1, anonymousId1)
Copy link
Contributor

Choose a reason for hiding this comment

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

typo here. Always asserts.

@@ -330,4 +330,87 @@ class DatadogCoreTests: XCTestCase {
core.flush()
XCTAssertEqual(requestBuilderSpy.requestParameters.count, 0, "It should not send any request")
}

func testItAppendsUserDataIfAnonymousIdentifierExists() {
let core = DatadogCore(
Copy link
Contributor

Choose a reason for hiding this comment

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

nit/ DatadogCoreProxy()? and in the other tests too?

self?.featureScope.set(anonymousId: anonymousId)
} else {
let anonymousId = self?.uuidGenerator.generateUnique().toRUMDataFormat
self?.featureScope.rumDataStore.setValue(anonymousId, forKey: .anonymousId)
Copy link
Contributor

Choose a reason for hiding this comment

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

let anonymousId = anonymousId ?? self?.uuidGenerator.generateUnique().toRUMDataFormat
And then the rumDataStore.setValue and featureScope.set ?

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.

4 participants