-
Notifications
You must be signed in to change notification settings - Fork 134
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RUMM-1718 error.sourceType
added
#650
Conversation
4512848
to
52a9e82
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 👍, however part of RUMM-1718
DOD was to also:
Read and use the custom
_dd.error.source_type
attribute value if any;
so we can configure it from bridge-ios
and then set from RN SDK respectively.
error.sourceType can be customized via attributes
52a9e82
to
a5905e4
Compare
let sourceType = (attributes.removeValue(forKey: RUMAttribute.internalErrorSourceType) as? String) | ||
.flatMap { | ||
return RUMErrorEvent.Error.SourceType(rawValue: $0) | ||
} ?? .ios |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're missing unit tests for this logic. However, that would require similar complex test to the one added in RUMViewScopeTests
. Wouldn't it be be better to resolve this attribute in RUMStopResourceWithErrorCommand
and RUMAddCurrentViewErrorCommand
?
Note that other properties describing resource error and view error are transported as RUMCommand
fields, produced upstream and only read downstream. If adding sourceType
to RUMStopResourceWithErrorCommand
and RUMAddCurrentViewErrorCommand
we can easily test all cases in RUMCommandTests
. If further making RUM scope tests fuzzy on sourceType: .mockRandom()
that would be minimal effort for max safety. WDYT @buranmert @maxep ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done ✅
errorSourceType is extracted in RUMCommand level instead of Scopes level
8887100
to
c683734
Compare
@@ -233,6 +233,7 @@ internal class RUMResourceScope: RUMScope { | |||
url: resourceURL | |||
), | |||
source: command.errorSource.toRUMDataFormat, | |||
sourceType: command.errorSourceType, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not covered in RUMResourceScopeTests.swift
- if we put an arbitrary value here, no test will trigger.
@@ -229,6 +229,7 @@ extension RUMErrorEvent: RandomMockable { | |||
url: .mockRandom() | |||
), | |||
source: [.source, .network, .custom].randomElement()!, | |||
sourceType: .ios, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is randomized mock (mockRandom()
) for fuzzy testing, so it should use random value (like source:
).
|
||
XCTAssertEqual(command2.errorSourceType, .reactNative) | ||
XCTAssertTrue(command2.attributes.isEmpty) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're missing the case of using .ios
default value when RUMAttribute.internalErrorSourceType
is not specified.
Missing test cases added
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👌 ✨
What and why?
Errors may come from different sources, this new property helps specifying the origin platform of the error.
How?
error.sourceType
is added to JSON schemas and iOS SDK sets it to.ios
by default.Review checklist