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-7797 Anonymous RUM Identifier #2487

Draft
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

maciejburda
Copy link
Member

@maciejburda maciejburda commented Jan 17, 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.

Motivation

This extra property adds ability to link sessions coming from the same device.

Additional Notes

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 (to be filled by reviewers)

  • Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
  • Make sure you discussed the feature or bugfix with the maintaining team in an Issue
  • Make sure each commit and the PR mention the Issue number (cf the CONTRIBUTING doc)

anonymousIdKey,
null,
AnonymousIdentifierReadCallback { anonymousId ->
if (anonymousId == null) {
Copy link
Contributor

@satween satween Jan 17, 2025

Choose a reason for hiding this comment

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

Are you sure wee need this lambda here? 🤔 It basically just add more instructions than saves

Just to consider:
You have an object, implementing DataStoreReadCallback :

object : DataStoreReadCallback<UUID>{

 override fun onSuccess(dataStoreContent: DataStoreContent<UUID>?) {
        core.setAnonymousId(dataStoreContent?.data)
    }

    override fun onFailure() {
       val newAnonymousId = UUID.randomUUID()
       dataStore.setValue(
          anonymousIdKey,
          newAnonymousId,
          0,
          null,
          AnonymousIdentifierSerializer()
       )
       core.setAnonymousId(newAnonymousId)
    }
}

And that's it, no AnonymousIdentifierReadCallback class, no if(anonymousId == null) and rest logic...

@@ -73,6 +73,14 @@ object Rum {

sdkCore.registerFeature(rumFeature)

sdkCore.getFeature(rumFeature.name)?.dataStore?.let {
val anonymousIdentifierManager = AnonymousIdentifierManager(
Copy link
Contributor

Choose a reason for hiding this comment

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

Note

Not sure that we need to create an instance of stateless class just to call one method... Maybe some static function will be better here?

@satween
Copy link
Contributor

satween commented Jan 17, 2025

Running

./local_ci.sh -n -c -t -a  

from the root of the project will help you to overcome detekt problems.

import com.datadog.android.core.persistence.datastore.DataStoreContent
import java.util.UUID

internal interface AnonymousIdentifierManaging {
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps renaming to

Suggested change
internal interface AnonymousIdentifierManaging {
internal interface AnonymousIdentifierManagement {

* @property id a unique identifier for the user, or null
* @property name the name of the user, or null
* @property email the email address of the user, or null
* @property additionalProperties a dictionary of custom properties attached to the current user
*/
data class UserInfo(
val anonymousId: String? = null,
Copy link
Member

Choose a reason for hiding this comment

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

In terms of naming, should we make it more explicit that this is an identifier of the device between sessions? anonymousDeviceId?

},
"anonymous_id": {
"type": "string",
"description": "Identifier of the user across sessions",
Copy link
Member

Choose a reason for hiding this comment

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

In some places it says identifier of the user and in others identifier of the device so I think we need to have a unified definition

@@ -0,0 +1,30 @@
{
"$schema": "http://json-schema.org/draft-07/schema",
Copy link
Member

Choose a reason for hiding this comment

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

Several changes that don't look directly connected to the pr are here due to updating the schemas. Perhaps we could update the schemas in a separate pr

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