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.

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

Question: Datadog trace for echo is giving useless trace stack for 4xx and 5xx status code. #987

Closed
ckyko opened this issue Aug 20, 2021 · 7 comments · Fixed by #1000
Closed
Labels
ack bug unintended behavior that has to be fixed question

Comments

@ckyko
Copy link

ckyko commented Aug 20, 2021

Hi, I am using Datadog trace for my go echo server. And, I saw there is a trace stack for 4xx and 5xx status codes on the Datadog APM traces tag tab. But, looks like it is showing the same error stack. And, it is not my error stack. Looks like it is the trace stack where we set the error-tag.

Should we show it if it is not the real error trace stack? Is there any way to disable it for the wrong error trace stack?
Also, I saw the error trace stack on both 'error' tab and 'tag' tab, is it duplicate information?

The trace stack on the Datadog APM traces:

gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer.(*span).setTagError
/Users/kachen/go/pkg/mod/gopkg.in/!data!dog/dd-trace-go.v1@v1.31.1/ddtrace/tracer/span.go:164
gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer.(*span).SetTag
/Users/kachen/go/pkg/mod/gopkg.in/!data!dog/dd-trace-go.v1@v1.31.1/ddtrace/tracer/span.go:107
gopkg.in/DataDog/dd-trace-go.v1/contrib/labstack/echo%2ev4.Middleware.func1.1
/Users/kachen/go/pkg/mod/gopkg.in/!data!dog/dd-trace-go.v1@v1.31.1/contrib/labstack/echo.v4/echotrace.go:55
github.com/labstack/echo/v4.(*Echo).add.func1
/Users/kachen/go/pkg/mod/github.com/labstack/echo/v4@v4.5.0/echo.go:544
github.com/labstack/echo/v4.(*Echo).ServeHTTP
/Users/kachen/go/pkg/mod/github.com/labstack/echo/v4@v4.5.0/echo.go:654
net/http.serverHandler.ServeHTTP
/usr/local/Cellar/go/1.15.8/libexec/src/net/http/server.go:2843
net/http.(*conn).serve
/usr/local/Cellar/go/1.15.8/libexec/src/net/http/server.go:1925
runtime.goexit
/usr/local/Cellar/go/1.15.8/libexec/src/runtime/asm_amd64.s:1374

Here is the code to generate the trace stack:

package main

import (
	"github.com/labstack/echo/v4"
	echotrace "gopkg.in/DataDog/dd-trace-go.v1/contrib/labstack/echo.v4"
	"gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer"
	"net/http"
)

func main() {
	tracer.Start(
		tracer.WithEnv("test"),
		tracer.WithService("test-go"),
		tracer.WithServiceVersion("abc123"),
	)

	defer tracer.Stop()

	e := echo.New()
	topGroup := e.Group("/top", echotrace.Middleware())
	subGroup := topGroup.Group("/sub")
	subGroup.Add("GET", "/foo/:bar", func(c echo.Context) error {
		return echo.NewHTTPError(http.StatusBadRequest, "my error message")
	})

	e.Logger.Fatal(e.Start(":1325"))

}

span.SetTag(ext.Error, err)

