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 and Feature: standardize config behavior across tracing libs #2873

Merged
merged 13 commits into from
Oct 3, 2024
+387 −57
Merged

Fix and Feature: standardize config behavior across tracing libs #2873

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
48ea87b
Report telemetry for 'config consistency' config items
mtoffl01 Sep 16, 2024
4bd670f
Change http client "RoundTripper" implementation to report errors on …
mtoffl01 Sep 18, 2024
8d242b8
Add support for DD_TRACE_HTTP_CLIENT_TAG_QUERY_STRING
mtoffl01 Sep 23, 2024
e661188
Merge branch 'main' into mtoff/config-consistency
mtoffl01 Sep 23, 2024
240ef30
Support DD_TRACE_HTTP_CLIENT_ERROR_STATUSES in http roundtripper
mtoffl01 Sep 24, 2024
ca9fedd
Add support for DD_TRACE_HTTP_SERVER_ERROR_STATUSES
mtoffl01 Sep 25, 2024
4911dad
Change default client IP headers to include cf-connecting-ipv6
mtoffl01 Sep 27, 2024
11d58d9
Merge branch 'main' into mtoff/config-consistency
mtoffl01 Sep 27, 2024
767938c
Fix defaultErrorCode naming
mtoffl01 Oct 1, 2024
23f311a
Nit: Remove unusued named argument in function
mtoffl01 Oct 2, 2024
0c0254f
Merge branch 'main' into mtoff/config-consistency
mtoffl01 Oct 3, 2024
09b6d5d
Fix urlFromRequest to fetch the active Scheme
mtoffl01 Oct 3, 2024
f20afbb
Merge branch 'main' into mtoff/config-consistency
mtoffl01 Oct 3, 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
7 changes: 4 additions & 3 deletions contrib/net/http/roundtripper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,13 @@ package http

import (
"fmt"
"gopkg.in/DataDog/dd-trace-go.v1/appsec/events"
"math"
"net/http"
"os"
"strconv"

"gopkg.in/DataDog/dd-trace-go.v1/appsec/events"

"gopkg.in/DataDog/dd-trace-go.v1/ddtrace"
"gopkg.in/DataDog/dd-trace-go.v1/ddtrace/ext"
"gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer"
Expand Down Expand Up @@ -94,8 +95,8 @@ func (rt *roundTripper) RoundTrip(req *http.Request) (res *http.Response, err er
}
} else {
span.SetTag(ext.HTTPCode, strconv.Itoa(res.StatusCode))
// treat 5XX as errors
if res.StatusCode/100 == 5 {
// Client spans consider 4xx as errors
if res.StatusCode/100 == 4 {
Copy link
Member

@darccio darccio Oct 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Isn't this a breaking behavioural change? Should we introduce an environment variable to allow keeping the old behaviour or would it be enough to set DD_TRACE_HTTP_CLIENT_ERROR_STATUSES to the right values?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@darccio , yes it is a breaking change, although in my opinion it's a "fix" as the http client spans should always have had 4xx as errors, not 5xx.
We can discuss further.

span.SetTag("http.errors", res.Status)
span.SetTag(ext.Error, fmt.Errorf("%d: %s", res.StatusCode, http.StatusText(res.StatusCode)))
}
Expand Down
59 changes: 37 additions & 22 deletions contrib/net/http/roundtripper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,18 +100,46 @@ func TestRoundTripper(t *testing.T) {
assert.Equal(t, wantPort, s1.Tag(ext.NetworkDestinationPort))
}

func TestRoundTripperServerError(t *testing.T) {
// Assert all error tags exist when status code is 4xx
func TestRoundTripperClientError(t *testing.T) {
mt := mocktracer.Start()
defer mt.Stop()

s := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
spanctx, err := tracer.Extract(tracer.HTTPHeadersCarrier(r.Header))
assert.NoError(t, err)
w.WriteHeader(http.StatusBadRequest)
w.Write([]byte("Error"))
}))
defer s.Close()

span := tracer.StartSpan("test",
tracer.ChildOf(spanctx))
defer span.Finish()
rt := WrapRoundTripper(http.DefaultTransport,
WithBefore(func(req *http.Request, span ddtrace.Span) {
span.SetTag("CalledBefore", true)
}),
WithAfter(func(res *http.Response, span ddtrace.Span) {
span.SetTag("CalledAfter", true)
}))

client := &http.Client{
Transport: rt,
}

resp, err := client.Get(s.URL + "/hello/world")
assert.Nil(t, err)
defer resp.Body.Close()

spans := mt.FinishedSpans()
assert.Len(t, spans, 1)
s1 := spans[0]
assert.Equal(t, "400: Bad Request", s1.Tag(ext.Error).(error).Error())
assert.Equal(t, "400", s1.Tag(ext.HTTPCode))
}

// Assert no error tags are set when status code is 5xx
func TestRoundTripperServerError(t *testing.T) {
mt := mocktracer.Start()
defer mt.Stop()

s := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(http.StatusInternalServerError)
w.Write([]byte("Error"))
}))
Expand All @@ -134,24 +162,11 @@ func TestRoundTripperServerError(t *testing.T) {
defer resp.Body.Close()

spans := mt.FinishedSpans()
assert.Len(t, spans, 2)
assert.Equal(t, spans[0].TraceID(), spans[1].TraceID())

s0 := spans[0]
assert.Equal(t, "test", s0.OperationName())
assert.Equal(t, "test", s0.Tag(ext.ResourceName))
assert.Len(t, spans, 1)

s1 := spans[1]
assert.Equal(t, "http.request", s1.OperationName())
assert.Equal(t, "http.request", s1.Tag(ext.ResourceName))
s1 := spans[0]
assert.Equal(t, "500", s1.Tag(ext.HTTPCode))
assert.Equal(t, "GET", s1.Tag(ext.HTTPMethod))
assert.Equal(t, s.URL+"/hello/world", s1.Tag(ext.HTTPURL))
assert.Equal(t, fmt.Errorf("500: Internal Server Error"), s1.Tag(ext.Error))
assert.Equal(t, true, s1.Tag("CalledBefore"))
assert.Equal(t, true, s1.Tag("CalledAfter"))
assert.Equal(t, ext.SpanKindClient, s1.Tag(ext.SpanKind))
assert.Equal(t, "net/http", s1.Tag(ext.Component))
assert.Empty(t, s1.Tag(ext.Error))
}

func TestRoundTripperNetworkError(t *testing.T) {
Expand Down
Loading