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.

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

[apmfasthttp] fix: set response context with closer close #1193

Merged
merged 14 commits into from Feb 21, 2022
10 changes: 7 additions & 3 deletions module/apmfasthttp/closer.go
Expand Up @@ -18,14 +18,16 @@
package apmfasthttp // import "go.elastic.co/apm/module/apmfasthttp/v2"

import (
"github.com/valyala/fasthttp"
"go.elastic.co/apm/v2"
)

// newTxCloser returns a transaction closer.
func newTxCloser(tx *apm.Transaction, bc *apm.BodyCapturer) *txCloser {
func newTxCloser(ctx *fasthttp.RequestCtx, tx *apm.Transaction, bc *apm.BodyCapturer) *txCloser {
return &txCloser{
tx: tx,
bc: bc,
ctx: ctx,
tx: tx,
bc: bc,
}
}

Expand All @@ -38,7 +40,9 @@ func newTxCloser(tx *apm.Transaction, bc *apm.BodyCapturer) *txCloser {
// RequestHandler. Additionally, Close method is called on each value
// implementing io.Closer before removing the value from ctx.
func (c *txCloser) Close() error {
setResponseContext(c.ctx, c.tx, c.bc)
c.tx.End()
c.bc.Discard()

return nil
}
11 changes: 7 additions & 4 deletions module/apmfasthttp/context.go
Expand Up @@ -64,8 +64,8 @@ func setRequestContext(ctx *fasthttp.RequestCtx, tracer *apm.Tracer, tx *apm.Tra

func setResponseContext(ctx *fasthttp.RequestCtx, tx *apm.Transaction, bc *apm.BodyCapturer) {
statusCode := ctx.Response.Header.StatusCode()

tx.Result = apmhttp.StatusCodeResult(statusCode)

if !tx.Sampled() {
return
}
Expand All @@ -80,8 +80,6 @@ func setResponseContext(ctx *fasthttp.RequestCtx, tx *apm.Transaction, bc *apm.B

tx.Context.SetHTTPResponseHeaders(headers)
tx.Context.SetHTTPStatusCode(statusCode)

return
}

// StartTransactionWithBody returns a new Transaction with name,
Expand Down Expand Up @@ -111,7 +109,12 @@ func StartTransactionWithBody(
return nil, nil, err
}

ctx.SetUserValue(txKey, newTxCloser(tx, bc))
// NOTE: the fasthttp documentation states that references to RequestCtx
// must not be held after returning from RequestHandler. This is due to
// RequestCtx being pooled, and released after the RequestHandler returns.
// However, it is safe to reference and call RequestCtx in a closer, as the
// closer is invoked before releasing the RequestCtx back to the pool.
ctx.SetUserValue(txKey, newTxCloser(ctx, tx, bc))
savsgio marked this conversation as resolved.
Show resolved Hide resolved

return tx, bc, nil
}
2 changes: 1 addition & 1 deletion module/apmfasthttp/go.mod
Expand Up @@ -5,7 +5,7 @@ go 1.13
require (
github.com/stretchr/testify v1.6.1
github.com/valyala/bytebufferpool v1.0.0
github.com/valyala/fasthttp v1.26.0
github.com/valyala/fasthttp v1.33.0
go.elastic.co/apm/module/apmhttp/v2 v2.0.0
go.elastic.co/apm/v2 v2.0.0
)
Expand Down
30 changes: 15 additions & 15 deletions module/apmfasthttp/go.sum
@@ -1,5 +1,5 @@
github.com/andybalholm/brotli v1.0.2 h1:JKnhI/XQ75uFBTiuzXpzFrUriDPiZjlOSzh6wXogP0E=
github.com/andybalholm/brotli v1.0.2/go.mod h1:loMXtMfwqflxFJPmdbJO0a3KNoPuLBgiu3qAvBg8x/Y=
github.com/andybalholm/brotli v1.0.4 h1:V7DdXeJtZscaqfNuAdSRuRFzuiKlHSC/Zh3zl9qY3JY=
github.com/andybalholm/brotli v1.0.4/go.mod h1:fO7iG3H7G2nSZ7m0zPUDn85XEX2GTukHGRSepvi9Eig=
github.com/armon/go-radix v1.0.0 h1:F4z6KzEeeQIMeLFa97iZU6vupzoecKdU5TX24SNppXI=
github.com/armon/go-radix v1.0.0/go.mod h1:ufUuZ+zHj4x4TnLV4JWEpy2hxWSpsRywHrMgIH9cCH8=
github.com/davecgh/go-spew v1.1.0 h1:ZDRjVQ15GmhC3fiQ8ni8+OwkZQO4DARzQgrnXU1Liz8=
Expand All @@ -10,16 +10,15 @@ github.com/elastic/go-sysinfo v1.1.1 h1:ZVlaLDyhVkDfjwPGU55CQRCRolNpc7P0BbyhhQZQ
github.com/elastic/go-sysinfo v1.1.1/go.mod h1:i1ZYdU10oLNfRzq4vq62BEwD2fH8KaWh6eh0ikPT9F0=
github.com/elastic/go-windows v1.0.0 h1:qLURgZFkkrYyTTkvYpsZIgf83AUsdIHfvlJaqaZ7aSY=
github.com/elastic/go-windows v1.0.0/go.mod h1:TsU0Nrp7/y3+VwE82FoZF8gC/XFg/Elz6CcloAxnPgU=
github.com/golang/snappy v0.0.3/go.mod h1:/XxbfmMg8lxefKM7IXC3fBNl/7bRcc72aCRzEWrmP2Q=
github.com/google/go-cmp v0.3.1 h1:Xye71clBPdm5HgqGwUkwhbynsUJZhDbS20FvLhQ2izg=
github.com/google/go-cmp v0.3.1/go.mod h1:8QqcDgzrUqlUb/G2PQTWiueGozuR1884gddMywk6iLU=
github.com/jcchavezs/porto v0.1.0 h1:Xmxxn25zQMmgE7/yHYmh19KcItG81hIwfbEEFnd6w/Q=
github.com/jcchavezs/porto v0.1.0/go.mod h1:fESH0gzDHiutHRdX2hv27ojnOVFco37hg1W6E9EZF4A=
github.com/jessevdk/go-flags v1.4.0/go.mod h1:4FA24M0QyGHXBuZZK/XkWh8h0e1EYbRYJSGM75WSRxI=
github.com/joeshaw/multierror v0.0.0-20140124173710-69b34d4ec901 h1:rp+c0RAYOWj8l6qbCUTSiRLG/iKnW3K3/QfPPuSsBt4=
github.com/joeshaw/multierror v0.0.0-20140124173710-69b34d4ec901/go.mod h1:Z86h9688Y0wesXCyonoVr47MasHilkuLMqGhRZ4Hpak=
github.com/klauspost/compress v1.12.2 h1:2KCfW3I9M7nSc5wOqXAlW2v2U6v+w6cbjvbfp+OykW8=
github.com/klauspost/compress v1.12.2/go.mod h1:8dP1Hq4DHOhN9w426knH3Rhby4rFm6D8eO+e+Dq5Gzg=
github.com/klauspost/compress v1.14.1 h1:hLQYb23E8/fO+1u53d02A97a8UnsddcvYzq4ERRU4ds=
github.com/klauspost/compress v1.14.1/go.mod h1:/3/Vjq9QcHkK5uEr5lBEmyoZ1iFhe47etQ6QUkpK6sk=
github.com/kr/pretty v0.1.0 h1:L/CwN0zerZDmRFUapSPitk6f+Q3+0za1rQkzVuMiMFI=
github.com/kr/pretty v0.1.0/go.mod h1:dAy3ld7l9f0ibDNOQOHHMYYIIbhfbHSm3C4ZsoJORNo=
github.com/kr/pty v1.1.1/go.mod h1:pFQYn66WHrOpPYNljwOMqo10TkYh1fy3cYio2l3bCsQ=
Expand All @@ -40,15 +39,15 @@ github.com/stretchr/testify v1.6.1 h1:hDPOHmpOpP40lSULcqw7IrRb/u7w6RpDC9399XyoNd
github.com/stretchr/testify v1.6.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg=
github.com/valyala/bytebufferpool v1.0.0 h1:GqA5TC/0021Y/b9FG4Oi9Mr3q7XYx6KllzawFIhcdPw=
github.com/valyala/bytebufferpool v1.0.0/go.mod h1:6bBcMArwyJ5K/AmCkWv1jt77kVWyCJ6HpOuEn7z0Csc=
github.com/valyala/fasthttp v1.26.0 h1:k5Tooi31zPG/g8yS6o2RffRO2C9B9Kah9SY8j/S7058=
github.com/valyala/fasthttp v1.26.0/go.mod h1:cmWIqlu99AO/RKcp1HWaViTqc57FswJOfYYdPJBl8BA=
github.com/valyala/fasthttp v1.33.0 h1:mHBKd98J5NcXuBddgjvim1i3kWzlng1SzLhrnBOU9g8=
github.com/valyala/fasthttp v1.33.0/go.mod h1:KJRK/MXx0J+yd0c5hlR+s1tIHD72sniU8ZJjl97LIw4=
github.com/valyala/tcplisten v1.0.0/go.mod h1:T0xQ8SeCZGxckz9qRXTfG43PvQ/mcWh7FwZEA7Ioqkc=
github.com/yuin/goldmark v1.1.27/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74=
go.elastic.co/fastjson v1.1.0 h1:3MrGBWWVIxe/xvsbpghtkFoPciPhOCmjsR/HfwEeQR4=
go.elastic.co/fastjson v1.1.0/go.mod h1:boNGISWMjQsUPy/t6yqt2/1Wx4YNPSe+mZjlyw9vKKI=
golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACkg1iLfiJU5Ep61QUkGW8qpdssI0+w=
golang.org/x/crypto v0.0.0-20191011191535-87dc89f01550/go.mod h1:yigFU9vqHzYiE8UmvKecakEJjdnWj3jj499lnFckfCI=
golang.org/x/crypto v0.0.0-20210513164829-c07d793c2f9a/go.mod h1:P+XmwS30IXTQdn5tA2iutPOUgjI07+tq3H3K9MVA1s8=
golang.org/x/crypto v0.0.0-20220112180741-5e0467b6c7ce/go.mod h1:IxCIyHEi3zRg3s0A5j5BB6A9Jmi73HwBIUl50j+osU4=
golang.org/x/lint v0.0.0-20201208152925-83fdc39ff7b5 h1:2M3HP5CCK1Si9FQhwnzYhXdG6DXeebvUHFpre8QvbyI=
golang.org/x/lint v0.0.0-20201208152925-83fdc39ff7b5/go.mod h1:3xt1FjdF8hUf6vQPIChWIBhFzV8gjjsPE/fR3IyQdNY=
golang.org/x/mod v0.1.1-0.20191105210325-c90efee705ee/go.mod h1:QqPTAvyqsEbceGzBzNggFXnrqF1CaUcvgkdR5Ot7KZg=
Expand All @@ -58,9 +57,9 @@ golang.org/x/mod v0.3.0/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA=
golang.org/x/net v0.0.0-20190404232315-eb5bcb51f2a3/go.mod h1:t9HGtf8HONx5eT2rtn7q6eTqICYqUVnKs3thJo3Qplg=
golang.org/x/net v0.0.0-20190620200207-3b0461eec859/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s=
golang.org/x/net v0.0.0-20200226121028-0de0cce0169b/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s=
golang.org/x/net v0.0.0-20210226172049-e18ecbb05110/go.mod h1:m0MpNAwzfU5UDzcl9v0D8zg8gWTRqZa9RBIspLL5mdg=
golang.org/x/net v0.0.0-20210510120150-4163338589ed h1:p9UgmWI9wKpfYmgaV/IZKGdXc5qEK45tDwwwDyjS26I=
golang.org/x/net v0.0.0-20210510120150-4163338589ed/go.mod h1:9nx3DQGgdP8bBQD5qxJ1jj9UTztislL4KSBs9R2vV5Y=
golang.org/x/net v0.0.0-20211112202133-69e39bad7dc2/go.mod h1:9nx3DQGgdP8bBQD5qxJ1jj9UTztislL4KSBs9R2vV5Y=
golang.org/x/net v0.0.0-20220111093109-d55c255bac03 h1:0FB83qp0AzVJm+0wcIlauAjJ+tNdh7jLuacRYCIVv7s=
golang.org/x/net v0.0.0-20220111093109-d55c255bac03/go.mod h1:9nx3DQGgdP8bBQD5qxJ1jj9UTztislL4KSBs9R2vV5Y=
golang.org/x/sync v0.0.0-20181221193216-37e7f081c4d4/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
golang.org/x/sync v0.0.0-20190423024810-112230192c58/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
golang.org/x/sync v0.0.0-20190911185100-cd5d95a43a6e/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
Expand All @@ -70,14 +69,15 @@ golang.org/x/sys v0.0.0-20191025021431-6c3a3bfe00ae/go.mod h1:h1NjWce9XRLGQEsW7w
golang.org/x/sys v0.0.0-20191204072324-ce4227a45e2e/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
golang.org/x/sys v0.0.0-20201119102817-f84b799fce68/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
golang.org/x/sys v0.0.0-20210423082822-04245dca01da/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
golang.org/x/sys v0.0.0-20210514084401-e8d321eab015 h1:hZR0X1kPW+nwyJ9xRxqZk1vx5RUObAPBdKVvXPDUH/E=
golang.org/x/sys v0.0.0-20210514084401-e8d321eab015/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
golang.org/x/sys v0.0.0-20210615035016-665e8c7367d1/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
golang.org/x/sys v0.0.0-20220111092808-5a964db01320 h1:0jf+tOCoZ3LyutmCOWpVni1chK4VfFLhRsDK7MhqGRY=
golang.org/x/sys v0.0.0-20220111092808-5a964db01320/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1/go.mod h1:bj7SfCRtBDWHUb9snDiAeCFNEtKQo2Wmx5Cou7ajbmo=
golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ=
golang.org/x/text v0.3.2/go.mod h1:bEr9sfX3Q8Zfm5fL9x+3itogRgK3+ptLWKqgva+5dAk=
golang.org/x/text v0.3.3/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ=
golang.org/x/text v0.3.6 h1:aRYxNxv6iGQlyVaZmk6ZgYEDa+Jg18DxebPSrd6bg1M=
golang.org/x/text v0.3.6/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ=
golang.org/x/text v0.3.7 h1:olpwvP2KacW1ZWvsR7uQhoyTYvKAupfQrRGBFM352Gk=
golang.org/x/text v0.3.7/go.mod h1:u+2+/6zg+i71rQMx5EYifcz6MCKuco9NR6JIITiCfzQ=
golang.org/x/tools v0.0.0-20180917221912-90fa682c2a6e/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ=
golang.org/x/tools v0.0.0-20191119224855-298f0cb1881e/go.mod h1:b+2E5dAYhXwXZwtnZ6UAqBI28+e2cm9otk0dWdXHAEo=
golang.org/x/tools v0.0.0-20200130002326-2f3ba24bd6e7/go.mod h1:TB2adYChydJhpapKDTa4BR/hXlZSLoq2Wpct/0txZ28=
Expand Down
2 changes: 0 additions & 2 deletions module/apmfasthttp/server.go
Expand Up @@ -88,8 +88,6 @@ func (h *apmHandler) handler(ctx *fasthttp.RequestCtx) {
}()

h.requestHandler(ctx)

setResponseContext(ctx, tx, bc)
}

// WithTracer returns a ServerOption which sets t as the tracer
Expand Down
139 changes: 117 additions & 22 deletions module/apmfasthttp/server_test.go
Expand Up @@ -18,10 +18,12 @@
package apmfasthttp_test

import (
"bufio"
"io"
"io/ioutil"
"net"
"net/http"
"sync"
"testing"
"time"

Expand All @@ -30,37 +32,42 @@ import (
"github.com/valyala/fasthttp"

"go.elastic.co/apm/module/apmfasthttp/v2"
"go.elastic.co/apm/v2/model"
"go.elastic.co/apm/v2/transport/transporttest"
)

func TestServerHTTPResponse(t *testing.T) {
type assertFunc func(model.Transaction, *http.Response)

func testServer(t *testing.T, s *fasthttp.Server, wg *sync.WaitGroup, assertFn assertFunc) {
t.Helper()

tracer, transport := transporttest.NewRecorderTracer()
defer tracer.Close()

handler := func(ctx *fasthttp.RequestCtx) {
ctx.Error(fasthttp.StatusMessage(fasthttp.StatusUnauthorized), fasthttp.StatusUnauthorized)
}
handler = apmfasthttp.Wrap(handler, apmfasthttp.WithTracer(tracer))
shutdown := make(chan error)
defer close(shutdown)

ln, err := net.Listen("tcp", "127.0.0.1:")
require.NoError(t, err)

s := &fasthttp.Server{
Handler: handler,
Name: "test-server",
}
s.Handler = apmfasthttp.Wrap(s.Handler, apmfasthttp.WithTracer(tracer))

go func() {
shutdown <- s.Serve(ln)
}()

addr := ln.Addr().String()

resp, err := http.Get("http://" + addr)
require.NoError(t, err)
io.Copy(ioutil.Discard, resp.Body) // consume body to complete request handler

_, _ = io.Copy(ioutil.Discard, resp.Body) // consume body to complete request handler
resp.Body.Close()
assert.Equal(t, fasthttp.StatusUnauthorized, resp.StatusCode)

if wg != nil {
wg.Wait()
}

tracer.Flush(nil)
payloads := transport.Payloads()

Expand All @@ -71,26 +78,114 @@ func TestServerHTTPResponse(t *testing.T) {
// This is unique to fasthttp's implementation.
timer := time.NewTimer(100 * time.Millisecond)
defer timer.Stop()
for {

for len(payloads.Transactions) == 0 {
select {
case <-timer.C:
t.Fatal("timed out waiting for payload")
default:
time.Sleep(100 * time.Millisecond)
tracer.Flush(nil)
payloads = transport.Payloads()
}
if len(payloads.Transactions) > 0 {
break
}
time.Sleep(10 * time.Millisecond)
tracer.Flush(nil)
payloads = transport.Payloads()
}

transaction := payloads.Transactions[0]
assert.Equal(t, "GET /", transaction.Name)
assert.Equal(t, "request", transaction.Type)
assert.Equal(t, "HTTP 4xx", transaction.Result)
assertFn(payloads.Transactions[0], resp)

// s.Serve returns on ln.Close()
ln.Close()
require.NoError(t, <-shutdown)
}

func TestServerHTTPResponse(t *testing.T) {
s := &fasthttp.Server{
Handler: func(ctx *fasthttp.RequestCtx) {
ctx.Error(fasthttp.StatusMessage(fasthttp.StatusUnauthorized), fasthttp.StatusUnauthorized)
},
Name: "test-server",
}

assertFn := func(transaction model.Transaction, resp *http.Response) {
expectedHeaders := model.Headers{
{Key: "Content-Length", Values: []string{"12"}},
{Key: "Content-Type", Values: []string{"text/plain; charset=utf-8"}},
{Key: "Server", Values: []string{s.Name}},
}

assert.Equal(t, fasthttp.StatusUnauthorized, resp.StatusCode)
assert.Equal(t, resp.StatusCode, transaction.Context.Response.StatusCode)
assert.Equal(t, "GET /", transaction.Name)
assert.Equal(t, "request", transaction.Type)
assert.Equal(t, "HTTP 4xx", transaction.Result)
assert.Equal(t, expectedHeaders, transaction.Context.Response.Headers)
}

testServer(t, s, nil, assertFn)
}

func TestServerHTTPResponseStream(t *testing.T) {
streamResponseDuration := time.Millisecond * 100

s := &fasthttp.Server{
Handler: func(ctx *fasthttp.RequestCtx) {
ctx.SetBodyStreamWriter(func(w *bufio.Writer) {
w.WriteString("Hello world")
time.Sleep(streamResponseDuration)
})
},
Name: "test-server",
}

assertFn := func(transaction model.Transaction, resp *http.Response) {
expectedHeaders := model.Headers{
{Key: "Content-Type", Values: []string{"text/plain; charset=utf-8"}},
{Key: "Server", Values: []string{s.Name}},
{Key: "Transfer-Encoding", Values: []string{"chunked"}},
}

assert.Equal(t, fasthttp.StatusOK, resp.StatusCode)
assert.Equal(t, resp.StatusCode, transaction.Context.Response.StatusCode)
assert.Equal(t, "GET /", transaction.Name)
assert.Equal(t, "request", transaction.Type)
assert.Equal(t, "HTTP 2xx", transaction.Result)
assert.GreaterOrEqual(t, transaction.Duration, float64(streamResponseDuration.Milliseconds()))
assert.Equal(t, transaction.Context.Response.Headers, expectedHeaders)
}

testServer(t, s, nil, assertFn)
}

func TestServerHTTPResponseHijack(t *testing.T) {
hijackResponseDuration := time.Millisecond * 100

wg := new(sync.WaitGroup)
wg.Add(1)

s := &fasthttp.Server{
Handler: func(ctx *fasthttp.RequestCtx) {
ctx.Hijack(func(c net.Conn) {
time.Sleep(hijackResponseDuration)
wg.Done()
})
},
Name: "test-server",
}

assertFn := func(transaction model.Transaction, resp *http.Response) {
expectedHeaders := model.Headers{
{Key: "Content-Length", Values: []string{"0"}},
{Key: "Content-Type", Values: []string{"text/plain; charset=utf-8"}},
{Key: "Server", Values: []string{s.Name}},
}

assert.Equal(t, fasthttp.StatusOK, resp.StatusCode)
assert.Equal(t, resp.StatusCode, transaction.Context.Response.StatusCode)
assert.Equal(t, "GET /", transaction.Name)
assert.Equal(t, "request", transaction.Type)
assert.Equal(t, "HTTP 2xx", transaction.Result)
assert.GreaterOrEqual(t, transaction.Duration, float64(hijackResponseDuration.Milliseconds()))
assert.Equal(t, transaction.Context.Response.Headers, expectedHeaders)
}

testServer(t, s, wg, assertFn)
}