feat: tracing improvements #4988
Conversation
|
This Pull Request is becoming stale. In order to minimize WIP, prevent merge conflicts and keep the tracker readable, I'm going close to this PR in 3 days if there isn't more activity. |
| httpmw.Recover(api.Logger), | ||
| tracing.Middleware(api.TracerProvider), |
There was a problem hiding this comment.
This will trace every static HTTP file... is that ok?
There was a problem hiding this comment.
There's a filter within the middleware itself (and a test) to ensure that this is skipped on non-API routes.
|
|
||
| sw, ok := rw.(*tracing.StatusWriter) | ||
| if !ok { | ||
| panic(fmt.Sprintf("ResponseWriter not a *tracing.StatusWriter; got %T", rw)) |
There was a problem hiding this comment.
It's runtime, right? Isn't it risky to panic here? I understand that you assume that it will be recovered.
There was a problem hiding this comment.
The response writer will always be a *tracing.StatusWriter, so I think it's fine for us to panic here as this is a developer error. We panic in a other places due to developer errors as well, like the middleware context value stuff also in the httpmw package.
We also already do this exact panic in the tracing middleware too
There was a problem hiding this comment.
I admit that I'm not a fan of it, but I understand why it's safe here 👍
tracing.RunWithoutSpanwhich runs the given function with a noop span attached to the context. This is used in a few places to avoid capturing logs as events in the span when the span already had all of the information contained in the log.