-
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
feat: add device.type and network.effectiveType attributes #154
Conversation
@@ -126,6 +126,8 @@ The SDK adds these fields to all telemetry: | |||
| `browser.mobile` | stable | static | [NavigatorUAData: mobile](https://developer.mozilla.org/en-US/docs/Web/API/NavigatorUAData/mobile) | true | | |||
| `browser.language` | stable | static | [Navigator: language](https://developer.mozilla.org/en-US/docs/Web/API/Navigator/language) | "fr-FR" | | |||
| `browser.touch_screen_enabled` | stable | static | [Navigator: maxTouchPoints](https://developer.mozilla.org/en-US/docs/Web/API/Navigator/maxTouchPoints) | true | | |||
| `device.type` | custom | static | Best guess of device type | "desktop", "mobile", "tablet", etc. | | |||
| `network.effectiveType` | custom | static | [NetworkInformation: effectiveType](https://developer.mozilla.org/en-US/docs/Web/API/NetworkInformation/effectiveType). Best guess of user's "effective network type", which is based on their overall network speed. Only available on Chromium devices, and only computed once when the SDK is initialized. | "slow-2g", "2g", "3g", "4g" | |
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.
is there any appetite for including the downlink property or the rtt
as well?
also chromium only, but can provide some extra granularity when diagnosing situations
Screenshot from our homegrown, pre-otel instrumentation. Took inspiration from the web-vitals-reporter
package
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 we should leave it out, at least for now/until we get a request for it.
I think it will also help to minimize the number of chrome-only fields, and this one in particular doesn't add a whole lot on top of connection.type
. I also tend to lean with Mozilla and Apple on the privacy concerns on this one.
It's also very easy for any customer who wants this to add their own attribute on their end, without needing us to track it by default.
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.
looks good - left question about potentially beefing up the attributes, as future food for thought, non-blocking
Which problem is this PR solving?
Adds a more detailed
device.type
attribute. We already havebrowser.isMobile
, which is good enough for a simple true/false check, but this attribute can resolve to any ofWe're already using
ua-parser-js
to determine browser.type, this just uses the same library's API to get device type info as well.Also adds a
network.effectiveType
, which maps to the existingNetworkInformation::effectiveType
in the Network Information API.The Network Information API is only implemented in Chromium so we fall back to the string
"unknown"
in cases where this API isn't available.How to verify that this has the expected result