-
Notifications
You must be signed in to change notification settings - Fork 133
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
Allow passing in a type for errors sent with a message #680
Conversation
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.
Thanks for this contribution @AvdLee 👌! We have discussed this internally and we'd be happy to extend our public API in the proposed way 👍 - indeed we should allow for customizing error.type
as it's not always inferred automatically.
Diff looks fine, although I think it would be more consistent to also update resource error API in the same way, as it clearly misses the type
as well:
public func stopResourceLoadingWithError(
resourceKey: String,
errorMessage: String,
response: URLResponse? = nil,
attributes: [AttributeKey: AttributeValue] = [:]
) {}
Second, we need to test this feature ⚔️. In case of RUMMonitor
and its public APIs we use integration tests defined in RUMMonitorTests
. I think we could simply update this test case to pass random type
in and expect it in assertion:
dd-sdk-ios/Tests/DatadogTests/Datadog/RUMMonitorTests.swift
Lines 379 to 381 in 8e7c19e
monitor.addError(message: "View error message", source: .source) | |
#sourceLocation() | |
monitor.addError(message: "Another error message", source: .webview, stack: "Error stack") |
We have similar behaviour test defined for resource error as well.
@ncreated I tried running tests locally, but those fail: I've also run make, but there seems to be something wrong in the script: avanderlee@Antoines-MBP dd-sdk-ios % make
⚙️ Installing dependencies...
Warning: carthage 0.38.0 already installed
Please update to the latest Carthage version: 0.38.0. You currently are on 0.36.0
Unrecognized arguments: --use-xcframeworks
make: *** [dependencies] Error 1 I'll try to update your requested code changes and write the tests w/o running them locally |
@ncreated added the tests. If they fail, I'm happy to fix them, but it would be great if I can run the tests locally. Could you look into the |
@AvdLee I think your Carthage version is behind what we require - I can see this in your log:
And thanks for adding tests! There was few things missing, not easy to spot by external contributor 🙂. When sending new resource / error events, the SDK might send some more intermediate events, which wasn't asserted correctly. I updated tests in this PR by using our new and simplified assertion mechanism that reduces intermediate events. It should be all good now 👍. I will merge it when it completes on CI (we're struggling with some flakiness today ☔). Once again, thanks a lot for this contribution 🚀🏅, we really appreciate your help 💪! |
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.
LGTM 🚀 Thanks again @AvdLee !
@ncreated I was already on
So I think there's something wrong in the script still 🤔
All good, thanks for helping out on those tests! 💪 |
What and why?
The current error reporting of Datadog didn't result in the best results for us:
As you can see, these 3 errors are the same, but they aren't seen as the same since they differ due to the associated value.
We've implemented our own custom error parsing locally which uses mirroring to get the enum case and associated values separately:
This results in an improved error message:
And the following custom attributes:
How?
Though, for this to work we had to be able to set the
type
when adding an error using a custom message. This PR opens up that initialiser due to adjusting the public method.Review checklist