Conversation
|
Probably should be labeled with |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 40720eabb7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…to a non-5xx response JAX-RS and Spring MVC resource/controller methods commonly signal a non-2xx response by throwing an exception (e.g. NotFoundException, ResponseStatusException, or a custom exception annotated with @ResponseStatus) that the framework's own exception-mapping machinery turns into a normal HTTP response. The tracer previously flagged the resource/controller span as an error unconditionally whenever such an exception was thrown, even when the framework maps it to a routine 4xx (or other non-5xx) status - flooding error tracking with non-actionable "errors" for expected control flow. These exceptions already carry their intended status (WebApplicationException's embedded Response, ResponseStatusException/ErrorResponse, or the @ResponseStatus annotation), so the fix decides the error flag from that status against the same "server error" set used for the root HTTP span, instead of unconditionally erroring. @ResponseStatus's value() and code() attributes are @aliasfor each other, but plain reflection on the annotation proxy doesn't resolve that aliasing - if a caller sets only code(), value() still reports its own default (INTERNAL_SERVER_ERROR) rather than the value mirrored from code(). Both Spring decorators now read code() first (reflectively on the Spring 3.1 classpath, which predates code()'s introduction in 4.2) and fall back to value() only when code() is left at its default.
…TTP status Some applications signal a response status via their own exception hierarchy and a generic exception-handling advice, rather than any of the JAX-RS/Spring conventions already handled (WebApplicationException, ResponseStatusException, ErrorResponse, @ResponseStatus). Those exceptions still get unconditionally flagged as errors even when they map to a routine non-5xx response, since the tracer has no way to know what status they carry. DD_TRACE_RESPONSE_STATUS_EXCEPTIONS / trace.response-status.exceptions lets users declare a list of fully.qualified.ExceptionClass#accessorMethod entries; when a thrown exception (or a subclass of one) matches, the named no-arg accessor is invoked reflectively and its numeric return value is used the same way as the built-in status extraction. This keeps the change fully opt-in and precise - it only ever reflects on classes/methods a user explicitly named, rather than guessing at common accessor names across arbitrary exceptions and risking a genuine error being silently cleared. A misconfigured accessor could return a value outside the valid HTTP status range (e.g. -1 as an unknown-status sentinel). That value would otherwise flow straight into Config#getHttpServerErrorStatuses, a BitSet indexed by status code, which throws on a negative index, aborting normal error handling for the span entirely. The accessor return value is now validated as a plausible HTTP status (100-599) before use, falling back to normal error handling otherwise.
40720ea to
93e9b3b
Compare
|
@bric3 @jordan-wong 👋 hi, it's been a week which is longer than the provided expectations so just wanted to check in to see how I can help move this along |
dougqh
left a comment
dougqh
left a comment
There was a problem hiding this comment.
A few things I noticed while reading through the reflective status-extraction path. Nothing blocking IMO, but flagging for a look.
| return status; | ||
| } | ||
| } | ||
| } catch (Throwable ignored) { |
There was a problem hiding this comment.
Catching Throwable here also swallows Errors (e.g. OutOfMemoryError, StackOverflowError) coming out of invoke, not just the reflection failures (IllegalAccessException, InvocationTargetException) the comment describes. Might be worth narrowing to ReflectiveOperationException (or re-throwing Error) so a genuine JVM-level failure isn't silently downgraded to "misconfigured entry".
There was a problem hiding this comment.
Left this as-is — catch (Throwable ignored) around reflective calls is the established idiom throughout this package (BaseDecorator, HttpServerDecorator, HttpClientDecorator all do the same for their own reflection/callback paths), so narrowing just this call site would be inconsistent with everything around it for limited benefit.
| if (httpStatus instanceof HttpStatus) { | ||
| return ((HttpStatus) httpStatus).value(); | ||
| } | ||
| } catch (Throwable ignored) { |
There was a problem hiding this comment.
Same broad catch (Throwable ignored) concern as in ConfiguredResponseStatusExceptions.invoke — this also swallows Errors, not just reflection failures on RESPONSE_STATUS_EXCEPTION_GET_STATUS.invoke.
There was a problem hiding this comment.
Same reasoning as the ConfiguredResponseStatusExceptions thread — left as-is to stay consistent with the rest of the package.
| if (code != HttpStatus.INTERNAL_SERVER_ERROR) { | ||
| return code.value(); | ||
| } | ||
| } catch (Throwable ignored) { |
There was a problem hiding this comment.
Same broad catch (Throwable ignored) concern here on RESPONSE_STATUS_CODE.invoke — swallows Errors along with the intended reflection failures.
There was a problem hiding this comment.
Same reasoning as the ConfiguredResponseStatusExceptions thread — left as-is to stay consistent with the rest of the package.
…or error mapping The bounds check added for ConfiguredResponseStatusExceptions didn't cover each decorator's own built-in status extraction, so an out-of-range status from there could still throw IndexOutOfBoundsException out of BitSet.get() and get silently swallowed by BaseDecorator.onError, leaving spans with exception tags but never flagged as errors. Extract the shared validate/flag/fallback tail of doOnError into one MappedExceptionStatus helper used by all 5 decorators, closing the gap in one place instead of five.
Covers each JAX-RS/Jakarta-RS decorator own extractResponseStatus path (not just the ConfiguredResponseStatusExceptions path, which already had one) with a WebApplicationException carrying a negative status, asserting it falls back to normal error handling instead of throwing.
This branch has not been deployed
2 participants
What Does This Do
WebApplicationException(or asubclass, e.g.
NotFoundException), thejax-rs.request/jakarta-rs.requestspan'serror flag is now decided from the status embedded in the exception's
Response,instead of unconditionally being marked as an error.
@ResponseStatus,ResponseStatusException, and (Spring 6+)ErrorResponse(e.g.NoResourceFoundException) on thespring-web-controllerspan.used for the root HTTP span (
DD_TRACE_HTTP_SERVER_ERROR_STATUSES), so behavior staysconsistent with whatever a user has already configured there.
DD_TRACE_RESPONSE_STATUS_EXCEPTIONS/trace.response-status.exceptionsconfig for exceptions that don't follow any of the above conventions - a comma-separated
list of
fully.qualified.ExceptionClass#accessorMethodentries. When a thrown exception(or a subclass of one) matches, the named no-arg accessor is invoked reflectively and its
numeric return value is used the same way. This is opt-in and precise on purpose: it only
ever reflects on classes/methods a user explicitly named, rather than guessing at common
accessor names across arbitrary exceptions (which risks silently clearing a genuine error).
Motivation
JAX-RS and Spring MVC commonly signal a non-2xx response by throwing an exception that the
framework's own exception-mapping machinery turns into a normal HTTP response (e.g. a 404).
The tracer was flagging the resource/controller span as an error unconditionally whenever
such an exception was thrown, even though the actual HTTP response was a routine non-5xx -
flooding error tracking with non-actionable "errors" for expected control flow. The root
span was never affected by this (it already reflects the real status code correctly), which
is why this showed up as a healthy root span next to an errored child span.
Solves #7141
Solves #7288
Additional Notes
still flagged as errors as before.
DD_TRACE_RESPONSE_STATUS_EXCEPTIONSconfig will need a follow-upupdate in the Datadog docs site.
Contributor Checklist
type:and (comp:orinst:) labels in addition to any other useful labelsclose,fix, or any linking keywords when referencing an issueUse
solvesinstead, and assign the PR milestone to the issue