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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jun 3, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions samples/rum/view.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,5 +57,16 @@
},
"context": {
"foo": "bar"
},
"device": {
"type": "tablet",
"name": "iPad",
"model": "iPad",
"brand": "Apple"
},
"os": {
"name": "iOS",
"version": "15.1.1",
"version_major": "15"
}
}
64 changes: 64 additions & 0 deletions schemas/rum/_common-schema.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,70 @@
},
"readOnly": true
},
"os": {
"type": "object",
"description": "Operating system properties",
"required": [
"name",
"version",
"version_major"
],
Comment on lines +210 to +214
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, e.g. Android, iOS",
"readOnly": true
},
"version": {
"type": "string",
"description": "Full operating system version, e.g. 8.1.1",
"readOnly": true
},
"version_major": {
"type": "string",
"description": "Major operating system version, e.g. 8",
"readOnly": true
}
}
},
"device": {
"type": "object",
"description": "Device properties",
"required": [
"type"
],
Comment on lines +236 to +238
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

"properties": {
"type": {
"type": "string",
"description": "Device type info",
"enum": [
"mobile",
"desktop",
"tablet",
"tv",
"gaming_console",
"bot",
Comment on lines +248 to +249
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 •
edited
Loading

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

"other"
],
"readOnly": true
},
"name": {
"type": "string",
"description": "Device marketing name, e.g. Xiaomi Redmi Note 8 Pro, Pixel 5, etc.",
"readOnly": true
},
"model": {
"type": "string",
"description": "Device SKU model, e.g. Samsung SM-988GN, etc. Quite often name and model can be the same.",
"readOnly": true
},
"brand": {
"type": "string",
"description": "Device marketing brand, e.g. Apple, OPPO, Xiaomi, etc.",
"readOnly": true
}
}
},
"_dd": {
"type": "object",
"description": "Internal properties",
Expand Down