RUMM-2197 Improve `device` and `os` info in RUM #888

ncreated · 2022-06-09T17:27:19Z

What and why?

📦 This PR updates the SDK to use new device and os attributes added recently to RUM schema.

With this change, device and os will no longer be inferred by backend from SDK UA header. Instead, BE can now read these values directly from RUM events.

As a user-facing change, we now enhance the device.model information. Previously it was either iPhone | iPad | iPod with no further context. Now it names the model along with its version, e.g. iPhone13,2 or iPad13,4.

How?

New spec for device and os:

device.name can be iPhone | iPad | Apple TV | ... (same as before),
device.model now includes version, e.g. AppleTV11,1 (can be a breaking change in some filters and dashboards ⚠️),
device.brand is always Apple,
os.name, os.version and os.version_major remain unchanged - their values will be just like before.

I used a simplified version of CwlUtils/CwlSysctl.swift to read device information from system attributes (HW_MACHINE). Nice discussion on this approach and its macOS vs iOS nuances can be found here.

Everything happens in MobileDevice class, which isn't perfect design, but seems pragmatic choice considering that we will make it more straightforward in with V2's SDK context.

I added 2 layers of tests:

unit tests assert if the values are set,
integration tests apply additional validation for all RUM events (through RUMSessionMatcher) to ensure that values are correct for current platform (e.g. iOS Simulator).

Tests

I tested all values on 4 simulator and 2 real devices:

iPhone Simulator:

brand: Apple
name: iPhone
model: iPhone13,2 Simulator
OS name: iOS
OS version: 15.2

iPod touch Simulator:

brand: Apple
name: iPhone (which looks like a bug 🐞 in UIDevice.model for iPod Simulator)
model: iPod9,1 Simulator
OS name: iOS
OS version: 15.2

iPad Simulator:

brand: Apple
name: iPad
model: iPad13,5 Simulator
OS name: iPadOS
OS version: 15.2

Apple TV Simulator:

brand: Apple
name: Apple TV
model: AppleTV11,1
OS name: tvOS
OS version: 15.2

Real iPhone:

brand: Apple
name: iPhone
model: iPhone10,6
OS name: iOS
OS version: 15.2

Real iPad:

brand: Apple
name: iPad
model: iPad13,4
OS name: iPadOS
OS version: 15.3.1

Review checklist

Feature or bugfix MUST have appropriate tests (unit, integration)
Make sure each commit and the PR mention the Issue number or JIRA reference
Add CHANGELOG entry for user facing changes

Custom CI job configuration (optional)

Run unit tests
Run integration tests
Run smoke tests

…events

ncreated · 2022-06-13T11:34:03Z

Sources/Datadog/Core/System/MobileDevice.swift

+    convenience init() {
+        let processInfo = ProcessInfo.processInfo
+        let device = UIDevice.current
+        let model: String
+
+        #if !targetEnvironment(simulator)
+        // Real tvOS device
+        model = (try? Sysctl.getModel()) ?? device.model
+        #else
+        // tvOS Simulator
+        let simulatorModel = processInfo.environment["SIMULATOR_MODEL_IDENTIFIER"] ?? device.model
+        model = "\(simulatorModel) Simulator"
+        #endif
+
+        self.init(
+            model: model,
+            uiDevice: device,
+            processInfo: processInfo,
+            notificationCenter: .default
+        )
+    }


These new initialisers in MobileDevice are added for init consistency on both iOS and tvOS (both define matching init()). This and the entire MobileDevice class should be however a subject to change in V2. Since we added tvOS support, it should no longer be "mobile device" and we should really split its responsibilities. For example, the cohesion of this class is very low - battery reasoning and management is only implemented for iOS and we use mock values for tvOS just to "make it work".

Sounds good!

ncreated · 2022-06-13T11:35:33Z

Sources/Datadog/Core/Upload/RequestBuilder.swift

-                value: .constant("\(sanitizedAppName)/\(appVersion) CFNetwork (\(device.model); \(device.osName)/\(device.osVersion))")
+                value: .constant("\(sanitizedAppName)/\(appVersion) CFNetwork (\(device.name); \(device.osName)/\(device.osVersion))")


These kind of changes in this PR are OK. What used to work as device.model before, now is called device.name (iPhone | iPad | ...). The device.model is now reserved for exact info, e.g. iPhone10,2.

ncreated · 2022-06-13T11:37:29Z

Sources/Datadog/FeaturesIntegration/CrashReporting/CrashReportingWithRUMIntegration.swift

+            // RUMM-2197: In very rare cases, the OS info computed below might not be exactly the one
+            // that the app crashed on. This would correspond to a scenario when the device OS was upgraded
+            // before restarting the app after crash. To solve this, the OS information would have to be
+            // persisted in `crashContext` the same way as we do for other dynamic information.
+            os: .init(from: deviceInfoProvider),


I haven't decided for this implementation as it will only address a very rare case with quite a work required. It is however a good driver for more robust SDK context management in V2.

…ents (for integration tests)

maxep

Looks great! 👍

Once this and v2 feature scope are merged, we should apply this to the feature/v2 and its context, so we keep v2 in sync.

maxep · 2022-06-14T15:21:21Z

