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

RUMM-2195: Add OS and device information properties to the RUM schema #62

Conversation

0xnm
Copy link
Member

@0xnm 0xnm commented May 24, 2022

This change adds OS and device information properties to the RUM schema to stop relying on the User Agent parsing for mobile devices, because UA parsing is not quite reliable as a result of the wide range of UA formats which can be found across different Mobile/Tablet/TV/etc. devices.

This change is mostly backfill of the backend logic which extracts the following from the parsed UA:

  • os.name
  • os.version
  • os.version_major
  • device.type
  • device.name
  • device.model
  • device.brand

All the fields of os property are required in assumption that it is always possible to get those values in any runtime.

Only type field of device is required, because SDK running on the desktop, for example, probably may not have any info about name, model, brand (or they can be not very representative).

device.type is taken from what we have already in the dashboard, with the only difference that Appliance type is replaced by the TV, because we have now a stronger focus on the TV platforms and any other device from appliance type can be marked as other.

@0xnm 0xnm requested review from a team as code owners May 24, 2022 08:42
Comment on lines +248 to +249
"gaming_console",
"bot",
Copy link
Member

Choose a reason for hiding this comment

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

Do we have an accurate way to detect those natively? Also on the naming, I might use gaming_system, but not sure about this.

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess gaming_console may be deducted by having Xbox, Playstation, etc. in the device name or brand.

Not sure about Bot usage (maybe synthetics or case of web crawler?), but we have such type on the backend, so I decided to keep it just in case.

Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we only add them once we need it? I think for now we can only use mobile, tablet and tv.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added them all just in case if Browser SDK start using these properties as well, because this is what I see for example for the rum-app:

image

But I'm ok with shrinking this list as well.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @0xnm , these are the values available in the backend and since the browser (also using this rum format) can run on those, we need to be compatible

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with being exhaustive from the start or add it as needed. FYI we don't plan to provide those properties via the Browser SDK.

Copy link
Collaborator

Choose a reason for hiding this comment

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

this is the definition of this enum backend side

Copy link
Collaborator

@louisj-dd louisj-dd May 31, 2022

Choose a reason for hiding this comment

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

i think you should try to replicate the backend enum values as much as possible nevermind i think it's fine this way as it keeps schema syntax consistent

Comment on lines +236 to +238
"required": [
"type"
],
Copy link
Member

Choose a reason for hiding this comment

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

Unless we upgrade the format version, all new fields are optionals

Copy link
Member Author

Choose a reason for hiding this comment

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

os and device parent fields are optional, so this is required rule only for the new optional parent fields, shouldn't break anything

Comment on lines +210 to +214
"required": [
"name",
"version",
"version_major"
],
Copy link
Member

Choose a reason for hiding this comment

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

Unless we upgrade the format version, all new fields are optionals

"properties": {
"name": {
"type": "string",
"description": "Operating system name, ex. Android, iOS",
Copy link
Member

Choose a reason for hiding this comment

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

nit: I believe it's not the right abbreviation but I might be wrong 🙃

Suggested change
"description": "Operating system name, ex. Android, iOS",
"description": "Operating system name, e.g. Android, iOS",

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, you are right

Comment on lines +248 to +249
"gaming_console",
"bot",
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we only add them once we need it? I think for now we can only use mobile, tablet and tv.

@0xnm 0xnm force-pushed the nogorodnikov/rumm-2195/add-device-and-os-information-to-rum-schema branch from 4b62d50 to e9f491a Compare May 25, 2022 12:25
Comment on lines +248 to +249
"gaming_console",
"bot",
Copy link
Member

Choose a reason for hiding this comment

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

I agree with @0xnm , these are the values available in the backend and since the browser (also using this rum format) can run on those, we need to be compatible

@0xnm 0xnm merged commit d5a943a into master Jun 3, 2022
@0xnm 0xnm deleted the nogorodnikov/rumm-2195/add-device-and-os-information-to-rum-schema branch June 3, 2022 09:43
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.

5 participants