From 657c57f938c7a92b9903bd4ccd384a5de87378e5 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Fri, 22 Nov 2024 10:11:49 +0000 Subject: [PATCH 1/2] chore(deps-dev): bump yarl from 1.17.1 to 1.18.0 in /_integration-tests/utils/agent (#412) Bumps [yarl](https://github.com/aio-libs/yarl) from 1.17.1 to 1.18.0.
Release notes

Sourced from yarl's releases.

1.18.0

Features

Contributor-facing changes

Miscellaneous internal changes

1.17.2

Bug fixes

Features

... (truncated)

Changelog

Sourced from yarl's changelog.

1.18.0

(2024-11-21)

Features

Contributor-facing changes

Miscellaneous internal changes

1.17.2

(2024-11-17)

Bug fixes

... (truncated)

Commits

[![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=yarl&package-manager=pip&previous-version=1.17.1&new-version=1.18.0)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) ---
Dependabot commands and options
You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot show ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)
Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- _integration-tests/utils/agent/requirements-dev.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/_integration-tests/utils/agent/requirements-dev.txt b/_integration-tests/utils/agent/requirements-dev.txt index 2e87438d..54d7fd94 100644 --- a/_integration-tests/utils/agent/requirements-dev.txt +++ b/_integration-tests/utils/agent/requirements-dev.txt @@ -18,4 +18,4 @@ requests==2.32.3 six==1.16.0 typing_extensions==4.12.2 urllib3==2.2.3 -yarl==1.17.1 +yarl==1.18.0 From a8237eeb1ce7cb868e47dc0b88ac6d9e88ff5ce7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rodrigo=20Arg=C3=BCello?= Date: Fri, 22 Nov 2024 11:34:44 +0100 Subject: [PATCH 2/2] fix: net/http server-side instrumentation (#403) Resolves https://github.com/DataDog/orchestrion/issues/173, resolves https://github.com/DataDog/orchestrion/issues/400 This PR changes `net/http` server-side instrumentation aspects to trace the library itself (similar approach as we are doing today with the client-side instrumentation). The hook where we inject is the `http.Server.Serve` method, which should be the common method that is always called when the http server is started (and at this point, the Handler should already be set). This fixes problems in the existing instrumentation where we: - Don't trace custom `http.Handler` implementations (https://github.com/DataDog/orchestrion/issues/173). - We double-trace `http.HandlerFunc(func(w http.ResponseWriter, r *http.Request))` declarations. - We might even break the build (as described in https://github.com/DataDog/orchestrion/issues/400). - Patterns using assignment like `srv.Handler = myHandler` are not traced. As a side-effect, this approach removes the ability to use `orchestrion:ignore` comments to skip tracing on http servers (which happens for other aspects as well where we instrument the library code). --------- Co-authored-by: Romain Marcadier Co-authored-by: Romain Marcadier --- _integration-tests/tests/chi.v5/chi.go | 30 ++++- _integration-tests/tests/echo.v4/echo.go | 30 ++++- _integration-tests/tests/fiber.v2/fiber.go | 13 +- _integration-tests/tests/gin/gin.go | 29 ++++- .../tests/{mux => gorilla_mux}/gen_test.go | 4 +- .../mux.go => gorilla_mux/gorilla_mux.go} | 25 +--- .../tests/net_http/{net_http.go => base.go} | 54 ++++---- .../tests/net_http/func_handler.go | 37 ++++++ _integration-tests/tests/net_http/gen_test.go | 16 ++- .../tests/net_http/handler_implementation.go | 46 +++++++ .../tests/net_http/issue_400.go | 39 ++++++ .../tests/net_http/serve_mux_handler.go | 21 ++++ _integration-tests/utils/suite.go | 4 +- instrument/http.go | 24 +++- internal/injector/builtin/generated.go | 83 ++---------- internal/injector/builtin/generated_deps.go | 1 - .../builtin/testdata/server/chiv5.go.snap | 11 +- .../builtin/testdata/server/gorilla.go.snap | 33 ----- .../builtin/testdata/server/main.go.snap | 12 +- .../testdata/server/other_handlers.go.snap | 118 ++++++------------ .../builtin/yaml/stdlib/net-http.server.yml | 103 +++------------ internal/injector/config/config_test.go | 2 +- .../config/testdata/instrument.snap.yml | 4 +- internal/toolexec/aspect/linkdeps/linkdeps.go | 75 ++++++++--- internal/toolexec/aspect/oncompile-main.go | 74 ++++++----- internal/toolexec/aspect/oncompile.go | 107 +++++++++------- internal/toolexec/aspect/onlink.go | 8 +- main_test.go | 114 ++++++++--------- samples/go.mod | 1 - samples/go.sum | 2 - samples/server/gorilla.go | 25 ---- 31 files changed, 600 insertions(+), 545 deletions(-) rename _integration-tests/tests/{mux => gorilla_mux}/gen_test.go (86%) rename _integration-tests/tests/{mux/mux.go => gorilla_mux/gorilla_mux.go} (80%) rename _integration-tests/tests/net_http/{net_http.go => base.go} (70%) create mode 100644 _integration-tests/tests/net_http/func_handler.go create mode 100644 _integration-tests/tests/net_http/handler_implementation.go create mode 100644 _integration-tests/tests/net_http/issue_400.go create mode 100644 _integration-tests/tests/net_http/serve_mux_handler.go delete mode 100644 internal/injector/builtin/testdata/server/gorilla.go.snap delete mode 100644 samples/server/gorilla.go diff --git a/_integration-tests/tests/chi.v5/chi.go b/_integration-tests/tests/chi.v5/chi.go index 3b931a0e..ab1ea728 100644 --- a/_integration-tests/tests/chi.v5/chi.go +++ b/_integration-tests/tests/chi.v5/chi.go @@ -28,7 +28,6 @@ type TestCase struct { func (tc *TestCase) Setup(t *testing.T) { router := chi.NewRouter() - //orchestrion:ignore tc.Server = &http.Server{ Addr: "127.0.0.1:" + utils.GetFreePort(t), Handler: router, @@ -55,25 +54,46 @@ func (tc *TestCase) Run(t *testing.T) { func (tc *TestCase) ExpectedTraces() trace.Traces { return trace.Traces{ { - // NB: Top-level span is from the HTTP Client, which is library-side instrumented. + // NB: 2 Top-level spans are from the HTTP Client/Server, which are library-side instrumented. Tags: map[string]any{ "name": "http.request", "resource": "GET /", + "service": "chi.v5.test", "type": "http", }, Meta: map[string]string{ - "http.url": fmt.Sprintf("http://%s/", tc.Server.Addr), + "http.url": fmt.Sprintf("http://%s/", tc.Server.Addr), + "component": "net/http", + "span.kind": "client", }, Children: trace.Traces{ { Tags: map[string]any{ "name": "http.request", "resource": "GET /", - "service": "chi.router", + "service": "chi.v5.test", "type": "web", }, Meta: map[string]string{ - "http.url": fmt.Sprintf("http://%s/", tc.Server.Addr), + "http.url": fmt.Sprintf("http://%s/", tc.Server.Addr), + "component": "net/http", + "span.kind": "server", + }, + Children: trace.Traces{ + { + Tags: map[string]any{ + "name": "http.request", + "resource": "GET /", + "service": "chi.router", + "type": "web", + }, + Meta: map[string]string{ + "http.url": fmt.Sprintf("http://%s/", tc.Server.Addr), + "component": "go-chi/chi.v5", + "span.kind": "server", + }, + Children: nil, + }, }, }, }, diff --git a/_integration-tests/tests/echo.v4/echo.go b/_integration-tests/tests/echo.v4/echo.go index 7266f99c..43210276 100644 --- a/_integration-tests/tests/echo.v4/echo.go +++ b/_integration-tests/tests/echo.v4/echo.go @@ -50,24 +50,48 @@ func (tc *TestCase) Run(t *testing.T) { } func (tc *TestCase) ExpectedTraces() trace.Traces { + httpUrl := "http://" + tc.addr + "/ping" return trace.Traces{ { - // NB: Top-level span is from the HTTP Client, which is library-side instrumented. + // NB: 2 Top-level spans are from the HTTP Client/Server, which are library-side instrumented. Tags: map[string]any{ "name": "http.request", "resource": "GET /ping", + "service": "echo.v4.test", "type": "http", }, + Meta: map[string]string{ + "http.url": httpUrl, + "component": "net/http", + "span.kind": "client", + }, Children: trace.Traces{ { Tags: map[string]any{ "name": "http.request", - "service": "echo", "resource": "GET /ping", + "service": "echo.v4.test", "type": "web", }, Meta: map[string]string{ - "http.url": "http://" + tc.addr + "/ping", + "http.url": httpUrl, + "component": "net/http", + "span.kind": "server", + }, + Children: trace.Traces{ + { + Tags: map[string]any{ + "name": "http.request", + "service": "echo", + "resource": "GET /ping", + "type": "web", + }, + Meta: map[string]string{ + "http.url": httpUrl, + "component": "labstack/echo.v4", + "span.kind": "server", + }, + }, }, }, }, diff --git a/_integration-tests/tests/fiber.v2/fiber.go b/_integration-tests/tests/fiber.v2/fiber.go index b70cb238..38b24fe1 100644 --- a/_integration-tests/tests/fiber.v2/fiber.go +++ b/_integration-tests/tests/fiber.v2/fiber.go @@ -42,27 +42,34 @@ func (tc *TestCase) Run(t *testing.T) { } func (tc *TestCase) ExpectedTraces() trace.Traces { + httpUrl := "http://" + tc.addr + "/ping" return trace.Traces{ { // NB: Top-level span is from the HTTP Client, which is library-side instrumented. + // The net/http server-side span does not appear here because fiber uses fasthttp internally instead of net/http. Tags: map[string]any{ "name": "http.request", "resource": "GET /ping", + "service": "fiber.v2.test", "type": "http", }, Meta: map[string]string{ - "http.url": "http://" + tc.addr + "/ping", + "http.url": httpUrl, + "component": "net/http", + "span.kind": "client", }, Children: trace.Traces{ { Tags: map[string]any{ "name": "http.request", - "service": "fiber", "resource": "GET /ping", + "service": "fiber", "type": "web", }, Meta: map[string]string{ - "http.url": "/ping", + "http.url": "/ping", // This is implemented incorrectly in the fiber.v2 dd-trace-go integration. + "component": "gofiber/fiber.v2", + "span.kind": "server", }, }, }, diff --git a/_integration-tests/tests/gin/gin.go b/_integration-tests/tests/gin/gin.go index 1f28b608..dd84f54d 100644 --- a/_integration-tests/tests/gin/gin.go +++ b/_integration-tests/tests/gin/gin.go @@ -50,26 +50,49 @@ func (tc *TestCase) Run(t *testing.T) { } func (tc *TestCase) ExpectedTraces() trace.Traces { + httpUrl := "http://" + tc.Server.Addr + "/ping" return trace.Traces{ { - // NB: Top-level span is from the HTTP Client, which is library-side instrumented. + // NB: 2 Top-level spans are from the HTTP Client/Server, which are library-side instrumented. Tags: map[string]any{ "name": "http.request", "resource": "GET /ping", + "service": "gin.test", "type": "http", }, Meta: map[string]string{ - "http.url": "http://" + tc.Server.Addr + "/ping", + "http.url": httpUrl, + "component": "net/http", + "span.kind": "client", }, Children: trace.Traces{ { Tags: map[string]any{ "name": "http.request", "resource": "GET /ping", + "service": "gin.test", "type": "web", }, Meta: map[string]string{ - "http.url": "http://" + tc.Server.Addr + "/ping", + "http.url": httpUrl, + "component": "net/http", + "span.kind": "server", + }, + + Children: trace.Traces{ + { + Tags: map[string]any{ + "name": "http.request", + "resource": "GET /ping", + "service": "gin.router", + "type": "web", + }, + Meta: map[string]string{ + "http.url": httpUrl, + "component": "gin-gonic/gin", + "span.kind": "server", + }, + }, }, }, }, diff --git a/_integration-tests/tests/mux/gen_test.go b/_integration-tests/tests/gorilla_mux/gen_test.go similarity index 86% rename from _integration-tests/tests/mux/gen_test.go rename to _integration-tests/tests/gorilla_mux/gen_test.go index 40e3b8ef..08b1f829 100644 --- a/_integration-tests/tests/mux/gen_test.go +++ b/_integration-tests/tests/gorilla_mux/gen_test.go @@ -7,13 +7,13 @@ //go:build integration -package mux +package gorilla_mux import ( "datadoghq.dev/orchestrion/_integration-tests/utils" "testing" ) -func TestIntegration_mux(t *testing.T) { +func TestIntegration_gorilla_mux(t *testing.T) { utils.RunTest(t, new(TestCase)) } diff --git a/_integration-tests/tests/mux/mux.go b/_integration-tests/tests/gorilla_mux/gorilla_mux.go similarity index 80% rename from _integration-tests/tests/mux/mux.go rename to _integration-tests/tests/gorilla_mux/gorilla_mux.go index 6e77fa72..9953c003 100644 --- a/_integration-tests/tests/mux/mux.go +++ b/_integration-tests/tests/gorilla_mux/gorilla_mux.go @@ -5,7 +5,7 @@ //go:build integration -package mux +package gorilla_mux import ( "context" @@ -57,12 +57,12 @@ func (tc *TestCase) ExpectedTraces() trace.Traces { url := fmt.Sprintf("http://%s/ping", tc.Server.Addr) return trace.Traces{ { - // NB: Top-level span is from the HTTP Client, which is library-side instrumented. + // NB: 2 Top-level spans are from the HTTP Client/Server, which are library-side instrumented. Tags: map[string]any{ "name": "http.request", "resource": "GET /ping", "type": "http", - "service": "mux.test", + "service": "gorilla_mux.test", }, Meta: map[string]string{ "http.url": url, @@ -75,7 +75,7 @@ func (tc *TestCase) ExpectedTraces() trace.Traces { "name": "http.request", "resource": "GET /ping", "type": "web", - "service": "mux.test", + "service": "gorilla_mux.test", }, Meta: map[string]string{ "http.url": url, @@ -95,22 +95,7 @@ func (tc *TestCase) ExpectedTraces() trace.Traces { "component": "gorilla/mux", "span.kind": "server", }, - Children: trace.Traces{ - { - // FIXME: this span shouldn't exist - Tags: map[string]any{ - "name": "http.request", - "resource": "GET /ping", - "type": "web", - "service": "mux.router", - }, - Meta: map[string]string{ - "http.url": url, - "component": "net/http", - "span.kind": "server", - }, - }, - }, + Children: nil, }, }, }, diff --git a/_integration-tests/tests/net_http/net_http.go b/_integration-tests/tests/net_http/base.go similarity index 70% rename from _integration-tests/tests/net_http/net_http.go rename to _integration-tests/tests/net_http/base.go index 69f689d0..8ee86101 100644 --- a/_integration-tests/tests/net_http/net_http.go +++ b/_integration-tests/tests/net_http/base.go @@ -21,35 +21,36 @@ import ( "github.com/stretchr/testify/require" ) -type TestCase struct { - *http.Server +type base struct { + srv *http.Server + handler http.Handler } -func (tc *TestCase) Setup(t *testing.T) { - mux := http.NewServeMux() - tc.Server = &http.Server{ - Addr: "127.0.0.1:" + utils.GetFreePort(t), - Handler: mux, - } +func (b *base) Setup(t *testing.T) { + require.NotNil(t, b.handler, "tc.handler needs to be initialized in your test case") - mux.HandleFunc("/hit", tc.handleHit) - mux.HandleFunc("/", tc.handleRoot) + b.srv = &http.Server{ + Addr: "127.0.0.1:" + utils.GetFreePort(t), + ReadTimeout: 5 * time.Second, + WriteTimeout: 10 * time.Second, + } + b.srv.Handler = b.handler - go func() { assert.ErrorIs(t, tc.Server.ListenAndServe(), http.ErrServerClosed) }() + go func() { assert.ErrorIs(t, b.srv.ListenAndServe(), http.ErrServerClosed) }() t.Cleanup(func() { ctx, cancel := context.WithTimeout(context.Background(), time.Second) defer cancel() - assert.NoError(t, tc.Server.Shutdown(ctx)) + assert.NoError(t, b.srv.Shutdown(ctx)) }) } -func (tc *TestCase) Run(t *testing.T) { - resp, err := http.Get(fmt.Sprintf("http://%s/", tc.Server.Addr)) +func (b *base) Run(t *testing.T) { + resp, err := http.Get(fmt.Sprintf("http://%s/", b.srv.Addr)) require.NoError(t, err) require.Equal(t, http.StatusOK, resp.StatusCode) } -func (tc *TestCase) ExpectedTraces() trace.Traces { +func (b *base) ExpectedTraces() trace.Traces { return trace.Traces{ { Tags: map[string]any{ @@ -80,7 +81,7 @@ func (tc *TestCase) ExpectedTraces() trace.Traces { "type": "http", }, Meta: map[string]string{ - "http.url": fmt.Sprintf("http://%s/hit", tc.Server.Addr), + "http.url": fmt.Sprintf("http://%s/hit", b.srv.Addr), "component": "net/http", "span.kind": "client", "network.destination.name": "127.0.0.1", @@ -97,9 +98,9 @@ func (tc *TestCase) ExpectedTraces() trace.Traces { Meta: map[string]string{ "http.useragent": "Go-http-client/1.1", "http.status_code": "200", - "http.host": tc.Server.Addr, + "http.host": b.srv.Addr, "component": "net/http", - "http.url": fmt.Sprintf("http://%s/hit", tc.Server.Addr), + "http.url": fmt.Sprintf("http://%s/hit", b.srv.Addr), "http.method": "POST", "span.kind": "server", }, @@ -113,10 +114,17 @@ func (tc *TestCase) ExpectedTraces() trace.Traces { } } -func (tc *TestCase) handleRoot(w http.ResponseWriter, r *http.Request) { +func (b *base) serveMuxHandler() http.Handler { + mux := http.NewServeMux() + mux.HandleFunc("/hit", b.handleHit) + mux.HandleFunc("/", b.handleRoot) + return mux +} + +func (b *base) handleRoot(w http.ResponseWriter, r *http.Request) { defer r.Body.Close() - resp, err := http.Post(fmt.Sprintf("http://%s/hit", tc.Server.Addr), "text/plain", r.Body) + resp, err := http.Post(fmt.Sprintf("http://%s/hit", b.srv.Addr), "text/plain", r.Body) if err != nil { w.WriteHeader(http.StatusBadRequest) _, _ = w.Write([]byte(err.Error())) @@ -124,7 +132,7 @@ func (tc *TestCase) handleRoot(w http.ResponseWriter, r *http.Request) { } defer resp.Body.Close() - b, err := io.ReadAll(resp.Body) + bytes, err := io.ReadAll(resp.Body) if err != nil { w.WriteHeader(http.StatusInternalServerError) _, _ = w.Write([]byte(err.Error())) @@ -132,10 +140,10 @@ func (tc *TestCase) handleRoot(w http.ResponseWriter, r *http.Request) { } w.WriteHeader(resp.StatusCode) - _, _ = w.Write(b) + _, _ = w.Write(bytes) } -func (*TestCase) handleHit(w http.ResponseWriter, r *http.Request) { +func (*base) handleHit(w http.ResponseWriter, r *http.Request) { defer r.Body.Close() b, err := io.ReadAll(r.Body) diff --git a/_integration-tests/tests/net_http/func_handler.go b/_integration-tests/tests/net_http/func_handler.go new file mode 100644 index 00000000..727c2385 --- /dev/null +++ b/_integration-tests/tests/net_http/func_handler.go @@ -0,0 +1,37 @@ +// Unless explicitly stated otherwise all files in this repository are licensed +// under the Apache License Version 2.0. +// This product includes software developed at Datadog (https://www.datadoghq.com/). +// Copyright 2023-present Datadog, Inc. + +//go:build integration + +package nethttp + +import ( + "net/http" + "testing" +) + +type TestCaseFuncHandler struct { + base +} + +func (tc *TestCaseFuncHandler) Setup(t *testing.T) { + tc.handler = http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/": + tc.handleRoot(w, r) + return + + case "/hit": + tc.handleHit(w, r) + return + + default: + w.WriteHeader(http.StatusNotFound) + return + } + }) + + tc.base.Setup(t) +} diff --git a/_integration-tests/tests/net_http/gen_test.go b/_integration-tests/tests/net_http/gen_test.go index d4a81476..005cca62 100644 --- a/_integration-tests/tests/net_http/gen_test.go +++ b/_integration-tests/tests/net_http/gen_test.go @@ -14,6 +14,18 @@ import ( "testing" ) -func TestIntegration_nethttp(t *testing.T) { - utils.RunTest(t, new(TestCase)) +func TestIntegration_nethttp_FuncHandler(t *testing.T) { + utils.RunTest(t, new(TestCaseFuncHandler)) +} + +func TestIntegration_nethttp_HandlerImplementation(t *testing.T) { + utils.RunTest(t, new(TestCaseHandlerImplementation)) +} + +func TestIntegration_nethttp_Issue400(t *testing.T) { + utils.RunTest(t, new(TestCaseIssue400)) +} + +func TestIntegration_nethttp_ServeMuxHandler(t *testing.T) { + utils.RunTest(t, new(TestCaseServeMuxHandler)) } diff --git a/_integration-tests/tests/net_http/handler_implementation.go b/_integration-tests/tests/net_http/handler_implementation.go new file mode 100644 index 00000000..c55e6afa --- /dev/null +++ b/_integration-tests/tests/net_http/handler_implementation.go @@ -0,0 +1,46 @@ +// Unless explicitly stated otherwise all files in this repository are licensed +// under the Apache License Version 2.0. +// This product includes software developed at Datadog (https://www.datadoghq.com/). +// Copyright 2023-present Datadog, Inc. + +//go:build integration + +package nethttp + +import ( + "net/http" + "testing" +) + +type customHandler struct { + handleRoot func(w http.ResponseWriter, r *http.Request) + handleHit func(w http.ResponseWriter, r *http.Request) +} + +func (c *customHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/": + c.handleRoot(w, r) + return + + case "/hit": + c.handleHit(w, r) + return + + default: + w.WriteHeader(http.StatusNotFound) + return + } +} + +type TestCaseHandlerImplementation struct { + base +} + +func (tc *TestCaseHandlerImplementation) Setup(t *testing.T) { + tc.handler = &customHandler{ + handleRoot: tc.handleRoot, + handleHit: tc.handleHit, + } + tc.base.Setup(t) +} diff --git a/_integration-tests/tests/net_http/issue_400.go b/_integration-tests/tests/net_http/issue_400.go new file mode 100644 index 00000000..db5eb836 --- /dev/null +++ b/_integration-tests/tests/net_http/issue_400.go @@ -0,0 +1,39 @@ +// Unless explicitly stated otherwise all files in this repository are licensed +// under the Apache License Version 2.0. +// This product includes software developed at Datadog (https://www.datadoghq.com/). +// Copyright 2023-present Datadog, Inc. + +//go:build integration + +package nethttp + +import ( + "net/http" + "testing" +) + +// TestCaseIssue400 tests regressions for https://github.com/DataDog/orchestrion/issues/400. +type TestCaseIssue400 struct { + base +} + +type handlerFunc func(http.ResponseWriter, *http.Request) + +// This issue happened because we used to replace with WrapHandlerFunc every time we saw a function that matched the +// signature func(w http.ResponseWriter, req *http.Request), and using a custom type like we do here broke the build. +func wrapCustomType(f func(http.ResponseWriter, *http.Request)) handlerFunc { + return func(w http.ResponseWriter, r *http.Request) { + f(w, r) + } +} + +func (tc *TestCaseIssue400) Setup(t *testing.T) { + handleHit := wrapCustomType(tc.handleHit) + handleRoot := wrapCustomType(tc.handleRoot) + mux := http.NewServeMux() + mux.HandleFunc("/hit", handleHit) + mux.HandleFunc("/", handleRoot) + tc.handler = mux + + tc.base.Setup(t) +} diff --git a/_integration-tests/tests/net_http/serve_mux_handler.go b/_integration-tests/tests/net_http/serve_mux_handler.go new file mode 100644 index 00000000..c22eb769 --- /dev/null +++ b/_integration-tests/tests/net_http/serve_mux_handler.go @@ -0,0 +1,21 @@ +// Unless explicitly stated otherwise all files in this repository are licensed +// under the Apache License Version 2.0. +// This product includes software developed at Datadog (https://www.datadoghq.com/). +// Copyright 2023-present Datadog, Inc. + +//go:build integration + +package nethttp + +import ( + "testing" +) + +type TestCaseServeMuxHandler struct { + base +} + +func (tc *TestCaseServeMuxHandler) Setup(t *testing.T) { + tc.handler = tc.serveMuxHandler() + tc.base.Setup(t) +} diff --git a/_integration-tests/utils/suite.go b/_integration-tests/utils/suite.go index e2b2bec7..0b12f139 100644 --- a/_integration-tests/utils/suite.go +++ b/_integration-tests/utils/suite.go @@ -48,14 +48,14 @@ func RunTest(t *testing.T, tc TestCase) { require.True(t, built.WithOrchestrion, "this test suite must be run with orchestrion enabled") mockAgent, err := agent.New(t) - require.NoError(t, err) + require.NoError(t, err, "failed to start mock agent") defer mockAgent.Close() t.Log("Running setup") tc.Setup(t) sess, err := mockAgent.NewSession(t) - require.NoError(t, err) + require.NoError(t, err, "failed to create a new mock agent session") t.Log("Running test") tc.Run(t) diff --git a/instrument/http.go b/instrument/http.go index 7f5e722c..cd16a656 100644 --- a/instrument/http.go +++ b/instrument/http.go @@ -21,8 +21,20 @@ func resourceNamer(r *http.Request) string { return fmt.Sprintf("%s %s", r.Method, r.URL.Path) } +type ddTraceHandler struct { + http.Handler +} + func WrapHandler(handler http.Handler) http.Handler { - return httptrace.WrapHandler(handler, "", "", httptrace.WithResourceNamer(resourceNamer)) + if isDDTraceHandler(handler) { + return handler + } + if mux, ok := handler.(*http.ServeMux); ok { + tracedMux := httptrace.NewServeMux() + tracedMux.ServeMux = mux + return tracedMux + } + return &ddTraceHandler{httptrace.WrapHandler(handler, "", "", httptrace.WithResourceNamer(resourceNamer))} // TODO: We'll reintroduce this later when we stop hard-coding dd-trace-go as above. // return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { // r = HandleHeader(r) @@ -50,3 +62,13 @@ func WrapHandlerFunc(handlerFunc http.HandlerFunc) http.HandlerFunc { // handlerFunc(w, r) // } } + +func isDDTraceHandler(h http.Handler) bool { + switch h.(type) { + case *httptrace.ServeMux, *ddTraceHandler: + return true + + default: + return false + } +} diff --git a/internal/injector/builtin/generated.go b/internal/injector/builtin/generated.go index 8caf9a26..efd27118 100644 --- a/internal/injector/builtin/generated.go +++ b/internal/injector/builtin/generated.go @@ -1664,79 +1664,21 @@ 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"), - join.ImportPath("github.com/go-chi/chi/v5/middleware"), - join.ImportPath("golang.org/x/net/http2"), - )), - ), - Advice: []advice.Advice{ - advice.WrapExpression(code.MustTemplate( - "//dd:startwrap\ninstrument.WrapHandler({{.}})\n//dd:endwrap", - map[string]string{ - "instrument": "github.com/DataDog/orchestrion/instrument", - }, - context.GoLangVersion{}, - )), - }, - }, - { - 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.FunctionBody(join.Function( + join.Receiver(join.MustTypeName("*net/http.Server")), + join.Name("Serve"), + )), Advice: []advice.Advice{ - advice.WrapExpression(code.MustTemplate( - "instrument.WrapHandlerFunc({{.}})", - map[string]string{ - "instrument": "github.com/DataDog/orchestrion/instrument", - }, + advice.InjectDeclarations(code.MustTemplate( + "//go:linkname __dd_orchestrion_instrument_WrapHandler github.com/DataDog/orchestrion/instrument.WrapHandler\nfunc __dd_orchestrion_instrument_WrapHandler(handler Handler) Handler", + map[string]string{}, context.GoLangVersion{}, - )), - }, - }, - { - JoinPoint: join.AllOf( - join.Configuration(map[string]string{ - "httpmode": "report", + ), []string{ + "github.com/DataDog/orchestrion/instrument", }), - 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"), - )), - ), - 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)", - map[string]string{ - "event": "github.com/DataDog/orchestrion/instrument/event", - "instrument": "github.com/DataDog/orchestrion/instrument", - }, + "{{- $srv := .Function.Receiver -}}\nif {{ $srv }}.Handler != nil {\n {{ $srv }}.Handler = __dd_orchestrion_instrument_WrapHandler({{ $srv }}.Handler)\n}", + map[string]string{}, context.GoLangVersion{}, )), }, @@ -1810,7 +1752,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", @@ -1874,4 +1815,4 @@ var InjectedPaths = [...]string{ } // Checksum is a checksum of the built-in configuration which can be used to invalidate caches. -const Checksum = "sha512:xmZzyYLOd/Rd6xiw6DaourRfLxpq0vKm9vnwvz8ENJhfN/wlYdnLA0ninNmp6UO8i8pbjrPV/l/mc2ZoZaYXpQ==" +const Checksum = "sha512:MJ4+HJMaOH/ACkbYHyW+erCE7e7moqEflMCs1vOIMiUQv4nkKGASjiZOukrnylyCtDrtAk/InP376zusJcmapw==" diff --git a/internal/injector/builtin/generated_deps.go b/internal/injector/builtin/generated_deps.go index 4172f28a..93fbda01 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..63282701 100644 --- a/internal/injector/builtin/testdata/server/chiv5.go.snap +++ b/internal/injector/builtin/testdata/server/chiv5.go.snap @@ -11,7 +11,6 @@ import ( "github.com/go-chi/chi/v5" //line - __orchestrion_instrument "github.com/DataDog/orchestrion/instrument" __orchestrion_chitrace "gopkg.in/DataDog/dd-trace-go.v1/contrib/go-chi/chi.v5" ) @@ -28,12 +27,8 @@ func chiV5Server() { return mux }() //line samples/server/chiv5.go:16 - router.Get("/", -//line - __orchestrion_instrument.WrapHandlerFunc( -//line samples/server/chiv5.go:16 - func(w http.ResponseWriter, _ *http.Request) { - w.Write([]byte("Hello World!\n")) - })) + router.Get("/", func(w http.ResponseWriter, _ *http.Request) { + w.Write([]byte("Hello World!\n")) + }) http.ListenAndServe(":8080", router) } diff --git a/internal/injector/builtin/testdata/server/gorilla.go.snap b/internal/injector/builtin/testdata/server/gorilla.go.snap deleted file mode 100644 index ee621949..00000000 --- a/internal/injector/builtin/testdata/server/gorilla.go.snap +++ /dev/null @@ -1,33 +0,0 @@ -//line samples/server/gorilla.go:1:1 -// Unless explicitly stated otherwise all files in this repository are licensed -// under the Apache License Version 2.0. -// This product includes software developed at Datadog (https://www.datadoghq.com/). -// Copyright 2023-present Datadog, Inc. - -package main - -import ( - "io" - "net/http" - - "github.com/gorilla/mux" -//line - __orchestrion_instrument "github.com/DataDog/orchestrion/instrument" -) - -//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) - - _, _ = io.WriteString(w, `{"message":"pong"}`) - }) - r.HandleFunc("/ping", ping).Methods("GET") - _ = http.ListenAndServe(":8080", r) -} diff --git a/internal/injector/builtin/testdata/server/main.go.snap b/internal/injector/builtin/testdata/server/main.go.snap index 3cca856e..cb37bad3 100644 --- a/internal/injector/builtin/testdata/server/main.go.snap +++ b/internal/injector/builtin/testdata/server/main.go.snap @@ -11,7 +11,6 @@ import ( "log" "net/http" //line - __orchestrion_instrument "github.com/DataDog/orchestrion/instrument" __orchestrion_tracer "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer" __orchestrion_profiler "gopkg.in/DataDog/dd-trace-go.v1/profiler" __orchestrion_os "os" @@ -33,17 +32,10 @@ func main() { } //line samples/server/main.go:15 s := &http.Server{ - Addr: ":8080", - Handler: - //dd:startwrap -//line - __orchestrion_instrument.WrapHandler( -//line samples/server/main.go:17 - http.HandlerFunc(myHandler)), - //dd:endwrap + Addr: ":8080", + Handler: http.HandlerFunc(myHandler), } -//line samples/server/main.go:20 log.Fatal(s.ListenAndServe()) } diff --git a/internal/injector/builtin/testdata/server/other_handlers.go.snap b/internal/injector/builtin/testdata/server/other_handlers.go.snap index 3e1d5f17..53ad5177 100644 --- a/internal/injector/builtin/testdata/server/other_handlers.go.snap +++ b/internal/injector/builtin/testdata/server/other_handlers.go.snap @@ -15,7 +15,6 @@ import ( "strings" "time" //line - __orchestrion_instrument "github.com/DataDog/orchestrion/instrument" __orchestrion_tracer "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer" ) @@ -28,20 +27,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 +41,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,41 +138,19 @@ 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( -//line samples/server/other_handlers.go:108 - handler), - //dd:endwrap + Addr: ":8080", + Handler: handler, } } diff --git a/internal/injector/builtin/yaml/stdlib/net-http.server.yml b/internal/injector/builtin/yaml/stdlib/net-http.server.yml index 283622d1..d2cc1e50 100644 --- a/internal/injector/builtin/yaml/stdlib/net-http.server.yml +++ b/internal/injector/builtin/yaml/stdlib/net-http.server.yml @@ -9,95 +9,24 @@ meta: description: HTTP server implementation. icon: at-symbol aspects: - # When httpmode is "wrap" - - id: Wrap http.Server.Handler + - id: Instrument http.Server#Serve join-point: - all-of: - - configuration: - httpmode: wrap - - struct-literal: - type: net/http.Server - field: Handler - # 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 - - import-path: golang.org/x/net/http2 + function-body: + function: + - receiver: '*net/http.Server' + - name: Serve advice: - - wrap-expression: - imports: - instrument: github.com/DataDog/orchestrion/instrument + - inject-declarations: + # We need to use go:linkname to refer to a number of declarations in order to avoid creating + # circular dependencies, as these features have transitive dependencies on `net/http`... + links: + - github.com/DataDog/orchestrion/instrument template: |- - //dd:startwrap - instrument.WrapHandler({{.}}) - //dd:endwrap - - id: Wrap http.HandlerFunc - 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 - advice: - - wrap-expression: - imports: - instrument: "github.com/DataDog/orchestrion/instrument" - template: |- - instrument.WrapHandlerFunc({{.}}) - - # When httpmode is "report" - - id: Report http.HandlerFunc calls - 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 - advice: + //go:linkname __dd_orchestrion_instrument_WrapHandler github.com/DataDog/orchestrion/instrument.WrapHandler + func __dd_orchestrion_instrument_WrapHandler(handler Handler) Handler - prepend-statements: - imports: - event: github.com/DataDog/orchestrion/instrument/event - 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 }} - ) + {{- $srv := .Function.Receiver -}} + if {{ $srv }}.Handler != nil { + {{ $srv }}.Handler = __dd_orchestrion_instrument_WrapHandler({{ $srv }}.Handler) + } diff --git a/internal/injector/config/config_test.go b/internal/injector/config/config_test.go index 24d2605a..308fc206 100644 --- a/internal/injector/config/config_test.go +++ b/internal/injector/config/config_test.go @@ -201,7 +201,7 @@ func TestLoad(t *testing.T) { enc.SetIndent(2) require.NoError(t, enc.Encode(cfg)) - assert.Len(t, cfg.Aspects(), 107) + assert.Len(t, cfg.Aspects(), 105) golden.Assert(t, buf.String(), "instrument.snap.yml") }) diff --git a/internal/injector/config/testdata/instrument.snap.yml b/internal/injector/config/testdata/instrument.snap.yml index ba96029a..15e1ade3 100644 --- a/internal/injector/config/testdata/instrument.snap.yml +++ b/internal/injector/config/testdata/instrument.snap.yml @@ -183,9 +183,7 @@ yaml: - Wire context through http.Get/Head/Post/PostForm - name: ./yaml/stdlib/net-http.server.yml aspects: - - Wrap http.Server.Handler - - Wrap http.HandlerFunc - - Report http.HandlerFunc calls + - Instrument http.Server#Serve - name: ./yaml/stdlib/ossec.yml aspects: - OpenFile diff --git a/internal/toolexec/aspect/linkdeps/linkdeps.go b/internal/toolexec/aspect/linkdeps/linkdeps.go index 656c50a4..6670fd5b 100644 --- a/internal/toolexec/aspect/linkdeps/linkdeps.go +++ b/internal/toolexec/aspect/linkdeps/linkdeps.go @@ -14,27 +14,60 @@ import ( "os/exec" "sort" "strings" + + "github.com/DataDog/orchestrion/internal/toolexec/importcfg" ) const ( - // LinkDepsFilename is the standard file name for link.deps files. - LinkDepsFilename = "link.deps" + // Filename is the standard file name for link.deps files. + Filename = "link.deps" - headerV1 = "#" + LinkDepsFilename + "@v1" + headerV1 = "#" + Filename + "@v1" ) -// LinkDeps represents the contents of a link.deps file. +// LinkDeps represents the contents of a [Filename] file. It lists all synthetic +// dependencies added by instrumentation into a Go object archive. These include +// the transitive closure of link-time dependencies, so that it is not necessary +// to perform a full traversal of transitive dependencies to consume. Link-time +// dependencies consist of new dependencies introduced to resolve go:linkname +// directives as well as new import-level directives that the Go toolchain is +// not normally aware of. type LinkDeps struct { deps map[string]struct{} } -// FromArchive reads a link.deps file from the provided Go archive file. Returns -// an empty LinkDeps if the archive does not contain a link.deps file. +// FromImportConfig aggregates entries from all [Filename] found in the +// archives listed in [importcfg.ImportConfig]. +func FromImportConfig(importcfg *importcfg.ImportConfig) (LinkDeps, error) { + var res LinkDeps + + for importPath, archivePath := range importcfg.PackageFile { + ld, err := FromArchive(archivePath) + if err != nil { + return LinkDeps{}, fmt.Errorf("reading %s from %s=%s: %w", Filename, importPath, archivePath, err) + } + + for _, dep := range ld.Dependencies() { + if _, satisfied := importcfg.PackageFile[dep]; satisfied { + // This transitive link-time dependency is already satisfied at + // compile-time, so we don't need to carry it over. + continue + } + res.Add(dep) + } + } + + return res, nil +} + +// FromArchive reads a [Filename] file from the provided Go archive file. +// Returns an empty [LinkDeps] if the archive does not contain a [Filename] +// file. func FromArchive(archive string) (res LinkDeps, err error) { var data io.Reader - data, err = readArchiveData(archive, LinkDepsFilename) + data, err = readArchiveData(archive, Filename) if err != nil { - return res, fmt.Errorf("reading %s from %q: %w", LinkDepsFilename, archive, err) + return res, fmt.Errorf("reading %s from %q: %w", Filename, archive, err) } if data == nil { return @@ -42,7 +75,7 @@ func FromArchive(archive string) (res LinkDeps, err error) { return Read(data) } -// ReadFile reads a link.deps file from the provided filename. +// ReadFile reads a [Filename] file from the provided filename. func ReadFile(filename string) (LinkDeps, error) { file, err := os.Open(filename) if err != nil { @@ -53,7 +86,7 @@ func ReadFile(filename string) (LinkDeps, error) { return Read(file) } -// Read reads a link.deps file content from the provided reader. +// Read reads a [Filename] file content from the provided [io.Reader]. func Read(r io.Reader) (l LinkDeps, err error) { rd := bufio.NewReader(r) @@ -71,7 +104,7 @@ func Read(r io.Reader) (l LinkDeps, err error) { } } -// parseV1 parses the contents of V1 link.deps files. +// parseV1 parses the contents of V1 [Filename] files. func parseV1(r *bufio.Reader) (l LinkDeps, err error) { for { var line string @@ -95,7 +128,14 @@ func parseV1(r *bufio.Reader) (l LinkDeps, err error) { } } -// Add registers a new import path in this LinkDeps instance. +// Contains checks whether a given import path is already represented by this +// [LinkDeps]. +func (l *LinkDeps) Contains(importPath string) bool { + _, found := l.deps[importPath] + return found +} + +// Add registers a new import path in this [LinkDeps] instance. func (l *LinkDeps) Add(importPath string) { if l.deps == nil { l.deps = make(map[string]struct{}) @@ -103,7 +143,7 @@ func (l *LinkDeps) Add(importPath string) { l.deps[importPath] = struct{}{} } -// Dependencies returns all import paths registered in this LinkDeps instance. +// Dependencies returns all import paths registered in this [LinkDeps] instance. func (l *LinkDeps) Dependencies() []string { deps := make([]string, 0, len(l.deps)) for importPath := range l.deps { @@ -112,17 +152,18 @@ func (l *LinkDeps) Dependencies() []string { return deps } -// Empty returns true if this LinkDeps instance is empty. +// Empty returns true if this [LinkDeps] instance is empty. func (l *LinkDeps) Empty() bool { return l.Len() == 0 } -// Len returns the number of import paths registered in this LinkDeps instance. +// Len returns the number of import paths registered in this [LinkDeps] +// instance. func (l *LinkDeps) Len() int { return len(l.deps) } -// WriteFile writes this LinkDeps instance to the provided filename. +// WriteFile writes this [LinkDeps] instance to the provided filename. func (l *LinkDeps) WriteFile(filename string) error { wr, err := os.Create(filename) if err != nil { @@ -133,7 +174,7 @@ func (l *LinkDeps) WriteFile(filename string) error { return l.Write(wr) } -// Write writes this LinkDeps instance to the provided writer. +// Write writes this [LinkDeps] instance to the provided writer. func (l *LinkDeps) Write(w io.Writer) error { if _, err := fmt.Fprintln(w, headerV1); err != nil { return err diff --git a/internal/toolexec/aspect/oncompile-main.go b/internal/toolexec/aspect/oncompile-main.go index 07b1360e..0455d944 100644 --- a/internal/toolexec/aspect/oncompile-main.go +++ b/internal/toolexec/aspect/oncompile-main.go @@ -8,14 +8,18 @@ package aspect import ( "errors" "fmt" + "go/ast" + "go/format" + "go/token" "os" "path/filepath" + "slices" + "strconv" "github.com/DataDog/orchestrion/internal/log" "github.com/DataDog/orchestrion/internal/toolexec/aspect/linkdeps" "github.com/DataDog/orchestrion/internal/toolexec/importcfg" "github.com/DataDog/orchestrion/internal/toolexec/proxy" - "github.com/dave/jennifer/jen" ) // OnCompileMain only performs changes when compiling the "main" package, adding blank imports for @@ -37,39 +41,33 @@ func (Weaver) OnCompileMain(cmd *proxy.CompileCommand) error { return fmt.Errorf("parsing %q: %w", cmd.Flags.ImportCfg, err) } - var addImports []string - for importPath, archive := range reg.PackageFile { - linkDeps, err := linkdeps.FromArchive(archive) + linkDeps, err := linkdeps.FromImportConfig(®) + if err != nil { + return fmt.Errorf("reading %s closure from %s: %w", linkdeps.Filename, cmd.Flags.ImportCfg, err) + } + + if linkDeps.Empty() { + // Nothing was added, we're done! + return nil + } + + newDeps := linkDeps.Dependencies() + + // Add package resolutions of link-time dependencies to the importcfg file: + for _, linkDepPath := range newDeps { + deps, err := resolvePackageFiles(linkDepPath, cmd.WorkDir) if err != nil { - return fmt.Errorf("reading %s from %q: %w", linkdeps.LinkDepsFilename, importPath, err) + return fmt.Errorf("resolving %q: %w", linkDepPath, err) } - - log.Debugf("Processing %s dependencies from %s[%s]...\n", linkdeps.LinkDepsFilename, importPath, archive) - for _, depPath := range linkDeps.Dependencies() { - if arch, found := reg.PackageFile[depPath]; found { - log.Debugf("Already satisfied %s dependency: %q => %q\n", linkdeps.LinkDepsFilename, depPath, arch) + for p, a := range deps { + if _, found := reg.PackageFile[p]; found { continue } - - deps, err := resolvePackageFiles(depPath, cmd.WorkDir) - if err != nil { - return fmt.Errorf("resolving %q: %w", depPath, err) - } - for p, a := range deps { - if _, found := reg.PackageFile[p]; !found { - log.Debugf("Recording resolved %s dependency: %q => %q\n", linkdeps.LinkDepsFilename, p, a) - reg.PackageFile[p] = a - } - } - addImports = append(addImports, depPath) + log.Debugf("Recording resolved %s dependency: %q => %q\n", linkdeps.Filename, p, a) + reg.PackageFile[p] = a } } - if len(addImports) == 0 { - // Nothing was added, we're done! - return nil - } - // We back up the original ImportCfg file only if there's not already such a file (could have been created by OnCompile) backupFile := cmd.Flags.ImportCfg + ".original" if _, err := os.Stat(backupFile); errors.Is(err, os.ErrNotExist) { @@ -83,9 +81,15 @@ func (Weaver) OnCompileMain(cmd *proxy.CompileCommand) error { return fmt.Errorf("writing updated %q: %w", cmd.Flags.ImportCfg, err) } - source := jen.NewFile("main") - for _, imp := range addImports { - source.Anon(imp) + // Generate a synthetic source file with blank imports to link-time + // dependencies, so the linker actually sees them. + genDecl := &ast.GenDecl{Tok: token.IMPORT, Specs: make([]ast.Spec, len(newDeps))} + fileAST := &ast.File{Name: ast.NewIdent("main"), Decls: []ast.Decl{genDecl}, Imports: make([]*ast.ImportSpec, len(newDeps))} + slices.Sort(newDeps) // Consistent order for deterministic output + for idx, path := range newDeps { + spec := &ast.ImportSpec{Name: ast.NewIdent("_"), Path: &ast.BasicLit{Kind: token.STRING, Value: strconv.Quote(path)}} + genDecl.Specs[idx] = spec + fileAST.Imports[idx] = spec } genDir := filepath.Join(filepath.Dir(cmd.Flags.Output), "orchestrion", "src", "synthetic") @@ -94,8 +98,14 @@ func (Weaver) OnCompileMain(cmd *proxy.CompileCommand) error { if err := os.MkdirAll(genDir, 0o755); err != nil { return fmt.Errorf("creating directory %q: %w", genDir, err) } - if err := source.Save(genFile); err != nil { - return fmt.Errorf("writing generated code to %q: %w", genFile, err) + + file, err := os.Create(genFile) + if err != nil { + return fmt.Errorf("create %q: %w", genFile, err) + } + defer file.Close() + if err := format.Node(file, token.NewFileSet(), fileAST); err != nil { + return fmt.Errorf("formatting generated code for %s: %w", genFile, err) } log.Debugf("Adding synthetic source file to command: %q\n", genFile) diff --git a/internal/toolexec/aspect/oncompile.go b/internal/toolexec/aspect/oncompile.go index e7661318..7439ab6a 100644 --- a/internal/toolexec/aspect/oncompile.go +++ b/internal/toolexec/aspect/oncompile.go @@ -62,10 +62,33 @@ var weavingSpecialCase = []specialCase{ {path: "github.com/DataDog/go-tuf/client", prefix: false, behavior: neverWeave}, } -func (w Weaver) OnCompile(cmd *proxy.CompileCommand) error { +func (w Weaver) OnCompile(cmd *proxy.CompileCommand) (err error) { log.SetContext("PHASE", "compile") defer log.SetContext("PHASE", "") + imports, err := importcfg.ParseFile(cmd.Flags.ImportCfg) + if err != nil { + return fmt.Errorf("parsing %q: %w", cmd.Flags.ImportCfg, err) + } + + linkDeps, err := linkdeps.FromImportConfig(&imports) + if err != nil { + return fmt.Errorf("reading %s closure from %s: %w", linkdeps.Filename, cmd.Flags.ImportCfg, err) + } + + orchestrionDir := filepath.Join(filepath.Dir(cmd.Flags.Output), "orchestrion") + + // Ensure we correctly register the [linkdeps.Filename] into the output + // archive upon returning, even if we made no changes. The contract is that + // an archive's [linkdeps.Filename] must represent all transitive link-time + // dependencies. + defer func() { + if err != nil { + return + } + err = writeLinkDeps(cmd, &linkDeps, orchestrionDir) + }() + aspects := builtin.Aspects[:] for _, sc := range weavingSpecialCase { if !sc.matches(w.ImportPath) { @@ -99,12 +122,6 @@ func (w Weaver) OnCompile(cmd *proxy.CompileCommand) error { break } - imports, err := importcfg.ParseFile(cmd.Flags.ImportCfg) - if err != nil { - return fmt.Errorf("parsing %q: %w", cmd.Flags.ImportCfg, err) - } - - orchestrionDir := filepath.Join(filepath.Dir(cmd.Flags.Output), "orchestrion") injector := injector.Injector{ Aspects: aspects, RootConfig: map[string]string{"httpmode": "wrap"}, @@ -140,29 +157,14 @@ func (w Weaver) OnCompile(cmd *proxy.CompileCommand) error { return nil } - var ( - linkDeps linkdeps.LinkDeps - regUpdated bool - ) + var regUpdated bool for depImportPath, kind := range references.Map() { if depImportPath == "unsafe" { // Unsafe isn't like other go packages, and it does not have an associated archive file. continue } - if archive, ok := imports.PackageFile[depImportPath]; ok { - deps, err := linkdeps.FromArchive(archive) - if err != nil { - return fmt.Errorf("reading %s from %q: %w", linkdeps.LinkDepsFilename, depImportPath, err) - } - log.Debugf("Processing %s dependencies from %s[%s]...", linkdeps.LinkDepsFilename, depImportPath, archive) - for _, tDep := range deps.Dependencies() { - if _, found := imports.PackageFile[tDep]; !found { - log.Debugf("Copying %s dependency on %q inherited from %q\n", linkdeps.LinkDepsFilename, tDep, depImportPath) - linkDeps.Add(tDep) - } - } - + if _, satisfied := imports.PackageFile[depImportPath]; satisfied { // Already part of natural dependencies, nothing to do... continue } @@ -179,12 +181,12 @@ func (w Weaver) OnCompile(cmd *proxy.CompileCommand) error { for dep, archive := range deps { deps, err := linkdeps.FromArchive(archive) if err != nil { - return fmt.Errorf("reading %s from %s[%s]: %w", linkdeps.LinkDepsFilename, dep, archive, err) + return fmt.Errorf("reading %s from %s[%s]: %w", linkdeps.Filename, dep, archive, err) } - log.Debugf("Processing %s dependencies from %s...\n", linkdeps.LinkDepsFilename, dep) + log.Debugf("Processing %s dependencies from %s...\n", linkdeps.Filename, dep) for _, tDep := range deps.Dependencies() { if _, found := imports.PackageFile[tDep]; !found { - log.Debugf("Copying transitive %s dependency on %q inherited from %q via %q\n", linkdeps.LinkDepsFilename, tDep, depImportPath, dep) + log.Debugf("Copying transitive %s dependency on %q inherited from %q via %q\n", linkdeps.Filename, tDep, depImportPath, dep) linkDeps.Add(tDep) } } @@ -200,12 +202,6 @@ func (w Weaver) OnCompile(cmd *proxy.CompileCommand) error { } } - if linkDeps.Empty() { - // There are no synthetic dependencies, so we don't need to write an updated importcfg or add - // extra objects in the output file. - return nil - } - if regUpdated { // Creating updated version of the importcfg file, with new dependencies if err := writeUpdatedImportConfig(imports, cmd.Flags.ImportCfg); err != nil { @@ -213,20 +209,6 @@ func (w Weaver) OnCompile(cmd *proxy.CompileCommand) error { } } - // Write the link.deps file and add it to the output object once the compilation has completed. - linkDepsFile := filepath.Join(orchestrionDir, linkdeps.LinkDepsFilename) - if err := linkDeps.WriteFile(linkDepsFile); err != nil { - return fmt.Errorf("writing %s file: %w", linkdeps.LinkDepsFilename, err) - } - cmd.OnClose(func() error { - log.Debugf("Adding %s file into %q\n", linkdeps.LinkDepsFilename, cmd.Flags.Output) - child := exec.Command("go", "tool", "pack", "r", cmd.Flags.Output, linkDepsFile) - if err := child.Run(); err != nil { - return fmt.Errorf("running %q: %w", child.Args, err) - } - return nil - }) - return nil } @@ -245,3 +227,34 @@ func writeUpdatedImportConfig(reg importcfg.ImportConfig, filename string) (err return nil } + +// writeLinkDeps writes the [linkdeps.Filename] file into the orchestrionDir, +// and registers it to be packed into the output archive. Does nothing if the +// provided [linkdeps.LinkDeps] is empty. +func writeLinkDeps(cmd *proxy.CompileCommand, linkDeps *linkdeps.LinkDeps, orchestrionDir string) error { + if linkDeps.Empty() { + // Nothing to do... + return nil + } + + // Write the link.deps file and add it to the output object once the compilation has completed. + if err := os.MkdirAll(orchestrionDir, 0o755); err != nil { + return fmt.Errorf("making directory %s: %w", orchestrionDir, err) + } + + linkDepsFile := filepath.Join(orchestrionDir, linkdeps.Filename) + if err := linkDeps.WriteFile(linkDepsFile); err != nil { + return fmt.Errorf("writing %s file: %w", linkdeps.Filename, err) + } + + cmd.OnClose(func() error { + log.Debugf("Adding %s file into %q\n", linkdeps.Filename, cmd.Flags.Output) + child := exec.Command("go", "tool", "pack", "r", cmd.Flags.Output, linkDepsFile) + if err := child.Run(); err != nil { + return fmt.Errorf("running %q: %w", child.Args, err) + } + return nil + }) + + return nil +} diff --git a/internal/toolexec/aspect/onlink.go b/internal/toolexec/aspect/onlink.go index 279a8c42..412fbb97 100644 --- a/internal/toolexec/aspect/onlink.go +++ b/internal/toolexec/aspect/onlink.go @@ -28,13 +28,13 @@ func (Weaver) OnLink(cmd *proxy.LinkCommand) error { for archiveImportPath, archive := range reg.PackageFile { linkDeps, err := linkdeps.FromArchive(archive) if err != nil { - return fmt.Errorf("reading %s from %q: %w", linkdeps.LinkDepsFilename, archiveImportPath, err) + return fmt.Errorf("reading %s from %q: %w", linkdeps.Filename, archiveImportPath, err) } - log.Debugf("Processing %s dependencies from %s[%s]...\n", linkdeps.LinkDepsFilename, archiveImportPath, archive) + log.Debugf("Processing %s dependencies from %s[%s]...\n", linkdeps.Filename, archiveImportPath, archive) for _, depPath := range linkDeps.Dependencies() { if arch, found := reg.PackageFile[depPath]; found { - log.Debugf("Already satisfied %s dependency: %q => %q\n", linkdeps.LinkDepsFilename, depPath, arch) + log.Debugf("Already satisfied %s dependency: %q => %q\n", linkdeps.Filename, depPath, arch) continue } @@ -44,7 +44,7 @@ func (Weaver) OnLink(cmd *proxy.LinkCommand) error { } for p, a := range deps { if _, found := reg.PackageFile[p]; !found { - log.Debugf("Recording resolved %s dependency: %q => %q\n", linkdeps.LinkDepsFilename, p, a) + log.Debugf("Recording resolved %s dependency: %q => %q\n", linkdeps.Filename, p, a) reg.PackageFile[p] = a changed = true } diff --git a/main_test.go b/main_test.go index 406f066a..6563954f 100644 --- a/main_test.go +++ b/main_test.go @@ -19,14 +19,13 @@ import ( "sync" "testing" - "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "golang.org/x/mod/semver" ) type testCase interface { - baseline() - instrumented() + baseline(b *testing.B) + instrumented(b *testing.B) } var testCases = map[string]func(b *testing.B) testCase{ @@ -43,19 +42,21 @@ var testCases = map[string]func(b *testing.B) testCase{ func Benchmark(b *testing.B) { for name, create := range testCases { - tc := create(b) - b.Run(fmt.Sprintf("repo=%s/variant=baseline", name), func(b *testing.B) { - b.ResetTimer() - for i := 0; i < b.N; i++ { - tc.baseline() - } - }) - - b.Run(fmt.Sprintf("repo=%s/variant=instrumented", name), func(b *testing.B) { - b.ResetTimer() - for i := 0; i < b.N; i++ { - tc.instrumented() - } + b.Run(fmt.Sprintf("repo=%s", name), func(b *testing.B) { + tc := create(b) + b.Run("variant=baseline", func(b *testing.B) { + b.ResetTimer() + for i := 0; i < b.N; i++ { + tc.baseline(b) + } + }) + + b.Run("variant=instrumented", func(b *testing.B) { + b.ResetTimer() + for i := 0; i < b.N; i++ { + tc.instrumented(b) + } + }) }) } } @@ -68,17 +69,17 @@ func benchmarkGithub(owner string, repo string, build string, testbuild bool) fu return func(b *testing.B) testCase { b.Helper() - tc := &benchGithub{harness{B: b, build: build, testbuild: testbuild}} + tc := &benchGithub{harness{build: build, testbuild: testbuild}} - tag := tc.findLatestGithubReleaseTag(owner, repo) - tc.gitCloneGithub(owner, repo, tag) - tc.exec("go", "mod", "download") - tc.exec("go", "mod", "edit", "-replace", fmt.Sprintf("github.com/DataDog/orchestrion=%s", rootDir)) + tag := tc.findLatestGithubReleaseTag(b, owner, repo) + tc.gitCloneGithub(b, owner, repo, tag) + tc.exec(b, "go", "mod", "download") + tc.exec(b, "go", "mod", "edit", "-replace", fmt.Sprintf("github.com/DataDog/orchestrion=%s", rootDir)) if stat, err := os.Stat(filepath.Join(tc.dir, "vendor")); err == nil && stat.IsDir() { // If there's a vendor dir, we need to update the `modules.txt` in there to reflect the replacement. - tc.exec("go", "mod", "vendor") + tc.exec(b, "go", "mod", "vendor") } - tc.exec(buildOrchestrion(b), "pin") + tc.exec(b, buildOrchestrion(b), "pin") return tc } @@ -88,79 +89,78 @@ type benchOrchestrion struct { harness } -func benchmarkOrchestrion(b *testing.B) testCase { - return &benchOrchestrion{harness{B: b, dir: rootDir, build: ".", testbuild: false}} +func benchmarkOrchestrion(_ *testing.B) testCase { + return &benchOrchestrion{harness{dir: rootDir, build: ".", testbuild: false}} } type harness struct { - *testing.B dir string // The directory in which the source code of the package to be built is located. build string // The package to be built as part of the test. testbuild bool // Whether the package to be built is a test package. } -func (h *harness) baseline() { - h.Helper() +func (h *harness) baseline(b *testing.B) { + b.Helper() var cmd *exec.Cmd if h.testbuild { - cmd = exec.Command("go", "test", "-c", "-o", h.TempDir(), h.build) + cmd = exec.Command("go", "test", "-c", "-o", b.TempDir(), h.build) } else { - cmd = exec.Command("go", "build", "-o", h.TempDir(), h.build) + cmd = exec.Command("go", "build", "-o", b.TempDir(), h.build) } cmd.Dir = h.dir - cmd.Env = append(os.Environ(), "GOCACHE="+h.TempDir()) + cmd.Env = append(os.Environ(), "GOCACHE="+b.TempDir()) output := bytes.NewBuffer(make([]byte, 0, 4_096)) cmd.Stdout = output cmd.Stderr = output - h.StartTimer() + b.StartTimer() err := cmd.Run() - h.StopTimer() + b.StopTimer() - assert.NoError(h, err, "build failed:\n%s", output) + require.NoError(b, err, "build failed:\n%s", output) } -func (h *harness) instrumented() { - h.Helper() +func (h *harness) instrumented(b *testing.B) { + b.Helper() var cmd *exec.Cmd if h.testbuild { - cmd = exec.Command(buildOrchestrion(h.B), "go", "test", "-c", "-o", h.TempDir(), h.build) + cmd = exec.Command(buildOrchestrion(b), "go", "test", "-c", "-o", b.TempDir(), h.build) } else { - cmd = exec.Command(buildOrchestrion(h.B), "go", "build", "-o", h.TempDir(), h.build) + cmd = exec.Command(buildOrchestrion(b), "go", "build", "-o", b.TempDir(), h.build) } cmd.Dir = h.dir - cmd.Env = append(os.Environ(), "GOCACHE="+h.TempDir()) + cmd.Env = append(os.Environ(), "GOCACHE="+b.TempDir()) output := bytes.NewBuffer(make([]byte, 0, 4_096)) cmd.Stdout = output cmd.Stderr = output - h.StartTimer() + b.StartTimer() err := cmd.Run() - h.StopTimer() + b.StopTimer() - assert.NoError(h, err, "build failed:\n%s", output) + require.NoError(b, err, "build failed:\n%s", output) } -func (h *harness) exec(name string, args ...string) { +func (h *harness) exec(b *testing.B, name string, args ...string) { cmd := exec.Command(name, args...) cmd.Dir = h.dir - cmd.Env = append(os.Environ(), "GOCACHE="+h.TempDir()) + cmd.Env = append(os.Environ(), "GOCACHE="+b.TempDir()) output := bytes.NewBuffer(make([]byte, 0, 4_096)) cmd.Stdout = output cmd.Stderr = output - require.NoError(h, cmd.Run(), "command failed: %s\n%s", cmd, output) + require.NoError(b, cmd.Run(), "command failed: %s\n%s", cmd, output) } -func (h *harness) findLatestGithubReleaseTag(owner string, repo string) string { - h.Helper() +func (*harness) findLatestGithubReleaseTag(b *testing.B, owner string, repo string) string { + b.Helper() // NB -- Default page size is 30, and releases are sorted by creation date... We should be able to rely on the tag // we are looking for being present in the first page, ergo we don't bother traversing all pages. req, err := http.NewRequest(http.MethodGet, fmt.Sprintf("https://api.github.com/repos/%s/%s/releases", owner, repo), nil) - require.NoError(h, err) + require.NoError(b, err) req.Header.Set("X-GitHub-Api-Version", "2022-11-28") if token, ok := getGithubToken(); ok { @@ -168,17 +168,17 @@ func (h *harness) findLatestGithubReleaseTag(owner string, repo string) string { } resp, err := http.DefaultClient.Do(req) - require.NoError(h, err) + require.NoError(b, err) defer resp.Body.Close() - require.Equal(h, http.StatusOK, resp.StatusCode, "error response body:\n%s", contentString{resp.Body}) + require.Equal(b, http.StatusOK, resp.StatusCode, "error response body:\n%s", contentString{resp.Body}) var payload []struct { Prerelease bool `json:"prerelease"` TagName string `json:"tag_name"` } - require.NoError(h, json.NewDecoder(resp.Body).Decode(&payload)) - require.NotEmpty(h, payload) + require.NoError(b, json.NewDecoder(resp.Body).Decode(&payload)) + require.NotEmpty(b, payload) var tagName string for _, release := range payload { @@ -191,7 +191,7 @@ func (h *harness) findLatestGithubReleaseTag(owner string, repo string) string { } } - require.NotEmpty(h, tagName) + require.NotEmpty(b, tagName) return tagName } @@ -213,11 +213,11 @@ func getGithubToken() (string, bool) { return strings.TrimSpace(bytes.String()), true } -func (h *harness) gitCloneGithub(owner string, repo string, tag string) string { - h.Helper() +func (h *harness) gitCloneGithub(b *testing.B, owner string, repo string, tag string) string { + b.Helper() - h.dir = h.TempDir() - h.exec("git", "clone", "--depth=1", fmt.Sprintf("--branch=%s", tag), fmt.Sprintf("https://github.com/%s/%s.git", owner, repo), h.dir) + h.dir = b.TempDir() + h.exec(b, "git", "clone", "--depth=1", fmt.Sprintf("--branch=%s", tag), fmt.Sprintf("https://github.com/%s/%s.git", owner, repo), h.dir) return h.dir } diff --git a/samples/go.mod b/samples/go.mod index 44d81723..bea25046 100644 --- a/samples/go.mod +++ b/samples/go.mod @@ -23,7 +23,6 @@ require ( github.com/gocql/gocql v1.7.0 github.com/gofiber/fiber/v2 v2.52.5 github.com/gomodule/redigo v1.9.2 - github.com/gorilla/mux v1.8.1 github.com/hashicorp/vault/api v1.15.0 github.com/jackc/pgx/v5 v5.7.1 github.com/jinzhu/gorm v1.9.16 diff --git a/samples/go.sum b/samples/go.sum index 98f5d0d2..4ec7fb61 100644 --- a/samples/go.sum +++ b/samples/go.sum @@ -569,8 +569,6 @@ github.com/googleapis/gax-go/v2 v2.0.5/go.mod h1:DWXyrwAJ9X0FpwwEdw+IPEYBICEFu5m github.com/googleapis/gax-go/v2 v2.14.0 h1:f+jMrjBPl+DL9nI4IQzLUxMq7XrAqFYB7hBPqMNIe8o= github.com/googleapis/gax-go/v2 v2.14.0/go.mod h1:lhBCnjdLrWRaPvLWhmc8IS24m9mr07qSYnHncrgo+zk= github.com/googleapis/google-cloud-go-testing v0.0.0-20200911160855-bcd43fbb19e8/go.mod h1:dvDLG8qkwmyD9a/MJJN3XJcT3xFxOKAvTZGvuZmac9g= -github.com/gorilla/mux v1.8.1 h1:TuBL49tXwgrFYWhqrNgrUNEY92u81SPhu7sTdzQEiWY= -github.com/gorilla/mux v1.8.1/go.mod h1:AKf9I4AEqPTmMytcMc0KkNouC66V3BtZ4qD5fmWSiMQ= github.com/gorilla/securecookie v1.1.1/go.mod h1:ra0sb63/xPlUeL+yeDciTfxMRAA+MP+HVt/4epWDjd4= github.com/gorilla/sessions v1.2.1/go.mod h1:dk2InVEVJ0sfLlnXv9EAgkf6ecYs/i80K/zI+bUmuGM= github.com/gorilla/websocket v1.5.3 h1:saDtZ6Pbx/0u+bgYQ3q96pZgCzfhKXGPqt7kZ72aNNg= diff --git a/samples/server/gorilla.go b/samples/server/gorilla.go deleted file mode 100644 index 4990ca95..00000000 --- a/samples/server/gorilla.go +++ /dev/null @@ -1,25 +0,0 @@ -// Unless explicitly stated otherwise all files in this repository are licensed -// under the Apache License Version 2.0. -// This product includes software developed at Datadog (https://www.datadoghq.com/). -// Copyright 2023-present Datadog, Inc. - -package main - -import ( - "io" - "net/http" - - "github.com/gorilla/mux" -) - -func gorillaMuxServer() { - r := mux.NewRouter() - ping := func(w http.ResponseWriter, r *http.Request) { - w.Header().Set("Content-Type", "application/json") - w.WriteHeader(http.StatusOK) - - _, _ = io.WriteString(w, `{"message":"pong"}`) - } - r.HandleFunc("/ping", ping).Methods("GET") - _ = http.ListenAndServe(":8080", r) -}