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

contrib: fix span start option races #2418

Merged
merged 23 commits into from
Dec 12, 2023
Merged

contrib: fix span start option races #2418

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
8f24d7b
contrib/*: fix multiple spanOpts potential append() data race
eliottness Dec 7, 2023
2ef8ddc
fix start span option races
Dec 7, 2023
5e8ff05
fix echo span tag ordering
Dec 7, 2023
1b1eba1
fix chi options
Dec 7, 2023
260f6d8
refactor to make copies more explicit and add helper method
katiehockman Dec 8, 2023
9778ffe
move OptionsCopy to a new contrib-wide internal package
katiehockman Dec 8, 2023
f24ebd3
add a unit test for OptionsCopy
katiehockman Dec 8, 2023
694bf1f
rename options.OptionsCopy to options.Copy to avoid repetitive naming
katiehockman Dec 8, 2023
4fe86a7
fix additional races in other packages
katiehockman Dec 8, 2023
346aa7c
gofmt on roundtripper.go
katiehockman Dec 8, 2023
64fd168
clarify godoc for options.Copy
katiehockman Dec 8, 2023
5bc5a22
small cleanups to avoid unnecessary changes
katiehockman Dec 8, 2023
bcb9b19
revert changes to fiber.go and roundtripper.go
katiehockman Dec 8, 2023
ddf67cd
avoid opts change side effects in goji.go and httptreemux.go
katiehockman Dec 8, 2023
7b0ae77
fix httptreemux.go based on https://go.dev/play/p/bcPus1ofHPd
katiehockman Dec 8, 2023
0b9c2db
remove all changes that aren't strictly necessary to fix the race
katiehockman Dec 11, 2023
bb7a30d
remove unecessary changes in httptreemux.go
katiehockman Dec 11, 2023
4b0e472
fix TraceAndServe in contrib/net/http
katiehockman Dec 11, 2023
492981b
Merge branch 'main' into eliott.bouhana/fix-span-opts-race
katiehockman Dec 11, 2023
01dd225
allocate enough space in the destination slice to make the fully copy
katiehockman Dec 11, 2023
1ab2a4c
Merge branch 'eliott.bouhana/fix-span-opts-race' of github.com:DataDo…
katiehockman Dec 11, 2023
3c089f2
remove unnecessary changes in goji.go
katiehockman Dec 12, 2023
d9b7e10
Merge branch 'main' into eliott.bouhana/fix-span-opts-race
katiehockman Dec 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion contrib/database/sql/sql.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,7 @@ func Open(driverName, dataSourceName string, opts ...Option) (*sql.DB, error) {
return nil, err
}
// since we're not using the dsnConnector, we need to register the dsn manually in the config
var optsCopy []Option
optsCopy := make([]Option, len(opts))
copy(optsCopy, opts) // avoid modifying the provided opts, so make a copy instead, and use this
optsCopy = append(optsCopy, WithDSN(dataSourceName))
return OpenDB(connector, optsCopy...), nil
Expand Down
3 changes: 0 additions & 3 deletions contrib/dimfeld/httptreemux.v5/httptreemux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import (
"net/http"
"strings"

"gopkg.in/DataDog/dd-trace-go.v1/contrib/internal/options"
httptrace "gopkg.in/DataDog/dd-trace-go.v1/contrib/net/http"
"gopkg.in/DataDog/dd-trace-go.v1/ddtrace/ext"
"gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer"
Expand Down Expand Up @@ -40,7 +39,6 @@ func New(opts ...RouterOption) *Router {
for _, fn := range opts {
fn(cfg)
}
cfg.spanOpts = options.Copy(cfg.spanOpts...)
cfg.spanOpts = append(cfg.spanOpts, tracer.Measured())
cfg.spanOpts = append(cfg.spanOpts, tracer.Tag(ext.SpanKind, ext.SpanKindServer))
cfg.spanOpts = append(cfg.spanOpts, tracer.Tag(ext.Component, componentName))
Expand Down Expand Up @@ -76,7 +74,6 @@ func NewWithContext(opts ...RouterOption) *ContextRouter {
for _, fn := range opts {
fn(cfg)
}
cfg.spanOpts = options.Copy(cfg.spanOpts...)
cfg.spanOpts = append(cfg.spanOpts, tracer.Measured())
cfg.spanOpts = append(cfg.spanOpts, tracer.Tag(ext.SpanKind, ext.SpanKindServer))
cfg.spanOpts = append(cfg.spanOpts, tracer.Tag(ext.Component, componentName))
Expand Down
10 changes: 4 additions & 6 deletions contrib/go-chi/chi.v5/chi.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,21 +38,19 @@ func Middleware(opts ...Option) func(next http.Handler) http.Handler {
fn(cfg)
}
log.Debug("contrib/go-chi/chi.v5: Configuring Middleware: %#v", cfg)
spanOpts := options.Copy(cfg.spanOpts...) // spanOpts must be a copy of cfg.spanOpts, locally scoped, to avoid races.
spanOpts = append(spanOpts,
tracer.ServiceName(cfg.serviceName),
spanOpts := append(cfg.spanOpts, tracer.ServiceName(cfg.serviceName),
tracer.Tag(ext.Component, componentName),
tracer.Tag(ext.SpanKind, ext.SpanKindServer))
if !math.IsNaN(cfg.analyticsRate) {
spanOpts = append(spanOpts, tracer.Tag(ext.EventSampleRate, cfg.analyticsRate))
}
return func(next http.Handler) http.Handler {
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
if cfg.ignoreRequest(r) {
next.ServeHTTP(w, r)
return
}
opts := options.Copy(spanOpts...) // opts must be a copy of spanOpts, locally scoped, to avoid races.
if !math.IsNaN(cfg.analyticsRate) {
opts = append(opts, tracer.Tag(ext.EventSampleRate, cfg.analyticsRate))
}
opts = append(opts, httptrace.HeaderTagsFromRequest(r, cfg.headerTags))
span, ctx := httptrace.StartRequestSpan(r, opts...)
ww := middleware.NewWrapResponseWriter(w, r.ProtoMajor)
Expand Down
9 changes: 4 additions & 5 deletions contrib/go-chi/chi/chi.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,20 +38,19 @@ func Middleware(opts ...Option) func(next http.Handler) http.Handler {
fn(cfg)
}
log.Debug("contrib/go-chi/chi: Configuring Middleware: %#v", cfg)
spanOpts := options.Copy(cfg.spanOpts...) // spanOpts must be a copy of cfg.spanOpts, locally scoped, to avoid races.
spanOpts = append(spanOpts, tracer.ServiceName(cfg.serviceName),
spanOpts := append(cfg.spanOpts, tracer.ServiceName(cfg.serviceName),
katiehockman marked this conversation as resolved.
Show resolved Hide resolved
tracer.Tag(ext.Component, componentName),
tracer.Tag(ext.SpanKind, ext.SpanKindServer))
if !math.IsNaN(cfg.analyticsRate) {
spanOpts = append(spanOpts, tracer.Tag(ext.EventSampleRate, cfg.analyticsRate))
}
return func(next http.Handler) http.Handler {
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
if cfg.ignoreRequest(r) {
next.ServeHTTP(w, r)
return
}
opts := options.Copy(spanOpts...) // opts must be a copy of spanOpts, locally scoped, to avoid races.
if !math.IsNaN(cfg.analyticsRate) {
opts = append(opts, tracer.Tag(ext.EventSampleRate, cfg.analyticsRate))
}
opts = append(opts, httptrace.HeaderTagsFromRequest(r, cfg.headerTags))
span, ctx := httptrace.StartRequestSpan(r, opts...)
ww := middleware.NewWrapResponseWriter(w, r.ProtoMajor)
Expand Down
2 changes: 1 addition & 1 deletion contrib/k8s.io/client-go/kubernetes/kubernetes.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ func WrapRoundTripper(rt http.RoundTripper) http.RoundTripper {
}

func wrapRoundTripperWithOptions(rt http.RoundTripper, opts ...httptrace.RoundTripperOption) http.RoundTripper {
var localOpts []httptrace.RoundTripperOption
localOpts := make([]httptrace.RoundTripperOption, len(opts))
copy(localOpts, opts) // make a copy of the opts, to avoid data races and side effects.
localOpts = append(localOpts, httptrace.WithBefore(func(req *http.Request, span ddtrace.Span) {
knusbaum marked this conversation as resolved.
Show resolved Hide resolved
span.SetTag(ext.ResourceName, RequestToResource(req.Method, req.URL.Path))
Expand Down
9 changes: 4 additions & 5 deletions contrib/labstack/echo.v4/echotrace.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,18 +40,14 @@ func Middleware(opts ...Option) echo.MiddlewareFunc {
}
log.Debug("contrib/labstack/echo.v4: Configuring Middleware: %#v", cfg)
spanOpts := make([]ddtrace.StartSpanOption, 0, 3+len(cfg.tags))
spanOpts = append(spanOpts, tracer.ServiceName(cfg.serviceName))
for k, v := range cfg.tags {
spanOpts = append(spanOpts, tracer.Tag(k, v))
}
spanOpts = append(spanOpts,
tracer.ServiceName(cfg.serviceName),
tracer.Tag(ext.Component, componentName),
tracer.Tag(ext.SpanKind, ext.SpanKindServer),
)
if !math.IsNaN(cfg.analyticsRate) {
spanOpts = append(spanOpts, tracer.Tag(ext.EventSampleRate, cfg.analyticsRate))
}

return func(next echo.HandlerFunc) echo.HandlerFunc {
return func(c echo.Context) error {
// If we have an ignoreRequestFunc, use it to see if we proceed with tracing
Expand All @@ -67,6 +63,9 @@ func Middleware(opts ...Option) echo.MiddlewareFunc {
route := c.Path()
resource := request.Method + " " + route
opts := options.Copy(spanOpts...) // opts must be a copy of spanOpts, locally scoped, to avoid races.
if !math.IsNaN(cfg.analyticsRate) {
opts = append(opts, tracer.Tag(ext.EventSampleRate, cfg.analyticsRate))
}
opts = append(opts,
tracer.ResourceName(resource),
tracer.Tag(ext.HTTPRoute, route),
Expand Down
6 changes: 3 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 @@ -47,14 +47,14 @@ func Middleware(opts ...Option) echo.MiddlewareFunc {
tracer.Tag(ext.Component, componentName),
tracer.Tag(ext.SpanKind, ext.SpanKindServer),
}
if !math.IsNaN(cfg.analyticsRate) {
spanOpts = append(spanOpts, tracer.Tag(ext.EventSampleRate, cfg.analyticsRate))
}
return func(next echo.HandlerFunc) echo.HandlerFunc {
return func(c echo.Context) error {
request := c.Request()
resource := request.Method + " " + c.Path()
opts := options.Copy(spanOpts...) // opts must be a copy of spanOpts, locally scoped, to avoid races.
if !math.IsNaN(cfg.analyticsRate) {
opts = append(opts, tracer.Tag(ext.EventSampleRate, cfg.analyticsRate))
}
opts = append(opts,
tracer.ResourceName(resource),
httptrace.HeaderTagsFromRequest(request, cfg.headerTags))
Expand Down
3 changes: 2 additions & 1 deletion contrib/net/http/trace.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"net/http"

"gopkg.in/DataDog/dd-trace-go.v1/contrib/internal/httptrace"
"gopkg.in/DataDog/dd-trace-go.v1/contrib/internal/options"
"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 @@ -53,7 +54,7 @@ func TraceAndServe(h http.Handler, w http.ResponseWriter, r *http.Request, cfg *
if cfg == nil {
cfg = new(ServeConfig)
}
opts := cfg.SpanOpts
opts := options.Copy(cfg.SpanOpts...) // make a copy of cfg.SpanOpts to avoid races
if cfg.Service != "" {
opts = append(opts, tracer.ServiceName(cfg.Service))
}
Expand Down
4 changes: 1 addition & 3 deletions contrib/zenazn/goji.v1/web/goji.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,7 @@ func Middleware(opts ...Option) func(*web.C, http.Handler) http.Handler {
warnonce sync.Once
)
defaults(&cfg)
localOpts := make([]Option, len(opts))
copy(localOpts, opts) // make a copy to avoid changes to opts having side effects to the returned router
for _, fn := range localOpts {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need this one anymore.

for _, fn := range opts {
fn(&cfg)
}
if !math.IsNaN(cfg.analyticsRate) {
Expand Down