Skip to content
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

Merged
merged 4 commits into from
Nov 8, 2021

Conversation

buranmert
Copy link
Contributor

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

  • Feature or bugfix MUST have appropriate tests (unit, integration)
  • Make sure each commit and the PR mention the Issue number or JIRA reference

@buranmert buranmert self-assigned this Oct 28, 2021
@buranmert buranmert requested a review from a team as a code owner October 28, 2021 10:03
@buranmert buranmert force-pushed the buranmert/RUMM-1718-error-sourceType branch from 4512848 to 52a9e82 Compare October 28, 2021 10:27
Copy link
Member

@ncreated ncreated left a 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.

Unverified

This user has not yet uploaded their public signing key.
error.sourceType can be customized via attributes
@buranmert buranmert force-pushed the buranmert/RUMM-1718-error-sourceType branch from 52a9e82 to a5905e4 Compare November 3, 2021 11:54
@buranmert buranmert requested a review from ncreated November 3, 2021 11:55
Comment on lines 211 to 214
let sourceType = (attributes.removeValue(forKey: RUMAttribute.internalErrorSourceType) as? String)
.flatMap {
return RUMErrorEvent.Error.SourceType(rawValue: $0)
} ?? .ios
Copy link
Member

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 ?

Copy link
Contributor Author

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
@buranmert buranmert force-pushed the buranmert/RUMM-1718-error-sourceType branch from 8887100 to c683734 Compare November 5, 2021 11:02
@@ -233,6 +233,7 @@ internal class RUMResourceScope: RUMScope {
url: resourceURL
),
source: command.errorSource.toRUMDataFormat,
sourceType: command.errorSourceType,
Copy link
Member

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,
Copy link
Member

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)
}
Copy link
Member

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
Copy link
Member

@ncreated ncreated left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👌 ✨

@buranmert buranmert merged commit eef82d8 into master Nov 8, 2021
@buranmert buranmert deleted the buranmert/RUMM-1718-error-sourceType branch November 8, 2021 14:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants