RUMM-961 DDError.title is used in RUMEvents.error.type #393

buranmert · 2021-01-29T14:17:04Z

What and why?

As part of error tracking, type: String? is introduced to RUMErrorEvent.Error
This value is expected to be the same with error.kind in Logger
Logger uses DDError.title as error.kind

How?

DDError.title was already used in RUMErrorEvent.Error.message property
Now we basically use the same value in message and type properties of the same instance

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

acgh

For context this is a first step we decided to take before thinking about/implementing a better structure for the input of DDErrors, which ideally would avoid repeating information like in that case title and message.
This PR looks good to me 🚀

ncreated

Few comments left. Also, this PR musts be rebased onto recent master, so it generated @objc interop code too.

ncreated · 2021-02-01T08:35:00Z

Sources/Datadog/RUM/DataModels/RUMDataModels.swift

+        /// User custom timings of the view
+        public struct CustomTimings: Codable {
+        }


We don't yet know how to support DataDog/rum-events-format#25 in rum-models-generator, but struct CustomTimigns doesn't seem to be the right way. We will probably need:

public let customTimings: [String: Int64]

Also, this is very puzzling for the user to see an empty struct in the public API.
→ as we don't support it yet, it should not change the public interface until it gets added in RUMM-1000.

ncreated · 2021-02-01T08:54:31Z

Sources/Datadog/RUM/RUMMonitor/Scopes/RUMResourceScope.swift

@@ -203,10 +203,11 @@ internal class RUMResourceScope: RUMScope {
                    url: resourceURL
                ),
                source: command.errorSource.toRUMDataFormat,
-                stack: command.stack
+                stack: command.stack,
+                type: command.errorMessage


Have you considered introducing command.errorType and setting it to dderror.title at the higher level?

// In RUMCommand // TODO: RUMM-1042 The way of setting error.type must unified with Logging and Tracing self.errorType = dderror.title

This might be safer as a temporary solution, as the work of passing the value down to the scopes will be already done. Also, it limits the refactoring to single file (RUMCommands.swift).

yes i thought about that but it makes more sense to make those changes in 1042

ncreated · 2021-02-01T09:18:18Z

tools/rum-models-generator/Sources/RUMModelsGeneratorCore/Output/Printing/SwiftPrinter.swift

+        // TODO: RUMM-1000 should remove this early return clause
+        if properties.isEmpty {
+            return
+        }


This is not the right place for this patch. rum-models-generator's architecture is build around the stream:

let rumModels = [*.json] → JSONSchema → JSONType → SwiftType -> RUM SwiftType

rumModels are then passed to SwiftPrinter() and ObjcInteropPrinter() for code generation. Instead of patching the printer(s), we should dismiss unsupported types as early as possible in the stream, so that it is not processed further causing unexpected behaviours.

Ideally, we should cut it off in JSONSchemaReader by explicitly deleting schemas which use unsupported additionalProperties modifier.

As this is temporary and short-living patch, fixing it in RUMSwiftTypeTransformer by filtering out the SwiftStruct named "CustomTimings" might be easier.

In any way, this should not reach the code generation, as it generates potentially wrong API (we may need to support additionalProperties in other way than a struct).

RUMModelsGenerator is patched: empty CodingKeys is not generated anymore

DDError.title is passed as error.kind in Logger Now the same value is used as error.type in RUMEvents

acgh · 2021-02-01T14:01:46Z

tools/rum-models-generator/Sources/RUMModelsGeneratorCore/RUM/RUMSwiftTypeTransformer.swift

@@ -94,7 +94,7 @@ internal class RUMSwiftTypeTransformer: TypeTransformer<SwiftType> {

        var `struct` = `struct`
        `struct`.name = format(structName: `struct`.name)
-        `struct`.properties = try `struct`.properties.map { try transform(structProperty: $0) }
+        `struct`.properties = try `struct`.properties.map { try transform(structProperty: $0) }.filter { property in property.name != "customTimings" }


Maybe add back the TODO: RUMM-1000 here?

oh yes definitely, thanks 🙏

DDError.title is passed as error.kind in Logger Now the same value is used as error.type in RUMEvents

ncreated

LGT 👌

buranmert self-assigned this Jan 29, 2021

buranmert requested review from ncreated, xgouchet, mariusc83 and acgh January 29, 2021 14:17

buranmert force-pushed the buranmert/RUMM-961-error-type branch from ac3b0c1 to 4e8cbba Compare January 29, 2021 17:24

buranmert added this to the next-version milestone Jan 29, 2021

buranmert marked this pull request as ready for review January 29, 2021 17:27

buranmert requested a review from a team as a code owner January 29, 2021 17:27

buranmert force-pushed the buranmert/RUMM-961-error-type branch from 4e8cbba to 0e41097 Compare January 29, 2021 17:38

acgh approved these changes Feb 1, 2021

View reviewed changes

ncreated requested changes Feb 1, 2021

View reviewed changes

buranmert added 3 commits February 1, 2021 12:54

RUMM-961 RUMDataModels updated

7166393

RUMModelsGenerator is patched: empty CodingKeys is not generated anymore

RUMM-961 Datadog source adapted to new RUMDataModels

3c39f05

RUMM-961 DDError.title is passed as error.type

ed5f19c

DDError.title is passed as error.kind in Logger Now the same value is used as error.type in RUMEvents

buranmert force-pushed the buranmert/RUMM-961-error-type branch 2 times, most recently from 629a21a to bb6e53f Compare February 1, 2021 13:53

acgh reviewed Feb 1, 2021

View reviewed changes

RUMM-961 DDError.title is passed as error.type

463e3b2

DDError.title is passed as error.kind in Logger Now the same value is used as error.type in RUMEvents

buranmert force-pushed the buranmert/RUMM-961-error-type branch from bb6e53f to 463e3b2 Compare February 1, 2021 14:16

buranmert requested a review from ncreated February 1, 2021 14:43

ncreated approved these changes Feb 2, 2021

View reviewed changes

buranmert merged commit 8b35eb7 into master Feb 2, 2021

buranmert deleted the buranmert/RUMM-961-error-type branch February 2, 2021 09:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RUMM-961 DDError.title is used in RUMEvents.error.type #393

RUMM-961 DDError.title is used in RUMEvents.error.type #393

buranmert commented Jan 29, 2021 •

edited

Loading

acgh left a comment

ncreated left a comment

ncreated Feb 1, 2021

ncreated Feb 1, 2021

buranmert Feb 1, 2021 •

edited

Loading

ncreated Feb 1, 2021

acgh Feb 1, 2021

buranmert Feb 1, 2021

ncreated left a comment

RUMM-961 DDError.title is used in RUMEvents.error.type #393

RUMM-961 DDError.title is used in RUMEvents.error.type #393

Conversation

buranmert commented Jan 29, 2021 • edited Loading

What and why?

How?

Review checklist

acgh left a comment

Choose a reason for hiding this comment

ncreated left a comment

Choose a reason for hiding this comment

ncreated Feb 1, 2021

Choose a reason for hiding this comment

ncreated Feb 1, 2021

Choose a reason for hiding this comment

buranmert Feb 1, 2021 • edited Loading

Choose a reason for hiding this comment

ncreated Feb 1, 2021

Choose a reason for hiding this comment

acgh Feb 1, 2021

Choose a reason for hiding this comment

buranmert Feb 1, 2021

Choose a reason for hiding this comment

ncreated left a comment

Choose a reason for hiding this comment

buranmert commented Jan 29, 2021 •

edited

Loading

buranmert Feb 1, 2021 •

edited

Loading