-
Notifications
You must be signed in to change notification settings - Fork 4
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
RUMM-2195: Add OS and device information properties to the RUM schema #62
Conversation
"gaming_console", | ||
"bot", |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
"required": [ | ||
"type" | ||
], |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
"required": [ | ||
"name", | ||
"version", | ||
"version_major" | ||
], |
There was a problem hiding this comment.
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
schemas/rum/_common-schema.json
Outdated
"properties": { | ||
"name": { | ||
"type": "string", | ||
"description": "Operating system name, ex. Android, iOS", |
There was a problem hiding this comment.
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 🙃
"description": "Operating system name, ex. Android, iOS", | |
"description": "Operating system name, e.g. Android, iOS", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, you are right
"gaming_console", | ||
"bot", |
There was a problem hiding this comment.
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
.
4b62d50
to
e9f491a
Compare
"gaming_console", | ||
"bot", |
There was a problem hiding this comment.
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
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 ofdevice
is required, because SDK running on thedesktop
, for example, probably may not have any info aboutname
,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 thatAppliance
type is replaced by theTV
, because we have now a stronger focus on the TV platforms and any other device fromappliance
type can be marked asother
.