feat(contrib/gin-gonic/gin): add WithStatusCheck and WithUseGinErrors options #3984
Conversation
theo303
force-pushed
the
gin-enhance-span-error-handling
branch
2 times, most recently
from
d4643e4 to
f28e644
Compare
contrib/gin-gonic/gin/gintrace.go
Outdated
| defer func() { | ||
| finishSpans(c.Writer.Status(), nil) | ||
| status := c.Writer.Status() | ||
| err := c.Errors.Last() |
There was a problem hiding this comment.
- Can c.Errors be nil?
- In the case where ErrorPropagation has been configured (set to true), does a user always want the last error in the chain to be surfaced as the tag? If not, maybe there's a better, more descriptive name for this option.
There was a problem hiding this comment.
You are right !
I just added a commit to join errors instead of using .Last(). It should solve the nil issue (I added a test case) and be more intuitive I think.
Tell me if you think Last was a better solution, and I will find a more descriptive name like you suggested.
There was a problem hiding this comment.
I've just learned that you can call method on nil pointers. So the solution with .Last() did work, even if c.Errors was nil
Don't enable the error propagation by default, so it's stay the same as before.
theo303
force-pushed
the
gin-enhance-span-error-handling
branch
from
f28e644 to
92daacb
Compare
contrib/gin-gonic/gin/option.go
Outdated
|
|
||
| // WithErrorPropagation enables the propagation of gin's errors to the span. | ||
| // If there are multiple errors in the gin context, they will be all added to the span. | ||
| func WithErrorPropagation() OptionFn { |
There was a problem hiding this comment.
I feel like this name could be slightly confusing to other users (it was not immediate clear to me what it was before reading the description).
WDYT about WithUseGinErrors?
contrib/gin-gonic/gin/gintrace.go
Outdated
| for _, e := range c.Errors { | ||
| err = errors.Join(err, e.Err) | ||
| } |
There was a problem hiding this comment.
we are already setting gin.errors equal to c.Errors.String() at line 80. Could we use the same strategy here?
Additionally, what if isStatusError and propagateError both return true, but c.Errors has length 0? is that possible?
In that case we should fallback to the previous method as well (crafting a custom error based on the status code), so you should additionally check in the condition && len(c.Errors) > 0
There was a problem hiding this comment.
Ah yes, you are right I missed that.
c.Errors has length 0 is possible, in this case err will be nil, and therefore tracer.WithError(err) will do nothing. In this case the custom error based on the status code will be crafted. I think it is exactly what happens in the second test case that I added.
Do you think this behavior is not obvious and the condition should be added ?
There was a problem hiding this comment.
nevermind, using c.Errors().String(), the condition should be added
rarguelloF
changed the title
rarguelloF
changed the title
theo303
force-pushed
the
gin-enhance-span-error-handling
branch
from
300f983 to
b7f83ac
Compare
3 participants
What does this PR do?
The goal is to have error from the gin context in the ext.Error span tag instead of the "500: Internal Server Error" that we currently have. This propagation is disabled by default, so the behavior stay the same for users that don't want that.
Motivation
Fixes #2815
Reviewer's Checklist
./scripts/lint.shlocally.