[Amplitude] add device_manufacturer from user agent #2025
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
For some devices (iPhones for instance), Amplitude requires a `device_manufacturer` property in order to compute their native property `Device Type` and `Device Family` According to [their SDK](https://github.com/amplitude/Amplitude-TypeScript/blob/v1.x/packages/plugin-user-agent-enrichment-browser/src/user-agent-enrichment-plugin.ts#L26-L38) I've done the following changes: - added `device_manufacturer` based on `device.vendor` - updated `device_model` to first use `device.model` before falling back to `os.name`. As we can see in the snapshots tests, the `device_model` property makes more sense to be `iPhone` instead of `iOS` (which is already the `os_name`
|
hi @ardeois - our partner engineer who reviews these PRs is currently out of office but will be back Thursday. If there is absolute urgency behind this PR, or you have any questions, let me know. |
|
Hi @tcgilbert , thanks for looking into this. No worries if he's back Thursday. Thank you ! |
joe-ayoub-segment
approved these changes
May 16, 2024
|
hi @ardeois - PR deployed. Please confirm the change is good! |
|
@joe-ayoub-segment
Thanks a lot for merging the PR! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
For some devices (iPhones for instance), Amplitude requires a
device_manufacturerproperty in order to compute their native propertyDevice TypeandDevice FamilyAccording to their SDK I've done the following changes:
device_manufacturerbased ondevice.vendordevice_modelto first usedevice.modelbefore falling back toos.name. As we can see in the snapshots tests, thedevice_modelproperty makes more sense to beiPhoneinstead ofiOS(which is already theos_nameNote this
device_manufacturerwas recommended directly from Amplitude's support.Screenshots
Safari iOS
Before
After
Chrome Mac OS
Before
After
Testing
Include any additional information about the testing you have completed to
ensure your changes behave as expected. For a speedy review, please check
any of the tasks you completed below during your testing.
Tested end-to-end using the local server[Segmenters] Tested in the staging environment