contrib/gin-gonic/gin: incorrect 200 status upon panic recovery hides real problems

[seiyab]

I'm not sure that this issue is a bug report, a feature request or something else. Anyway, I think the current behavior can be problematic under practical situations. Indeed, I couldn't find failed requests when an incident happened.

https://github.com/DataDog/dd-trace-go/blob/25044cfed84f67e5283810a133973f880a5c5c72/contrib/internal/httptrace/httptrace.go#L59-L60

Version of dd-trace-go

v1.49.1

Describe what happened: Trace for a request that returns 500 error was recorded as 200.

Describe what you expected: It is recorded as 500, or a special one like "unknown".

Steps to reproduce the issue:

1. Use gin and its Recovery middleware.
   • NOTE: We want to use Recovery before dd-trace, because we want to recover not only when handler panics but also when dd-trace panics.
2. Let handler panic.

dd-trace sees status code 0 and record it as 200. Then, Recovery middleware sets status code 500.

Additional environment details (Version of Go, Operating System, etc.): N/A

History that I investigated

• As https://github.com/DataDog/dd-trace-go/pull/740, status code 0 had been considered as 200 only for chi.
• https://github.com/DataDog/dd-trace-go/issues/745 was created (but no progress?).
• https://github.com/DataDog/dd-trace-go/pull/1286 looks to have changed behavior applying https://github.com/DataDog/dd-trace-go/pull/740 into all the contrib packages.

[seiyab]

Anyone?

[katiehockman]

@Julio-Guerra it looks like this is something that changed ~a year ago based on a PR you contributed. The current code uses the ResponseWriter's status code to determine the http.status_code, which isn't playing nicely with Recovery middleware. LMK if this is something you'd like to look into, or if you want to pass ownership over to @DataDog/apm-ecosystems

[seiyab]

Any update?