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

Adding generateAnonymousId config option #1989

Merged

Conversation

hannah-smartbear
Copy link
Contributor

CHANGES

Added generateAnonymousId config option, which, when set to false:

  • Does not use a previously persisted device ID if there is one
  • Does not generate or store a new device ID
  • However, it also does not wipe any device ID that has been previously stored. I’m not sure if this is something I should add?

I implemented this by returning null early in loadDeviceId function in DeviceIdStore.kt if generateAnonymousId is set to false. This seemed the easiest way to do this with the least amount of files changed.

Where UserStore.kt tries to use the deviceId as the user ID, deviceId should be null anyway if generateAnonymousId config option is false, due to my changes to loadDeviceId. However, there’s one scenario where there the current behaviour is perhaps not ideal:

  • Let’s say a customer previously had default config options for persistUser and generateAnonymousId (i.e. true for both), and they are not setting user info themselves so both device ID and user ID are set to the device ID value
  • The customer then decides they don’t want that Device ID value reported on events, so looks at the docs and concludes that they need to set generateAnonymousId to false.
  • However, since they still have persistUser = true, the device ID will actually still appear on their events in the user tab, but we won’t necessarily know not to add this to events since we don’t know that the user ID stored on the device is actually the device ID and that we shouldn’t use it
    So I’m not sure if there’s anything we can do to prevent this happening, or if we should just include a note in the docs to make this clear? We couldn’t just not return the persisted user if generateAnonymousId is false, since the persisted user ID may be something set by the customer and not the device ID.

TESTING

  • I tested this locally by publishing my changes to Maven local and setting the generateAnonymousId config option in my example app and triggering events
  • I added a unit test to DeviceIdStoreTest.kt to test that a previously persisted device ID is not loaded when generateAnonymousId is set to false
  • I was also thinking that I would like to add a test to check that no new device ID is generated or persisted when generateAnonymousId is set to false, but wasn’t too sure how to do this

@bugsnagbot
Copy link
Collaborator

bugsnagbot commented Mar 5, 2024

Android notifier sizes

Format Size impact of Bugsnag (kB) Size impact of Bugsnag when Minified (kB)
APK 1842.72 1668.07
arm64_v8a 626.95 450.82
armeabi_v7a 561.42 385.29
x86 700.66 524.53
x86_64 671.99 495.86

Generated by 🚫 Danger

@hannah-smartbear hannah-smartbear requested a review from lemnik March 5, 2024 15:19
@hannah-smartbear hannah-smartbear marked this pull request as ready for review March 13, 2024 09:50
Copy link
Contributor

@lemnik lemnik left a comment

Choose a reason for hiding this comment

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

LGTM - With a small suggestion

Comment on lines +261 to +262
file.writeText("{\"id\": \"${ids[0]}\"}")
fileInternal.writeText("{\"id\": \"${ids[1]}\"}")
Copy link
Contributor

Choose a reason for hiding this comment

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

these look like a good candidate for raw string literals:

Suggested change
file.writeText("{\"id\": \"${ids[0]}\"}")
fileInternal.writeText("{\"id\": \"${ids[1]}\"}")
file.writeText("""{"id": "${ids[0]}"}""")
fileInternal.writeText("""{"id": "${ids[1]}"}""")

@lemnik lemnik force-pushed the PLAT-10289/add-generateanonymousid-to-android-config-options branch from a73cdab to 4bf6c5f Compare March 14, 2024 14:27
@lemnik lemnik merged commit a21721c into next Mar 14, 2024
22 of 27 checks passed
@lemnik lemnik deleted the PLAT-10289/add-generateanonymousid-to-android-config-options branch March 14, 2024 15:08
This was referenced Mar 18, 2024
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