diff --git a/_integration-tests/tests/net_http/net_http.go b/_integration-tests/tests/net_http/net_http.go index fe443888..25a220be 100644 --- a/_integration-tests/tests/net_http/net_http.go +++ b/_integration-tests/tests/net_http/net_http.go @@ -26,6 +26,22 @@ type TestCase struct { *http.Server } +type handler struct{} + +func (h handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { + defer r.Body.Close() + + b, err := io.ReadAll(r.Body) + if err != nil { + w.WriteHeader(http.StatusBadRequest) + w.Write([]byte(err.Error())) + return + } + + w.WriteHeader(http.StatusOK) + w.Write(b) +} + func (tc *TestCase) Setup(t *testing.T) { mux := http.NewServeMux() tc.Server = &http.Server{ @@ -35,6 +51,21 @@ func (tc *TestCase) Setup(t *testing.T) { mux.HandleFunc("/hit", tc.handleHit) mux.HandleFunc("/", tc.handleRoot) + // TODO: hit these endpoints, check for spans + mux.Handle("/handler", handler{}) + mux.HandleFunc("/literal", func(w http.ResponseWriter, r *http.Request) { + defer r.Body.Close() + + b, err := io.ReadAll(r.Body) + if err != nil { + w.WriteHeader(http.StatusBadRequest) + w.Write([]byte(err.Error())) + return + } + + w.WriteHeader(http.StatusOK) + w.Write(b) + }) go func() { assert.ErrorIs(t, tc.Server.ListenAndServe(), http.ErrServerClosed) }() } diff --git a/instrument/http.go b/instrument/http.go index 7f5e722c..a90b7627 100644 --- a/instrument/http.go +++ b/instrument/http.go @@ -21,6 +21,24 @@ func resourceNamer(r *http.Request) string { return fmt.Sprintf("%s %s", r.Method, r.URL.Path) } +// MaybeWrapHandler instruments an http.Handler if it is nil or an *http.ServeMux, +// and otherwise returns the same handler. +// This is intended for instrumenting http.ListenAndServe, etc, +// where we may already be passed an instrumented handler. +// We assume it is already instrumented unless it's a net/http handler. +func MaybeWrapHandler(handler http.Handler) http.Handler { + if handler == nil { + handler = http.DefaultServeMux + } + switch handler.(type) { + case *http.ServeMux, *http.HandlerFunc: + return WrapHandler(handler) + default: + // Don't change it, it may have already been instrumented + return handler + } +} + func WrapHandler(handler http.Handler) http.Handler { return httptrace.WrapHandler(handler, "", "", httptrace.WithResourceNamer(resourceNamer)) // TODO: We'll reintroduce this later when we stop hard-coding dd-trace-go as above. diff --git a/internal/injector/builtin/generated.go b/internal/injector/builtin/generated.go index 07d9618d..50502878 100644 --- a/internal/injector/builtin/generated.go +++ b/internal/injector/builtin/generated.go @@ -768,9 +768,6 @@ var Aspects = [...]aspect.Aspect{ // From stdlib/net-http.server.yml { JoinPoint: join.AllOf( - join.Configuration(map[string]string{ - "httpmode": "wrap", - }), join.StructLiteralField(join.MustTypeName("net/http.Server"), "Handler"), join.Not(join.OneOf( join.ImportPath("github.com/go-chi/chi/v5"), @@ -780,7 +777,7 @@ var Aspects = [...]aspect.Aspect{ ), Advice: []advice.Advice{ advice.WrapExpression(code.MustTemplate( - "//dd:startwrap\ninstrument.WrapHandler({{.}})\n//dd:endwrap", + "//dd:startwrap\ninstrument.MaybeWrapHandler({{.}})\n//dd:endwrap", map[string]string{ "instrument": "github.com/DataDog/orchestrion/instrument", }, @@ -788,26 +785,13 @@ var Aspects = [...]aspect.Aspect{ }, }, { - JoinPoint: join.AllOf( - join.Configuration(map[string]string{ - "httpmode": "wrap", - }), - join.Function( - join.Name(""), - join.Signature( - []join.TypeName{join.MustTypeName("net/http.ResponseWriter"), join.MustTypeName("*net/http.Request")}, - nil, - ), - ), - join.Not(join.OneOf( - join.ImportPath("github.com/go-chi/chi/v5"), - join.ImportPath("github.com/go-chi/chi/v5/middleware"), - join.ImportPath("golang.org/x/net/http2"), - )), + JoinPoint: join.OneOf( + join.FunctionCall("net/http.ListenAndServe"), + join.FunctionCall("net/http.Serve"), ), Advice: []advice.Advice{ advice.WrapExpression(code.MustTemplate( - "instrument.WrapHandlerFunc({{.}})", + "{{.AST.Fun}}(\n {{ index .AST.Args 0}},\n instrument.MaybeWrapHandler({{ index .AST.Args 1 }}),\n)", map[string]string{ "instrument": "github.com/DataDog/orchestrion/instrument", }, @@ -815,27 +799,14 @@ var Aspects = [...]aspect.Aspect{ }, }, { - JoinPoint: join.AllOf( - join.Configuration(map[string]string{ - "httpmode": "report", - }), - join.FunctionBody(join.Function( - join.Signature( - []join.TypeName{join.MustTypeName("net/http.ResponseWriter"), join.MustTypeName("*net/http.Request")}, - nil, - ), - )), - join.Not(join.OneOf( - join.ImportPath("github.com/go-chi/chi/v5"), - join.ImportPath("github.com/go-chi/chi/v5/middleware"), - join.ImportPath("golang.org/x/net/http2"), - )), + JoinPoint: join.OneOf( + join.FunctionCall("net/http.ListenAndServeTLS"), + join.FunctionCall("net/http.ServeTLS"), ), Advice: []advice.Advice{ - advice.PrependStmts(code.MustTemplate( - "{{- $arg := .Function.Argument 1 -}}\n{{- $name := .Function.Name -}}\n{{$arg}} = {{$arg}}.WithContext(instrument.Report(\n {{$arg}}.Context(),\n event.EventStart,\n {{with $name}}\"function-name\", {{printf \"%q\" .}},{{end}}\n \"span.kind\", \"server\",\n \"http.method\", {{$arg}}.Method,\n \"http.url\", {{$arg}}.URL,\n \"http.useragent\", {{$arg}}.Header.Get(\"User-Agent\"),\n {{ range .DirectiveArgs \"dd:span\" -}}{{printf \"%q, %q,\\n\" .Key .Value}}{{ end }}\n))\ndefer instrument.Report(\n {{$arg}}.Context(),\n event.EventEnd,\n {{with $name}}\"function-name\", {{printf \"%q\" .}},{{end}}\n \"span.kind\", \"server\",\n \"http.method\", {{$arg}}.Method,\n \"http.url\", {{$arg}}.URL,\n \"http.useragent\", {{$arg}}.Header.Get(\"User-Agent\"),\n {{ range .DirectiveArgs \"dd:span\" -}}{{printf \"%q, %q,\" .Key .Value}}{{- end }}\n)", + advice.WrapExpression(code.MustTemplate( + "{{.AST.Fun}}(\n {{ index .AST.Args 0}},\n {{ index .AST.Args 1}},\n {{ index .AST.Args 2}},\n instrument.MaybeWrapHandler({{ index .AST.Args 3 }}),\n)", map[string]string{ - "event": "github.com/DataDog/orchestrion/instrument/event", "instrument": "github.com/DataDog/orchestrion/instrument", }, )), @@ -906,7 +877,6 @@ var InjectedPaths = [...]string{ "context", "fmt", "github.com/DataDog/orchestrion/instrument", - "github.com/DataDog/orchestrion/instrument/event", "github.com/DataDog/orchestrion/instrument/net/http", "gopkg.in/DataDog/dd-trace-go.v1/appsec/events", "gopkg.in/DataDog/dd-trace-go.v1/contrib/99designs/gqlgen", @@ -960,4 +930,4 @@ var InjectedPaths = [...]string{ } // Checksum is a checksum of the built-in configuration which can be used to invalidate caches. -const Checksum = "sha512:pXFHjupxWE+tdgFLK/03Q7H+ireSfSHPXx8ETxc5ZuHABZLZc1DPOpFxCcZmOP2tCoSAc+K6iWr3iwKmJE3/qQ==" +const Checksum = "sha512:aqg9Dgqq1vXx4veNMV6aYcOFjwCgIlfmf5rTl5mnioph9LsDCpi2h4XYNHXx+yZ23KmPcgJP6ozzmtNOfSvjGA==" diff --git a/internal/injector/builtin/generated_deps.go b/internal/injector/builtin/generated_deps.go index e4fc0ba5..194a7752 100644 --- a/internal/injector/builtin/generated_deps.go +++ b/internal/injector/builtin/generated_deps.go @@ -13,7 +13,6 @@ import ( _ "context" _ "fmt" _ "github.com/DataDog/orchestrion/instrument" - _ "github.com/DataDog/orchestrion/instrument/event" _ "github.com/DataDog/orchestrion/instrument/net/http" _ "gopkg.in/DataDog/dd-trace-go.v1/appsec/events" _ "gopkg.in/DataDog/dd-trace-go.v1/contrib/99designs/gqlgen" diff --git a/internal/injector/builtin/testdata/server/chiv5.go.snap b/internal/injector/builtin/testdata/server/chiv5.go.snap index 6ea8f358..f613c9a5 100644 --- a/internal/injector/builtin/testdata/server/chiv5.go.snap +++ b/internal/injector/builtin/testdata/server/chiv5.go.snap @@ -28,12 +28,15 @@ func chiV5Server() { return mux }() //line samples/server/chiv5.go:16 - router.Get("/", + router.Get("/", func(w http.ResponseWriter, _ *http.Request) { + w.Write([]byte("Hello World!\n")) + }) //line - __orchestrion_instrument.WrapHandlerFunc( -//line samples/server/chiv5.go:16 - func(w http.ResponseWriter, _ *http.Request) { - w.Write([]byte("Hello World!\n")) - })) - http.ListenAndServe(":8080", router) +//line samples/server/chiv5.go:19 + http.ListenAndServe(":8080", +//line + __orchestrion_instrument.MaybeWrapHandler( +//line samples/server/chiv5.go:19 + router), + ) } diff --git a/internal/injector/builtin/testdata/server/gorilla.go.snap b/internal/injector/builtin/testdata/server/gorilla.go.snap index ee621949..376c682c 100644 --- a/internal/injector/builtin/testdata/server/gorilla.go.snap +++ b/internal/injector/builtin/testdata/server/gorilla.go.snap @@ -18,16 +18,20 @@ import ( //line samples/server/gorilla.go:15 func gorillaMuxServer() { r := mux.NewRouter() - ping := -//line - __orchestrion_instrument.WrapHandlerFunc( -//line samples/server/gorilla.go:17 - func(w http.ResponseWriter, r *http.Request) { - w.Header().Set("Content-Type", "application/json") - w.WriteHeader(http.StatusOK) + ping := func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) - _, _ = io.WriteString(w, `{"message":"pong"}`) - }) + _, _ = io.WriteString(w, `{"message":"pong"}`) + } r.HandleFunc("/ping", ping).Methods("GET") - _ = http.ListenAndServe(":8080", r) + _ = +//line +//line samples/server/gorilla.go:24 + http.ListenAndServe(":8080", +//line + __orchestrion_instrument.MaybeWrapHandler( +//line samples/server/gorilla.go:24 + r), + ) } diff --git a/internal/injector/builtin/testdata/server/main.go.snap b/internal/injector/builtin/testdata/server/main.go.snap index 2738abec..28b524c0 100644 --- a/internal/injector/builtin/testdata/server/main.go.snap +++ b/internal/injector/builtin/testdata/server/main.go.snap @@ -30,7 +30,7 @@ func main() { Handler: //dd:startwrap //line - __orchestrion_instrument.WrapHandler( + __orchestrion_instrument.MaybeWrapHandler( //line samples/server/main.go:17 http.HandlerFunc(myHandler)), //dd:endwrap diff --git a/internal/injector/builtin/testdata/server/other_handlers.go.snap b/internal/injector/builtin/testdata/server/other_handlers.go.snap index 3e1d5f17..a3b1f993 100644 --- a/internal/injector/builtin/testdata/server/other_handlers.go.snap +++ b/internal/injector/builtin/testdata/server/other_handlers.go.snap @@ -28,20 +28,12 @@ func (f foo) fooHandler(rw http.ResponseWriter, req *http.Request) { } func buildHandlers() { - http.HandleFunc("/foo/bar", -//line - __orchestrion_instrument.WrapHandlerFunc( -//line samples/server/other_handlers.go:26 - func(writer http.ResponseWriter, request *http.Request) { - writer.Write([]byte("done!")) - })) - v := -//line - __orchestrion_instrument.WrapHandlerFunc( -//line samples/server/other_handlers.go:29 - func(w http.ResponseWriter, r *http.Request) { - w.Write([]byte("another one!")) - }) + http.HandleFunc("/foo/bar", func(writer http.ResponseWriter, request *http.Request) { + writer.Write([]byte("done!")) + }) + v := func(w http.ResponseWriter, r *http.Request) { + w.Write([]byte("another one!")) + } fmt.Printf("%T\n", v) @@ -50,54 +42,39 @@ func buildHandlers() { } x := holder{ - f: -//line - __orchestrion_instrument.WrapHandlerFunc( -//line samples/server/other_handlers.go:40 - func(w http.ResponseWriter, request *http.Request) { - w.Write([]byte("asdf")) - }), + f: func(w http.ResponseWriter, request *http.Request) { + w.Write([]byte("asdf")) + }, } fmt.Println(x) // silly legal things -//line - __orchestrion_instrument.WrapHandlerFunc( -//line samples/server/other_handlers.go:48 - func(w http.ResponseWriter, r *http.Request) { - client := &http.Client{ - Timeout: time.Second, - } - req, err := http.NewRequestWithContext(context.Background(), http.MethodPost, "localhost:8080", strings.NewReader(os.Args[1])) - if err != nil { - panic(err) - } - resp, err := client.Do(req) - if err != nil { - panic(err) - } - fmt.Println(resp.Status) - w.Write([]byte("expression!")) - })(httptest.NewRecorder(), httptest.NewRequest(http.MethodPost, "/asfd", nil)) + func(w http.ResponseWriter, r *http.Request) { + client := &http.Client{ + Timeout: time.Second, + } + req, err := http.NewRequestWithContext(context.Background(), http.MethodPost, "localhost:8080", strings.NewReader(os.Args[1])) + if err != nil { + panic(err) + } + resp, err := client.Do(req) + if err != nil { + panic(err) + } + fmt.Println(resp.Status) + w.Write([]byte("expression!")) + }(httptest.NewRecorder(), httptest.NewRequest(http.MethodPost, "/asfd", nil)) for i := 0; i < 10; i++ { - go -//line - __orchestrion_instrument.WrapHandlerFunc( -//line samples/server/other_handlers.go:65 - func(w http.ResponseWriter, r *http.Request) { - w.Write([]byte("goroutine!")) - })(httptest.NewRecorder(), httptest.NewRequest(http.MethodPost, "/asfd", nil)) + go func(w http.ResponseWriter, r *http.Request) { + w.Write([]byte("goroutine!")) + }(httptest.NewRecorder(), httptest.NewRequest(http.MethodPost, "/asfd", nil)) } - defer -//line - __orchestrion_instrument.WrapHandlerFunc( -//line samples/server/other_handlers.go:70 - func(w http.ResponseWriter, r *http.Request) { - w.Write([]byte("goroutine!")) - })(httptest.NewRecorder(), httptest.NewRequest(http.MethodPost, "/asfd", nil)) + defer func(w http.ResponseWriter, r *http.Request) { + w.Write([]byte("goroutine!")) + }(httptest.NewRecorder(), httptest.NewRequest(http.MethodPost, "/asfd", nil)) } //dd:span foo:bar type:potato @@ -162,39 +139,23 @@ func registerHandlers() { handler := http.HandlerFunc(myHandler) http.Handle("/handle-1", handler) http.Handle("/hundle-2", http.HandlerFunc(myHandler)) - http.Handle("/hundle-3", http.HandlerFunc( -//line - __orchestrion_instrument.WrapHandlerFunc( -//line samples/server/other_handlers.go:95 - func(w http.ResponseWriter, r *http.Request) {}))) + http.Handle("/hundle-3", http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {})) http.HandleFunc("/handlefunc-1", handler) http.HandleFunc("/handlefunc-2", http.HandlerFunc(myHandler)) - http.HandleFunc("/handlefunc-3", -//line - __orchestrion_instrument.WrapHandlerFunc( -//line samples/server/other_handlers.go:98 - func(w http.ResponseWriter, r *http.Request) {})) + http.HandleFunc("/handlefunc-3", func(w http.ResponseWriter, r *http.Request) {}) s := http.NewServeMux() s.Handle("/handle-mux", handler) s.Handle("/handle-mux", http.HandlerFunc(myHandler)) - s.Handle("/handle-mux", http.HandlerFunc( -//line - __orchestrion_instrument.WrapHandlerFunc( -//line samples/server/other_handlers.go:102 - func(w http.ResponseWriter, r *http.Request) {}))) + s.Handle("/handle-mux", http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {})) s.HandleFunc("/handlefunc-1", handler) s.HandleFunc("/handlefunc-2", http.HandlerFunc(myHandler)) - s.HandleFunc("/handlefunc-3", -//line - __orchestrion_instrument.WrapHandlerFunc( -//line samples/server/other_handlers.go:105 - func(w http.ResponseWriter, r *http.Request) {})) + s.HandleFunc("/handlefunc-3", func(w http.ResponseWriter, r *http.Request) {}) _ = &http.Server{ Addr: ":8080", Handler: //dd:startwrap //line - __orchestrion_instrument.WrapHandler( + __orchestrion_instrument.MaybeWrapHandler( //line samples/server/other_handlers.go:108 handler), //dd:endwrap diff --git a/internal/injector/builtin/yaml/stdlib/net-http.server.yml b/internal/injector/builtin/yaml/stdlib/net-http.server.yml index 283622d1..59de432c 100644 --- a/internal/injector/builtin/yaml/stdlib/net-http.server.yml +++ b/internal/injector/builtin/yaml/stdlib/net-http.server.yml @@ -9,12 +9,9 @@ meta: description: HTTP server implementation. icon: at-symbol aspects: - # When httpmode is "wrap" - id: Wrap http.Server.Handler join-point: all-of: - - configuration: - httpmode: wrap - struct-literal: type: net/http.Server field: Handler @@ -30,74 +27,35 @@ aspects: instrument: github.com/DataDog/orchestrion/instrument template: |- //dd:startwrap - instrument.WrapHandler({{.}}) + instrument.MaybeWrapHandler({{.}}) //dd:endwrap - - id: Wrap http.HandlerFunc + - id: "Wrap starting a server - part 1 - TODO: this description sucks" join-point: - all-of: - - configuration: - httpmode: wrap - - function: - - name: '' # This filters only *dst.FuncLit - - signature: - args: [net/http.ResponseWriter, '*net/http.Request'] - # No instrumenting github.com/go-chi/chi/v5 as this causes a circular dependency. - - not: - one-of: - - import-path: github.com/go-chi/chi/v5 - - import-path: github.com/go-chi/chi/v5/middleware - # No instrumenting golang.org/x/net as it causes a circular dependency via GRPC - - import-path: golang.org/x/net/http2 + one-of: + - function-call: net/http.ListenAndServe + - function-call: net/http.Serve advice: - wrap-expression: imports: instrument: "github.com/DataDog/orchestrion/instrument" template: |- - instrument.WrapHandlerFunc({{.}}) - - # When httpmode is "report" - - id: Report http.HandlerFunc calls + {{.AST.Fun}}( + {{ index .AST.Args 0}}, + instrument.MaybeWrapHandler({{ index .AST.Args 1 }}), + ) + - id: "Wrap starting a server - part 2 - TODO: this description sucks" join-point: - all-of: - - configuration: - httpmode: report - - function-body: - function: - - signature: - args: [net/http.ResponseWriter, '*net/http.Request'] - # No instrumenting github.com/go-chi/chi/v5 as this causes a circular in wrap mode, and we - # don't want the behavior to be significantly different between wrap and report modes. - - not: - one-of: - - import-path: github.com/go-chi/chi/v5 - - import-path: github.com/go-chi/chi/v5/middleware - # No instrumenting golang.org/x/net as it causes a circular dependency via GRPC - - import-path: golang.org/x/net/http2 + one-of: + - function-call: net/http.ListenAndServeTLS + - function-call: net/http.ServeTLS advice: - - prepend-statements: + - wrap-expression: imports: - event: github.com/DataDog/orchestrion/instrument/event - instrument: github.com/DataDog/orchestrion/instrument + instrument: "github.com/DataDog/orchestrion/instrument" template: |- - {{- $arg := .Function.Argument 1 -}} - {{- $name := .Function.Name -}} - {{$arg}} = {{$arg}}.WithContext(instrument.Report( - {{$arg}}.Context(), - event.EventStart, - {{with $name}}"function-name", {{printf "%q" .}},{{end}} - "span.kind", "server", - "http.method", {{$arg}}.Method, - "http.url", {{$arg}}.URL, - "http.useragent", {{$arg}}.Header.Get("User-Agent"), - {{ range .DirectiveArgs "dd:span" -}}{{printf "%q, %q,\n" .Key .Value}}{{ end }} - )) - defer instrument.Report( - {{$arg}}.Context(), - event.EventEnd, - {{with $name}}"function-name", {{printf "%q" .}},{{end}} - "span.kind", "server", - "http.method", {{$arg}}.Method, - "http.url", {{$arg}}.URL, - "http.useragent", {{$arg}}.Header.Get("User-Agent"), - {{ range .DirectiveArgs "dd:span" -}}{{printf "%q, %q," .Key .Value}}{{- end }} + {{.AST.Fun}}( + {{ index .AST.Args 0}}, + {{ index .AST.Args 1}}, + {{ index .AST.Args 2}}, + instrument.MaybeWrapHandler({{ index .AST.Args 3 }}), )