Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: net/http server-side instrumentation #403

Merged
merged 18 commits into from
Nov 22, 2024
Merged

fix: net/http server-side instrumentation #403

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
cc548ac
fix: net/http server-side instrumentation
rarguelloF Nov 19, 2024
168f794
add tests
rarguelloF Nov 19, 2024
b639ad6
chore: update generated files
RomainMuller Nov 19, 2024
9fe2277
fix lint
rarguelloF Nov 19, 2024
04ebf84
fix tests
rarguelloF Nov 20, 2024
0a6b4ae
Merge branch 'main' into rarguellof/APPSEC-55883/fix-net-http-server
rarguelloF Nov 20, 2024
fbc4f4d
remove tab
rarguelloF Nov 20, 2024
7f19466
update test
rarguelloF Nov 20, 2024
81f79b0
fix fiber test
rarguelloF Nov 20, 2024
9cebe89
use *testing.B from the subtest instead of the parent test
rarguelloF Nov 20, 2024
7c847e7
fix lint
rarguelloF Nov 20, 2024
f691bcd
fix: properly forward link.deps from transitive dependencies even if …
RomainMuller Nov 20, 2024
7a289c4
Merge branch 'main' into rarguellof/APPSEC-55883/fix-net-http-server
RomainMuller Nov 20, 2024
065ecf7
clean up some of the on-compile hook to make it easier to follow
RomainMuller Nov 21, 2024
a290c3a
Merge remote-tracking branch 'origin/rarguellof/APPSEC-55883/fix-net-…
RomainMuller Nov 21, 2024
4995082
Remove excessive pruning of link.deps file
RomainMuller Nov 21, 2024
a28ba36
Merge remote-tracking branch 'origin/main' into rarguellof/APPSEC-558…
RomainMuller Nov 21, 2024
f868c46
Fix unit test
RomainMuller Nov 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 25 additions & 5 deletions _integration-tests/tests/chi.v5/chi.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
},
},
},
},
Expand Down
30 changes: 27 additions & 3 deletions _integration-tests/tests/echo.v4/echo.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
},
},
},
},
},
Expand Down
13 changes: 10 additions & 3 deletions _integration-tests/tests/fiber.v2/fiber.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
},
},
},
Expand Down
29 changes: 26 additions & 3 deletions _integration-tests/tests/gin/gin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
},
},
},
},
},
Expand Down
4 changes: 2 additions & 2 deletions _integration-tests/tests/mux/gen_test.go → ...ation-tests/tests/gorilla_mux/gen_test.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

25 changes: 5 additions & 20 deletions _integration-tests/tests/mux/mux.go → ...on-tests/tests/gorilla_mux/gorilla_mux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

//go:build integration

package mux
package gorilla_mux

import (
"context"
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand All @@ -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,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it expected that the children are gone here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, see in the diff there was a comment // FIXME: this span shouldn't exist
This is because we were somehow double instrumenting something (not sure where exactly).

Before (4 spans):

client -> server (net/http) -> server (gorilla/mux) -> server (net/http)

Now (3 spans):

client -> server (net/http) -> server (gorilla/mux)

},
},
},
Expand Down
54 changes: 31 additions & 23 deletions ...egration-tests/tests/net_http/net_http.go → _integration-tests/tests/net_http/base.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{
rarguelloF marked this conversation as resolved.
Show resolved Hide resolved
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{
Expand Down Expand Up @@ -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",
Expand All @@ -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",
},
Expand All @@ -113,29 +114,36 @@ 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()))
return
}
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()))
return
}

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)
Expand Down
Loading