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

Allow passing in a type for errors sent with a message #680

Merged
merged 6 commits into from
Dec 9, 2021

Conversation

AvdLee
Copy link
Contributor

@AvdLee AvdLee commented Dec 3, 2021

What and why?

The current error reporting of Datadog didn't result in the best results for us:
image

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:

// A wrapper around tracked errors to convert into Datadog suitable error messages.
struct DatadogError {
    let underlyingError: Error
    let type: String
    let message: String
    let attributes: [String: Encodable]
    let file: StaticString
    let line: UInt

    init(trackedError: TrackedError) {
        let caseName = trackedError.error.caseName
        let associatedValues = trackedError.error.associatedValues
        let errorDescription = (trackedError.error as? LocalizedError)?.errorDescription ?? caseName

        message = errorDescription
        type = String(describing: Swift.type(of: trackedError.error))
        underlyingError = trackedError.error
        file = trackedError.file
        line = trackedError.line
        attributes = [
            "description": trackedError.description,
            "file": "\(URL(fileURLWithPath: String(describing: trackedError.file)).lastPathComponent)#L\(trackedError.line)",
            "function": String(describing: trackedError.function),
            "associated_values": associatedValues
        ]
    }
}

This results in an improved error message:

image

And the following custom attributes:

image

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

  • Feature or bugfix MUST have appropriate tests (unit, integration) (note: tests didn't run locally, so I'll see what CI reports)
  • Make sure each commit and the PR mention the Issue number or JIRA reference (There was no issue for this)

@AvdLee AvdLee requested a review from a team as a code owner December 3, 2021 10:41
@AvdLee AvdLee requested a review from maxep December 6, 2021 15:53
Copy link
Collaborator

@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.

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:

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.

@AvdLee
Copy link
Contributor Author

AvdLee commented Dec 7, 2021

@ncreated I tried running tests locally, but those fail:

image

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

@AvdLee
Copy link
Contributor Author

AvdLee commented Dec 7, 2021

@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 make file and see what's wrong? It would help for other contributors too.

@AvdLee AvdLee requested a review from ncreated December 7, 2021 13:25
@ncreated
Copy link
Collaborator

ncreated commented Dec 8, 2021

@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 make file and see what's wrong? It would help for other contributors too.

@AvdLee I think your Carthage version is behind what we require - I can see this in your log:

Please update to the latest Carthage version: 0.38.0. You currently are on 0.36.0

dd-sdk-ios requires 0.38.0, which comes with XCFrameworks support. I tested it on my side, it works fine with just make, but we will update CONTRIBUTING.md with this requirement 👌.


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 💪!

Copy link
Member

@maxep maxep left a 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 ncreated merged commit 68711e4 into DataDog:master Dec 9, 2021
@AvdLee
Copy link
Contributor Author

AvdLee commented Dec 13, 2021

@ncreated I was already on 0.38.0 as the logs state as well:

Warning: carthage 0.38.0 already installed

So I think there's something wrong in the script still 🤔

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 💪!

All good, thanks for helping out on those tests! 💪

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.

None yet

4 participants