Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix: Don't call c.Error(...) inside echotrace middlewares #2609

Merged
merged 6 commits into from
Apr 10, 2024
Merged

fix: Don't call c.Error(...) inside echotrace middlewares #2609

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d3735fb
Don't call c.Error inside echotrace middlewares
nakkamarra Mar 11, 2024
ea63b88
Merge branch 'main' into fix-issue-2608
rarguelloF Apr 1, 2024
230fae8
Add test to verify broken behavior, test should fail
nakkamarra Apr 1, 2024
74931da
Merge branch 'fix-issue-2608' of github.com:nakkamarra/dd-trace-go in…
nakkamarra Apr 1, 2024
7fc87fe
Remove c.Error invocation from test
nakkamarra Apr 1, 2024
d74b236
Merge branch 'main' into fix-issue-2608
rarguelloF Apr 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 1 addition & 8 deletions contrib/labstack/echo.v4/echotrace.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,11 +52,7 @@ func Middleware(opts ...Option) echo.MiddlewareFunc {
return func(c echo.Context) error {
// If we have an ignoreRequestFunc, use it to see if we proceed with tracing
if cfg.ignoreRequestFunc != nil && cfg.ignoreRequestFunc(c) {
if err := next(c); err != nil {
c.Error(err)
return err
}
return nil
return next(c)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should add some test to verify this behavior and ensure we don't introduce a regression in the future.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rarguelloF Actually, if somebody was relying in the existing behaviour, they probably won't feel a difference. The existing behaviour blocked middlewares running before our own to manipulate the response, as c.Error already commited it to the client.

I think it's safe to go ahead as it is right now.

}

request := c.Request()
Expand Down Expand Up @@ -90,9 +86,6 @@ func Middleware(opts ...Option) echo.MiddlewareFunc {
// serve the request to the next middleware
err := next(c)
if err != nil && !shouldIgnoreError(cfg, err) {
// invokes the registered HTTP error handler
c.Error(err)

// It is impossible to determine what the final status code of a request is in echo.
// This is the best we can do.
if echoErr, ok := cfg.translateError(err); ok {
Expand Down
6 changes: 5 additions & 1 deletion contrib/labstack/echo.v4/echotrace_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,9 +151,13 @@ func TestError(t *testing.T) {
mt := mocktracer.Start()
defer mt.Stop()
var called, traced bool
handlerCalled := 0

// setup
router := echo.New()
router.HTTPErrorHandler = func(err error, c echo.Context) {
handlerCalled++
}
router.Use(Middleware(WithServiceName("foobar")))
wantErr := errors.New("oh no")

Expand All @@ -163,7 +167,6 @@ func TestError(t *testing.T) {
called = true

err := wantErr
c.Error(err)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rarguelloF I've updated the test to ensure that the error handler is called once, and I've removed the c.Error call here as well, since this is a test handler and from the echo docs we don't want to explicitly invoke the c.Error inside of handlers.

// Avoid using this method in handlers as no middleware will be able to effectively handle errors after that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fact that this change has removed this call and not broken anything in the rest of test should hopefully further highlight the fact that the expected behavior is already happening without this call, and explicitly calling it is causing as a second, unnecessary call.

return err
})
r := httptest.NewRequest("GET", "/err", nil)
Expand All @@ -173,6 +176,7 @@ func TestError(t *testing.T) {
// verify the errors and status are correct
assert.True(called)
assert.True(traced)
assert.Equal(1, handlerCalled)

spans := mt.FinishedSpans()
require.Len(t, spans, 1)
Expand Down
3 changes: 0 additions & 3 deletions contrib/labstack/echo/echotrace.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,9 +76,6 @@ func Middleware(opts ...Option) echo.MiddlewareFunc {
// serve the request to the next middleware
err := next(c)
if err != nil {
// invokes the registered HTTP error handler
c.Error(err)

// It is impossible to determine what the final status code of a request is in echo.
// This is the best we can do.
var echoErr *echo.HTTPError
Expand Down
Loading