RUMM-1113 Pass logged error to RUM integration #423
Conversation
Now Log has its own Error property Integration use Error property and pass it to RUM Also, Scopes take error.type instead of using error.message for type
| internal struct DDError: Equatable { | ||
| let title: String | ||
| let message: String | ||
| let details: String |
There was a problem hiding this comment.
Today we know much more on the unified "Datadog error format". Error Tracking team uses error.type, error.message and error.stackTrace consistently and those values are commonly present in Logging, APM and RUM. This is a new knowledge comparing to what we knew when introducing this DDError.
How about improving DDError to act as a unified Datadog Error format? Can be done with simple renaming:
/// Datadog Error format
internal struct DDError: Equatable {
let type: String
let message: String
let stack: String
}This should make the work with DDError much easier, as all the downstream mappings will be more straightforward, e.g.:
- in
Logencoding: error stack =dderror.detailswill become error stack =dderror.stack - in
LoggingtoRUMintegration, error type =dderror.titlewill become error type =dderror.type
Also, this would help a lot with upcoming RUMM-1050 (Send crash reports as Logs), where type, message and stack are read separately from DDCrashReport. With this change, we could simply build DDError(type: ddcrash.type, message: ddcrash.message, stack: ddcrash.stack) and pass it to the LogBuilder using new logic added in this PR 👌.
There was a problem hiding this comment.
good idea, done ✅
| @@ -15,5 +15,5 @@ internal struct LogAttributes { | |||
|
|
|||
| /// Type writting logs to some destination. | |||
| internal protocol LogOutput { | |||
| func writeLogWith(level: LogLevel, message: String, date: Date, attributes: LogAttributes, tags: Set<String>) | |||
| func writeLogWith(level: LogLevel, message: String, error: Error?, date: Date, attributes: LogAttributes, tags: Set<String>) | |||
There was a problem hiding this comment.
If we change error: Error? to dderror: DDError? here, we won't need to play with the vague Error representation downstream (it will be mapped to DDError early in the Logger). This will also enable seamless integration of crash reporting (RUMM-1050), as the DDError() for crash report can be easily constructed.
| recordedLog = RecordedLog( | ||
| level: level, | ||
| message: message, | ||
| error: error.flatMap { DDError(error: $0) }, |
There was a problem hiding this comment.
DDError in LogOutput signature. Now we need to make this mock smarter (by using part of the real output logic), to make it work. Mapping to DDError as soon as possible (in Logger) would fix this.
Sources/Datadog/RUMMonitor.swift
Outdated
| @@ -307,11 +307,13 @@ public class RUMMonitor: DDRUMMonitor, RUMCommandSubscriber { | |||
| } | |||
| return nil | |||
| }() | |||
| addError(message: message, stack: stack, source: RUMInternalErrorSource(source), attributes: attributes) | |||
| // Note for CR: should `type` be nil? | |||
There was a problem hiding this comment.
I think this is what we concluded with the Android SDK team, right?
| // Note for CR: should `type` be nil? |
Sources/Datadog/RUMMonitor.swift
Outdated
| @@ -491,6 +494,8 @@ public class RUMMonitor: DDRUMMonitor, RUMCommandSubscriber { | |||
| resourceKey: resourceKey, | |||
| time: dateProvider.currentDate(), | |||
| message: errorMessage, | |||
| // Note for CR: should we leave it nil? | |||
There was a problem hiding this comment.
Same - concluded with the Android SDK team.
| // Note for CR: should we leave it nil? |
buranmert
force-pushed
the
buranmert/RUMM-1113-pass-log-error-to-rum-integration
branch
from
01a6f60
to
c32ee1d
Compare
buranmert
deleted the
buranmert/RUMM-1113-pass-log-error-to-rum-integration
branch
What and why?
When
logger.error(...)is called,LoggingWithRUMIntegrationcreates an equivalent RUM error.However, this integration is not fully up to date: when it was written
logger.error(...)could only takemessage: Stringas parameter but now it also takeserror: Error?as parameter.How?
This PR passes logged
errorto RUM integration. That way, RUM error is created out of the loggederrorinstead of loggedmessage. This gives more accurate information in RUM Explorer as can be seen in screenshots below.log.error("an error message occurred")log.error("some message", error: NSError(...))Review checklist