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

fix(reporting): add error in datadog APM #1615

Merged
merged 4 commits into from
Feb 5, 2024

Conversation

bodinsamuel
Copy link
Collaborator

@bodinsamuel bodinsamuel commented Feb 5, 2024

Describe your changes

Adding error in datadog APM automatically.
Because of a "bug" in dd-trace, the errors are not reported automatically when used in express (cf: DataDog/dd-trace-js#1944)
To resolve that we can tag the error manually.

Issue ticket number and link

Fixes NAN-320

Checklist before requesting a review

  • I added observability

Examples

before after
Screenshot 2024-02-05 at 15 21 07 Screenshot 2024-02-05 at 15 20 54

@bodinsamuel bodinsamuel self-assigned this Feb 5, 2024
@@ -27,7 +27,7 @@
"@trpc/client": "^10.44.1",
"@trpc/server": "^10.44.1",
"@types/fs-extra": "^11.0.1",
"dd-trace": "4.20.0",
"dd-trace": "5.2.0",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Upgrading all the packages because they changed the types (the main breaking change was the drop of Node16)

@@ -46,7 +47,7 @@ class ErrorManager {
}
}

public async report(e: any, config: ErrorOptionalConfig = { source: ErrorSourceEnum.PLATFORM }) {
public report(e: unknown, config: ErrorOptionalConfig = { source: ErrorSourceEnum.PLATFORM }, tracer?: Tracer): void {
Copy link
Collaborator Author

@bodinsamuel bodinsamuel Feb 5, 2024

Choose a reason for hiding this comment

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

I had to pass the tracer around because the report mechanism is used at very different places. Let me know if you think of another way to log to the span, but I think it's handy and could be reused inside the runner or persist.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it is fine. There is definitely a lot of room for improvements wrt how we do error handling in general and across services

@bodinsamuel bodinsamuel marked this pull request as ready for review February 5, 2024 14:52
Copy link

linear bot commented Feb 5, 2024

@@ -46,7 +47,7 @@ class ErrorManager {
}
}

public async report(e: any, config: ErrorOptionalConfig = { source: ErrorSourceEnum.PLATFORM }) {
public report(e: unknown, config: ErrorOptionalConfig = { source: ErrorSourceEnum.PLATFORM }, tracer?: Tracer): void {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it is fine. There is definitely a lot of room for improvements wrt how we do error handling in general and across services

packages/shared/lib/utils/error.manager.ts Outdated Show resolved Hide resolved
@bodinsamuel bodinsamuel merged commit bd9aef0 into master Feb 5, 2024
28 checks passed
@bodinsamuel bodinsamuel deleted the fix/datadog-server-error branch February 5, 2024 15:33
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.

2 participants