Skip to content

Commit

Permalink
fix: net/http server-side instrumentation (#403)
Browse files Browse the repository at this point in the history 
Resolves #173, resolves
#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
(#173).
- We double-trace `http.HandlerFunc(func(w http.ResponseWriter, r
*http.Request))` declarations.
- We might even break the build (as described in
#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 <[email protected]>
Co-authored-by: Romain Marcadier <[email protected]>
  • Loading branch information
@rarguelloF @RomainMuller
3 people authored Nov 22, 2024
1 parent 657c57f commit a8237ee
Show file tree
Hide file tree
Showing 31 changed files with 600 additions and 545 deletions.
30 changes: 25 additions & 5 deletions _integration-tests/tests/chi.v5/chi.go
View file
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
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
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
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

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
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,
},
},
},
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
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{
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

0 comments on commit a8237ee

Please sign in to comment.