-
Notifications
You must be signed in to change notification settings - Fork 298
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
Express tracing does not add error tags to the service entry span #1944
Comments
The main issue in a case like this is that the error is handled by a middleware. An exception would only reach the request if it's not handled at all. For example, in your above code the error is handled by the middleware and then |
This would be a really great solution. |
Even other tools (e.g. Sentry) offer specific express handlers for catching errors in the stack. The new API could look something like that. |
I am having the issue too. This is quite a blocker, the Error Tracking feature of DataDog is broken and unusable as-is because of this. |
The workaround above could be improved by using a span hook in combination to remove the need for any private API. For example: // at the top of the app entry point right after trace.init() is called
tracer.use('express', {
hooks: {
request (span, res, req) {
if (res.error) {
span.setTag('error', res.error);
}
}
}
});
// middleware added after all other middleware
app.use((err, req, res, next) => {
res.error = err;
res.status(500).send();
}); While we could definitely support this better, I think this solution is good enough since it only requires adding a small middleware and a span hook to the application. We're also looking at changing our defaults to potentially automatically bubble up middleware errors depending on the status code. |
Having the same issue. However, I'm using typescript so I can't access
And we are using hapi, which doesn't seem to work with hooks
Any help will be appreciated |
I couldn't make the hook to work, not sure if it is because we are creating our However, I was able to access to the private api by doing the following on typescript:
|
@rochdev I have to ask... how is this actually intended to work with the express plugin (or in my case, the Koa plugin)? Is it because Koa (and presumably Express) have built-in default error handlers? I just don't see a way to follow the docs and get this working out of the box without additional hacking around. |
@jrnail23 We're actually revisiting this right now. There are basically tradeoffs either way, because if an error that we intercepted is later handled, it could result in false positives, but with the current approach we end up missing errors instead. We will be working on a fix as soon as we have settled on an approach that will work for all Web frameworks. |
@rochdev could you give some estimates? It'd be great if I could set up a reminder for myself. |
@vitkarpov For now we've decided to simply keep the last seen error if the status code is 5xx to avoid any breaking change. This also makes the change more trivial. We'll try to include it in 3.0 if possible, but otherwise we should be able to land it in 3.1. So this means this month, and possibly next week if 3.0. |
Still not working, The error do not appear in Error Tracking |
I'm still experiencing this issue as well, though I'm using NextJS. Still, though, |
@akirillo Is there an actual error being thrown to the middleware with either |
i have test endpoint that looks like:
|
@akirillo What version of dd-trace are you using? Also, can you share a reproduction snippet? |
If you must use a type assertion, probably sensible to use something narrower than import tracer, { Span, SpanContext } from 'dd-trace';
tracer.init();
interface ContextWithStartedExposed extends SpanContext {
_trace: {
started: Span[];
};
}
const assertsHasUnderscoreTrace: (
context: SpanContext
) => asserts context is ContextWithStartedExposed = (context) => {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
const contextAny = context as any;
const errorMessage =
'Tried to set span on service entry tag using undocumented API that appears to no longer exist';
if (!contextAny._trace) {
throw new Error(errorMessage);
}
if (!contextAny._trace.started) {
throw new Error(errorMessage);
}
};
// ... more code
export const doSomethingWithError = (error: Error) => {
const context = tracer.scope().active()?.context();
if (context) {
assertsHasUnderscoreTrace(context);
const serviceEntrySpan = context._trace.started[0];
serviceEntry.setTag(...);
}
}; |
We currently use NestJS and also have the issue where all errors just show error: true with no context and do not appear in error tracking since the message and stack trace are missing. NestJS handles errors before the express layer so the error context is always undefined. Here is a work around we found that should work with any framework as long as you can get access to the express request. It re-uses the dd-trace/src/plugins/util/web.js addError to forcibly set the error context within our framework's (NestJS) error handling.
import { Catch, ArgumentsHost } from "@nestjs/common";
import { BaseExceptionFilter } from "@nestjs/core";
import { addError } from "dd-trace/packages/dd-trace/src/plugins/util/web";
@Catch()
export class AllExceptionsFilter extends BaseExceptionFilter {
catch(exception: unknown, host: ArgumentsHost) {
// Without this guard any rpc, ws, or graphql calls can fail
if (host.getType() === "http") {
const ctx = host.switchToHttp();
addError(ctx.getRequest(), exception);
super.catch(exception, host);
} else if (host.getType<GqlContextType>() === "graphql") {
// This is a little dirty but req is not easily exposed anywhere currently
// https://stackoverflow.com/questions/73123824/change-graphql-context-to-http-context
// https://github.com/nestjs/docs.nestjs.com/issues/2143
const gqlHost = GqlArgumentsHost.create(host);
const ctx = gqlHost.getContext();
addError(ctx.req, exception);
// DO NOT HANDLE ERROR - gql handles errors in its own way
}
}
} EDIT: I cannot get the graphql part to work it never has the express reqest context it just has the node When configuring the nest app const { httpAdapter } = app.get(HttpAdapterHost);
app.useGlobalFilters(new AllExceptionsFilter(httpAdapter)); Had to add types to be able to import in TS
declare module "dd-trace/packages/dd-trace/src/plugins/util/web" {
export const addError: (req: unknown, error: unknown) => void;
} |
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.
Describe the bug
I am trying out the Error Tracking feature and am running into some unexpected results.
It seems that the
dd-trace
library is not correctly marking the service entry span in an express request with an error which happens deeper in the route stack.Reproducible with the following code:
When inspecting the trace we see the error attached correctly to the deeper route span:
But when we look at the top level span:
And since the documentation clearly states that:
And because this service entry span is missing those error tags, we don't see the error in the Error Tracking UI:
HACKY Workaround
While searching for this issue, I stumbled upon #725.
It seems we could access the service entry span from an express error handler:
Needless to say, this is sub-optimal, since it's using private APIs, which could be removed in future versions.
This feature should be supported by the
dd-trace
library as a native feature.Environment
The text was updated successfully, but these errors were encountered: