RUMM-776 Add custom timings API for RUM Views #323
Conversation
this is a workaround for backend issue with overwriting flattened JSON keys when `view.custom_timings` precedes the `view` object in the RUM event payload.
| @@ -15,7 +15,9 @@ extension JSONEncoder { | |||
| try container.encode(formatted) | |||
| } | |||
| if #available(iOS 13.0, OSX 10.15, *) { | |||
| encoder.outputFormatting = [.withoutEscapingSlashes] | |||
| encoder.outputFormatting = [.withoutEscapingSlashes, .sortedKeys] | |||
There was a problem hiding this comment.
we can add a comment explaining .sortedKeys
Sources/Datadog/DDRUMMonitor.swift
Outdated
| /// Adds a specific timing in the currently presented View. The timing duration will be computed as the | ||
| /// number of nanoseconds between the time the View was started and the time the timing was added. | ||
| /// - Parameters: | ||
| /// - name: the name of the custom timing attribute. |
There was a problem hiding this comment.
we also use name as key and they must be unique, that might be worth mentioning here
There was a problem hiding this comment.
Added the note that name must be unique for each timing 馃憤.
| /// Custom View timings (only available if `DM` is a RUM View model) | ||
| let customViewTimings: [String: Int64]? |
There was a problem hiding this comment.
this comment doesn't sound like it's intended.?
IMO we can add customViewTimings via an RUMView extension
extension RUMView {
func addCustomTimingAttribute(_ timing) { self.attributes["view.custom_timings.\(timing.name)"] = timing.value }
// ...and if needed
var customViewTimings { return attributes.filter { $0.startsWith("view.custom_timings") } }
}There was a problem hiding this comment.
RUMView is created by the user to configure the auto-instrumentation and UIKitRUMViewsPredicate, I don't find it like a good place to store custom timings. Do you mean RUMDataView: RUMDataModel? That one in turn is auto-generated, so I didn't want to extend it with any custom functionality (even through extension in separate file).
As all other encoding logic is done in RUMEventEncoder, I found it quite natural to put timings also in there. Having additional property for timings (customViewTimings) will come handy for data scrubbing, as it adds clear context to those values and we can make them (un-)editable as we want. If merged with attributes, we'd have to additionally filter them out as you suggest, so I prefer the easiest way for now.
There was a problem hiding this comment.
sorry yes, i meant RUMDataView
ideally this was supposed to be part of RUMDataView but it's not so due to limitations of JSON schema
i'd stay as close as possible to the ideal solution
but i'm resolving because we just follow android here 馃憣
| var four = 1 | ||
|
|
||
| enum CodingKeys: String, CodingKey { | ||
| case one = "aaaaaa" |
There was a problem hiding this comment.
i think our use case is keys with ., no? if we add keys with . here, maybe we can catch potential issues in next iOS versions
There was a problem hiding this comment.
True 馃憣. Added one more variant: aaa.aaa to cover this.
ncreated
deleted the
ncreated/RUMM-776-add-custom-timings-support-in-RUM
branch
What and why?
馃摝 This PR introduces custom timings API for RUM:
How?
Timing is measured as the number of nanoseconds since the start of the current view.
Timings are encoded as
view.custom_timings.<name-of-the-timing>values in RUM event JSON. To make it work, I had to apply workaround for backend issue: we must enforce thatview.custom_timings.*appears after theview.object (achieved by enabling.sortedKeysoption in JSON encoder).To test timings with mocked
RelativeDateProvider, I had to ensure that it is not used by the storage part of the mocked feature. This is done by injectingSystemDateProviderwhen partially mocking each feature.Review checklist