s.setTagError(value, errorConfig{

@dubonzi
Copy link
Contributor

dubonzi commented Aug 21, 2021

I might be wrong, but I believe this line of the stack

github.com/labstack/echo/v4.(*Echo).add.func1
/Users/kachen/go/pkg/mod/github.com/labstack/echo/v4@v4.5.0/echo.go:544

refers to your handler defined in

subGroup.Add("GET", "/foo/:bar", func(c echo.Context) error {
	return echo.NewHTTPError(http.StatusBadRequest, "my error message")
})

Its not clear because its an anonymous function. Try defining a actual function and check the stack again. Something like

subGroup.Add("GET", "/foo/:bar", fooHandler)

func fooHandler(c echo.Context) error {
	return echo.NewHTTPError(http.StatusBadRequest, "my error message")
}

@ckyko
Copy link
Author

ckyko commented Aug 23, 2021

I try the actual function and the stack looks the same.

gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer.(*span).setTagError
/Users/kachen/go/pkg/mod/gopkg.in/!data!dog/dd-trace-go.v1@v1.31.1/ddtrace/tracer/span.go:164
gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer.(*span).SetTag
/Users/kachen/go/pkg/mod/gopkg.in/!data!dog/dd-trace-go.v1@v1.31.1/ddtrace/tracer/span.go:107
gopkg.in/DataDog/dd-trace-go.v1/contrib/labstack/echo%2ev4.Middleware.func1.1
/Users/kachen/go/pkg/mod/gopkg.in/!data!dog/dd-trace-go.v1@v1.31.1/contrib/labstack/echo.v4/echotrace.go:55
github.com/labstack/echo/v4.(*Echo).add.func1
/Users/kachen/go/pkg/mod/github.com/labstack/echo/v4@v4.5.0/echo.go:544
github.com/labstack/echo/v4.(*Echo).ServeHTTP
/Users/kachen/go/pkg/mod/github.com/labstack/echo/v4@v4.5.0/echo.go:654
net/http.serverHandler.ServeHTTP
/usr/local/Cellar/go/1.15.8/libexec/src/net/http/server.go:2843
net/http.(*conn).serve
/usr/local/Cellar/go/1.15.8/libexec/src/net/http/server.go:1925
runtime.goexit
/usr/local/Cellar/go/1.15.8/libexec/src/runtime/asm_amd64.s:1374
package main

import (
	"github.com/labstack/echo/v4"
	echotrace "gopkg.in/DataDog/dd-trace-go.v1/contrib/labstack/echo.v4"
	"gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer"
	"net/http"
)

func main() {
	tracer.Start(
		tracer.WithEnv("test"),
		tracer.WithService("test-go"),
		tracer.WithServiceVersion("abc123"),
	)

	defer tracer.Stop()

	e := echo.New()
	topGroup := e.Group("/top", echotrace.Middleware())
	subGroup := topGroup.Group("/sub")
	subGroup.Add("GET", "/foo/:bar", fooHandler)

	e.Logger.Fatal(e.Start(":1325"))

}

func fooHandler(c echo.Context) error {
	return echo.NewHTTPError(http.StatusBadRequest, "my error message")
}

The stack doesn't show my handler.

@knusbaum
Copy link
Contributor

knusbaum commented Aug 26, 2021

Hi, @ckyko.
Stack trace collecting is done on a best-effort basis. We can't know the full stack of an error since that information doesn't get propagated with the errors. Instead, we collect stack traces as close to the error as we can get (span.Finish). Sometimes these are useful, but other times they are completely unhelpful.

Also, I saw the error trace stack on both 'error' tab and 'tag' tab, is it duplicate information?

The information is presented in two places in the UI, but the information is not duplicated in the span (there is only one copy of the data being sent from the application). The UI just chooses to display the error tag specially.

You can consider using the WithDebugStack option, which will disable collection of stack traces for the entire program.

If that's not an acceptable solution, we can add an Option to the echo integration similar to NoDebugStack from goji or grpc and several others.

@ckyko
Copy link
Author

ckyko commented Sep 7, 2021

Thanks, I don't think disabling stack traces for the entire program is a good solution. I would expect there is an option to disable the stack traces when the traces are completely unhelpful.

@knusbaum
Copy link
Contributor

knusbaum commented Sep 8, 2021

@ckyko No problem, I've opened a proposal (#999) to add that option.

@knusbaum
Copy link
Contributor

knusbaum commented Sep 8, 2021

I looked more closely into this. Aside from the stack traces not being useful, the echo integration is doing some things wrongly that it shouldn't:

Unfortunately, we can't know the final value of the status code or the final error, but we can make a good guess based on what is returned from the downstream middleware and handlers.

knusbaum added a commit that referenced this issue Sep 8, 2021
This adds improved error detection to the echo integrations. Previously,
any returned error was recorded as an error in the echo span. This patch
causes the integration to inspect the returned error to determine if it is
a echo.HTTPError which it can use to discriminate between real errors (5xx
and up) or non-errors (4xx and below)

Fixes #987
@knusbaum knusbaum added the bug unintended behavior that has to be fixed label Sep 8, 2021
@knusbaum knusbaum added the ack label Dec 16, 2021
@kevin521
Copy link

kevin521 commented Oct 6, 2022

Any resolution on this? We're trying to clean up our APM and only have 500s for our echo implementation.

Happy to help in any way to get the PR merged.

knusbaum added a commit that referenced this issue Dec 8, 2022
This adds improved error detection to the echo integrations. Previously,
any returned error was recorded as an error in the echo span. This patch
causes the integration to inspect the returned error to determine if it is
a echo.HTTPError which it can use to discriminate between real errors (5xx
and up) or non-errors (4xx and below)

Fixes #987
knusbaum added a commit that referenced this issue Dec 9, 2022
This adds improved error detection to the echo integrations. Previously,
any returned error was recorded as an error in the echo span. This patch
causes the integration to inspect the returned error to determine if it is
a echo.HTTPError which it can use to discriminate between real errors (5xx
and up) or non-errors (4xx and below)

Fixes #987
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ack bug unintended behavior that has to be fixed question
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants