From 496de29a7b37b7db64166963c737b59cb2fc4145 Mon Sep 17 00:00:00 2001 From: Andrew Wilkins Date: Mon, 1 Feb 2021 11:31:46 +0800 Subject: [PATCH] Tidy up modelwriter Some light refactoring to move "context" model building logic out of modelwriter and into the Context.build method. There remains some context model building in modelwriter (related to setting destination service type) because it requires additional information outside of "context", i.e. the span type. --- context.go | 31 ++++++++++++++++++++---------- error.go | 1 + modelwriter.go | 9 --------- sanitizer_test.go | 49 ++++++++++++++++++++++++++++++----------------- transaction.go | 5 +---- 5 files changed, 54 insertions(+), 41 deletions(-) diff --git a/context.go b/context.go index 53f010f80..53338c2a2 100644 --- a/context.go +++ b/context.go @@ -22,6 +22,7 @@ import ( "net/http" "go.elastic.co/apm/internal/apmhttputil" + "go.elastic.co/apm/internal/wildcard" "go.elastic.co/apm/model" ) @@ -29,16 +30,17 @@ import ( // // NOTE this is entirely unrelated to the standard library's context.Context. type Context struct { - model model.Context - request model.Request - requestBody model.RequestBody - requestSocket model.RequestSocket - response model.Response - user model.User - service model.Service - serviceFramework model.Framework - captureHeaders bool - captureBodyMask CaptureBodyMode + model model.Context + request model.Request + requestBody model.RequestBody + requestSocket model.RequestSocket + response model.Response + user model.User + service model.Service + serviceFramework model.Framework + captureHeaders bool + captureBodyMask CaptureBodyMode + sanitizedFieldNames wildcard.Matchers } func (c *Context) build() *model.Context { @@ -52,6 +54,15 @@ func (c *Context) build() *model.Context { default: return nil } + if len(c.sanitizedFieldNames) != 0 { + if c.model.Request != nil { + sanitizeRequest(c.model.Request, c.sanitizedFieldNames) + } + if c.model.Response != nil { + sanitizeResponse(c.model.Response, c.sanitizedFieldNames) + } + + } return &c.model } diff --git a/error.go b/error.go index 14023f4be..8ed096859 100644 --- a/error.go +++ b/error.go @@ -148,6 +148,7 @@ func (t *Tracer) newError() *Error { if e.recording { e.Timestamp = time.Now() e.Context.captureHeaders = instrumentationConfig.captureHeaders + e.Context.sanitizedFieldNames = instrumentationConfig.sanitizedFieldNames e.stackTraceLimit = instrumentationConfig.stackTraceLimit } diff --git a/modelwriter.go b/modelwriter.go index be0a37e3e..65fcfcdc8 100644 --- a/modelwriter.go +++ b/modelwriter.go @@ -125,15 +125,6 @@ func (w *modelWriter) buildModelTransaction(out *model.Transaction, tx *Transact if sampled { out.Context = td.Context.build() } - - if len(td.sanitizedFieldNames) != 0 && out.Context != nil { - if out.Context.Request != nil { - sanitizeRequest(out.Context.Request, td.sanitizedFieldNames) - } - if out.Context.Response != nil { - sanitizeResponse(out.Context.Response, td.sanitizedFieldNames) - } - } } func (w *modelWriter) buildModelSpan(out *model.Span, span *Span, sd *SpanData) { diff --git a/sanitizer_test.go b/sanitizer_test.go index 665a3c949..9b5af1cab 100644 --- a/sanitizer_test.go +++ b/sanitizer_test.go @@ -22,6 +22,7 @@ import ( "net/http" "testing" + "github.com/pkg/errors" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -43,34 +44,46 @@ func TestSanitizeRequestResponse(t *testing.T) { req.AddCookie(c) } - tx, _, _ := apmtest.WithTransaction(func(ctx context.Context) { + tx, _, errors := apmtest.WithTransaction(func(ctx context.Context) { + e := apm.CaptureError(ctx, errors.New("boom!")) + defer e.Send() + tx := apm.TransactionFromContext(ctx) tx.Context.SetHTTPRequest(req) + e.Context.SetHTTPRequest(req) h := make(http.Header) h.Add("Set-Cookie", (&http.Cookie{Name: "foo", Value: "bar"}).String()) h.Add("Set-Cookie", (&http.Cookie{Name: "baz", Value: "qux"}).String()) tx.Context.SetHTTPResponseHeaders(h) tx.Context.SetHTTPStatusCode(http.StatusTeapot) + e.Context.SetHTTPResponseHeaders(h) + e.Context.SetHTTPStatusCode(http.StatusTeapot) }) - assert.Equal(t, tx.Context.Request.Cookies, model.Cookies{ - {Name: "Custom-Credit-Card-Number", Value: "[REDACTED]"}, - {Name: "secret", Value: "[REDACTED]"}, - {Name: "sessionid", Value: "[REDACTED]"}, - {Name: "user_id", Value: "456"}, - }) - assert.Equal(t, model.Headers{{ - Key: "Authorization", - Values: []string{"[REDACTED]"}, - }}, tx.Context.Request.Headers) - - // NOTE: the response includes multiple Set-Cookie headers, - // but we only report a single "[REDACTED]" value. - assert.Equal(t, model.Headers{{ - Key: "Set-Cookie", - Values: []string{"[REDACTED]"}, - }}, tx.Context.Response.Headers) + checkContext := func(context *model.Context) { + assert.Equal(t, context.Request.Cookies, model.Cookies{ + {Name: "Custom-Credit-Card-Number", Value: "[REDACTED]"}, + {Name: "secret", Value: "[REDACTED]"}, + {Name: "sessionid", Value: "[REDACTED]"}, + {Name: "user_id", Value: "456"}, + }) + assert.Equal(t, model.Headers{{ + Key: "Authorization", + Values: []string{"[REDACTED]"}, + }}, context.Request.Headers) + + // NOTE: the response includes multiple Set-Cookie headers, + // but we only report a single "[REDACTED]" value. + assert.Equal(t, model.Headers{{ + Key: "Set-Cookie", + Values: []string{"[REDACTED]"}, + }}, context.Response.Headers) + } + checkContext(tx.Context) + for _, e := range errors { + checkContext(e.Context) + } } func TestSetSanitizedFieldNamesNone(t *testing.T) { diff --git a/transaction.go b/transaction.go index 1ab427995..347bc5e05 100644 --- a/transaction.go +++ b/transaction.go @@ -23,8 +23,6 @@ import ( "math/rand" "sync" "time" - - "go.elastic.co/apm/internal/wildcard" ) // StartTransaction returns a new Transaction with the specified @@ -68,7 +66,7 @@ func (t *Tracer) StartTransactionOptions(name, transactionType string, opts Tran tx.stackTraceLimit = instrumentationConfig.stackTraceLimit tx.Context.captureHeaders = instrumentationConfig.captureHeaders tx.propagateLegacyHeader = instrumentationConfig.propagateLegacyHeader - tx.sanitizedFieldNames = instrumentationConfig.sanitizedFieldNames + tx.Context.sanitizedFieldNames = instrumentationConfig.sanitizedFieldNames tx.breakdownMetricsEnabled = t.breakdownMetrics.enabled var root bool @@ -346,7 +344,6 @@ type TransactionData struct { stackTraceLimit int breakdownMetricsEnabled bool propagateLegacyHeader bool - sanitizedFieldNames wildcard.Matchers timestamp time.Time mu sync.Mutex