RUM-9494: Align os and device attributes across all product features#2322
Conversation
maxep
left a comment
maxep
left a comment
There was a problem hiding this comment.
Looks good! I left some minor comments.
| self.systemBootTime = systemBootTime | ||
| } | ||
|
|
||
| public var rumDevice: Device { |
There was a problem hiding this comment.
suggestion/ Does the rum* properties needs to be defined in the Internal module? Can it be extensions on the RUM module instead?
There was a problem hiding this comment.
I think it is a question of naming since the Device and OS are now common models for all the feature products.
I renamed it to normalizedDevice to better state this.
There was a problem hiding this comment.
I see the motivation, the thing is that we are crossing domains here and we are hiding that fact: this model (same as OperatingSystem) is a RUM generated definition but this is no longer explicit. I think we should keep the RUM* prefix in the generator and create an extension for casting, such as:
extension RUMDevice: {
init(_ deviceInfo: DeviceInfo)
}This way, it will be explicit that we are applying the RUM format across domains. And it will be clear that this definition is coming from the generated models. Would that make sense?
It is a nit pick but I feel it will make thing easier to navigate.
There was a problem hiding this comment.
We want to have the feature products isolated from each other. In this case, we want to make sure that the RUM concepts are not leaked to the features using device and operating system models, like Tracing, Logs and Crash Reporting.
And with this approach we can avoid redundant models at the same time we follow a schema that can be updated in the future.
| public struct Device: Codable { | ||
| /// The CPU architecture of the device. Used to symbolication and deobfuscation. | ||
| public let architecture: String | ||
| } |
There was a problem hiding this comment.
So we are duplicating the architecture attribute here for symbolication. Have we consider placing all device info in _dd.device, and _dd.os?
There was a problem hiding this comment.
Yes, it was considered but was not followed because:
There was a problem hiding this comment.
OK 👍 All good if we plan to remove this duplicate.
a36a4a8 to
e699161
Compare
e699161 to
875e7a5
Compare
Datadog ReportBranch report: ✅ 0 Failed, 2113 Passed, 1817 Skipped, 3m 19.02s Total duration (1m 53.96s time saved) |
3 participants
What and why?
This PR aligns the
osanddevicedetails sent to the Datadog intake acrossLogs,RUMandTracefeatures.It aims to have consistent models across features to improve discoverability and future additions of more information.
How?
The main point to address consistency is to have central models that can be reused across the product features.
This means that we have just one model definition of
DeviceandOperatingSystem. They are defined onDatadogInternaland all redundant models are removed.The models respect the schema defined for RUM models including being set in the root of the event as common parameters across events.
For more details on the properties and types, you can refer to the Device schema and OS schema.
Note 1: The Log events have
_ddandhttpparameters with extradeviceandosinformation. The_ddhas the device architecture to help symbolication and deobfuscation and thehttpdevice info is added by the backend after the intake.Note 2: There is a second
DeviceInfomodel (notCodable) but it is used only for internal logic defined in theDatadogContext.Review checklist
make api-surface)