contrib/internal/httptrace: proposal: Add option to ignore setting errors for 5xx HTTP status codes

[kazukousen]

Only errors located in the uppermost service span are processed by ErrorTracking.

We use tracer.SetTag(ext.Error, err) to record error in the uppermost service span.

However, the gorilla/mux tracer we use which internally uses http tracer, also overrides the error with tracer.SetTag() when the HTTP Errors (5xx) occur.

dd-trace-go/contrib/internal/httptrace/httptrace.go

Lines 74 to 76 in 6becedc

	if status >= 500 && status < 600 {
	s.SetTag(ext.Error, fmt.Errorf("%s: %s", statusStr, http.StatusText(status)))
	}

As a result, ErrorTracking groups all errors into a generic 5xx error issue, displays examples like 500: Internel Server Error in its web UI.

We'd like discuss about adding an option to ignore setting the error. this could be implemented as follows:

if !cfg.disabledSetHTTPError && status >= 500 && status < 600 {
	s.SetTag(ext.Error, fmt.Errorf("%s: %s", statusStr, http.StatusText(status)))
}

[kazukousen]

I worked on #2432

@ajgajg1134 @katiehockman Hi, folks. I would be grateful if you could take some time to review it.

[katiehockman]

Hi @kazukousen, thanks for filing this issue. We've been discussing this, and wonder if we should instead verify that there is not already an error status code and error tag on the span before we overwrite it. We're not sure that there is a good use case for overriding it, so rather than make this configurable, we should probably just fix this. Let us know what you think, and if this would fix your issue.

We'll work on this and add some tests, and get back in touch with you.

[katiehockman]

We're discussing adding a Tag(key string) interface{} method to the Span struct, which given a key would return the tag value. This would allow us to check if there is already an error tag here, and if there is, either 1) don't change the error or 2) concatenate the error so it doesn't get lost. That will also avoid needing additional configuration.

[kazukousen]

Hi @katiehockman, Thank you for your reply.

We've been discussing this, and wonder if we should instead verify that there is not already an error status code and error tag on the span before we overwrite it. We're not sure that there is a good use case for overriding it, so rather than make this configurable, we should probably just fix this. Let us know what you think, and if this would fix your issue.

That's make sense for us. It is clear. Thanks!

[threesquared]

Was anything ever done about this? It seems like #2523 was eventually reverted?

We are also using the gorilla/mux tracer and all the errors we see are this generic 500: Internel Server Error with a very unhelpful stack trace.

[mtoffl01]

Hi there,

We are still discussing how to handle the scenario where the tracer assigns an error to span that already has one. In the meantime, you can technically disable the tracer's automatic error status setter with the DD_TRACE_HTTP_SERVER_ERROR_STATUSES option, as of of v1.69.0. If set to a value that does not show up in your request/response pattern, the tracer will never set an error status on your HTTP server spans, effectively never overwriting your custom error. Note that this would apply to multiple HTTP integrations.

Some HTTP integrations support integration-level error code configuration, e.g, negroni and work is currently in progress to support this in the net/http contrib package. This is not yet supported nor in progress for the gorilla/mux integration. If you believe integration-level support for gorilla/mux would help you, let us know and we can consider prioritizing it as a temporary fix for you.