Sources/Datadog/Core/System/MobileDevice.swift

+    convenience init() {
+        let processInfo = ProcessInfo.processInfo
+        let device = UIDevice.current
+        let model: String
+
+        #if !targetEnvironment(simulator)
+        // Real tvOS device
+        model = (try? Sysctl.getModel()) ?? device.model
+        #else
+        // tvOS Simulator
+        let simulatorModel = processInfo.environment["SIMULATOR_MODEL_IDENTIFIER"] ?? device.model
+        model = "\(simulatorModel) Simulator"
+        #endif
+
+        self.init(
+            model: model,
+            uiDevice: device,
+            processInfo: processInfo,
+            notificationCenter: .default
+        )
+    }


Sounds good!

ncreated self-assigned this Jun 9, 2022

ncreated force-pushed the ncreated/RUMM-2197-add-os-and-device-info-to-rum-part2 branch 2 times, most recently from 3463f0d to 478c401 Compare June 10, 2022 08:40

RUMM-2197 Read device model from sysctl

d705e7b

ncreated force-pushed the ncreated/RUMM-2197-add-os-and-device-info-to-rum-part2 branch from 478c401 to 9effbb3 Compare June 10, 2022 11:08

RUMM-2197 Infer device and OS info from MobileDevice

5949bd3

ncreated force-pushed the ncreated/RUMM-2197-add-os-and-device-info-to-rum-part2 branch from 9effbb3 to 559daee Compare June 13, 2022 10:39

ncreated changed the title ~~RUMM-2197 Improve device.model in RUM~~ RUMM-2197 Improve device and os info in RUM Jun 13, 2022

ncreated added 4 commits June 13, 2022 13:30

RUMM-2197 Validate os and device info in tests

70acb67

RUMM-2197 Add unit tests for including device and os info in RUM …

ffdcb06

…events

RUMM-2197 Add CwlUtils to LICENSE-3rdparty.csv

d22aa12

RUMM-2197 Update CHANGELOG.md

8409225

ncreated force-pushed the ncreated/RUMM-2197-add-os-and-device-info-to-rum-part2 branch from defa092 to 8409225 Compare June 13, 2022 11:30

ncreated commented Jun 13, 2022

View reviewed changes

RUMM-2197 Fix integration tests build

793bdcc

ncreated marked this pull request as ready for review June 13, 2022 12:03

ncreated requested a review from a team as a code owner June 13, 2022 12:03

RUMM-2197 Avoid strict device and os validation in browser SDK ev…

1023c41

…ents (for integration tests)

ncreated force-pushed the ncreated/RUMM-2197-add-os-and-device-info-to-rum-part2 branch from 6cdf6c6 to 1023c41 Compare June 13, 2022 15:50

xgouchet approved these changes Jun 13, 2022

View reviewed changes

maxep approved these changes Jun 14, 2022

View reviewed changes

Base automatically changed from ncreated/RUMM-2197-add-os-and-device-info-to-rum-part1 to develop June 14, 2022 18:00

ncreated merged commit e98ab54 into develop Jun 14, 2022

ncreated deleted the ncreated/RUMM-2197-add-os-and-device-info-to-rum-part2 branch June 14, 2022 21:04

ncreated mentioned this pull request Jun 20, 2022

Merge develop into feature/v2 #901

Merged

6 tasks

ncreated mentioned this pull request Jul 21, 2022

Dogfood recent changes #937

Merged

6 tasks

This was referenced Sep 16, 2022

Release 1.12.0 #1000

Merged

Merge release/1.12.0 to develop #1001

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RUMM-2197 Improve `device` and `os` info in RUM #888

RUMM-2197 Improve `device` and `os` info in RUM #888

ncreated commented Jun 9, 2022 •

edited

Loading

ncreated Jun 13, 2022

maxep Jun 14, 2022

ncreated Jun 13, 2022

ncreated Jun 13, 2022

maxep left a comment

maxep Jun 14, 2022

		value: .constant("\(sanitizedAppName)/\(appVersion) CFNetwork (\(device.model); \(device.osName)/\(device.osVersion))")
		value: .constant("\(sanitizedAppName)/\(appVersion) CFNetwork (\(device.name); \(device.osName)/\(device.osVersion))")

RUMM-2197 Improve device and os info in RUM #888

RUMM-2197 Improve device and os info in RUM #888

Conversation

ncreated commented Jun 9, 2022 • edited Loading

What and why?

How?

Tests

iPhone Simulator:

iPod touch Simulator:

iPad Simulator:

Apple TV Simulator:

Real iPhone:

Real iPad:

Review checklist

Custom CI job configuration (optional)

ncreated Jun 13, 2022

Choose a reason for hiding this comment

maxep Jun 14, 2022

Choose a reason for hiding this comment

ncreated Jun 13, 2022

Choose a reason for hiding this comment

ncreated Jun 13, 2022

Choose a reason for hiding this comment

maxep left a comment

Choose a reason for hiding this comment

maxep Jun 14, 2022

Choose a reason for hiding this comment

RUMM-2197 Improve `device` and `os` info in RUM #888

RUMM-2197 Improve `device` and `os` info in RUM #888

ncreated commented Jun 9, 2022 •

edited

Loading