From 3722c76f5b03f97a46a00773d3b1c12611d0e350 Mon Sep 17 00:00:00 2001 From: Andrew Wilkins Date: Tue, 2 Oct 2018 18:25:07 +0800 Subject: [PATCH 1/3] model: add Context.Service Enable transactions and errors to override the service in their context. At the time of writing, the server merges back-to-front such that service-level overrides event-level, but even that's OK for our purposes as we will only set framework info in the transaction context, and never in service-level metadata. --- model/marshal_fastjson.go | 75 ++++++++++++++++++++++++++++++++++----- model/marshal_test.go | 14 +++++++- model/model.go | 7 ++-- 3 files changed, 84 insertions(+), 12 deletions(-) diff --git a/model/marshal_fastjson.go b/model/marshal_fastjson.go index 62b027f5f..006d37640 100644 --- a/model/marshal_fastjson.go +++ b/model/marshal_fastjson.go @@ -8,28 +8,75 @@ import ( func (v *Service) MarshalFastJSON(w *fastjson.Writer) { w.RawByte('{') - w.RawString("\"agent\":") - v.Agent.MarshalFastJSON(w) - w.RawString(",\"name\":") - w.String(v.Name) + first := true + if v.Agent != nil { + const prefix = ",\"agent\":" + if first { + first = false + w.RawString(prefix[1:]) + } else { + w.RawString(prefix) + } + v.Agent.MarshalFastJSON(w) + } if v.Environment != "" { - w.RawString(",\"environment\":") + const prefix = ",\"environment\":" + if first { + first = false + w.RawString(prefix[1:]) + } else { + w.RawString(prefix) + } w.String(v.Environment) } if v.Framework != nil { - w.RawString(",\"framework\":") + const prefix = ",\"framework\":" + if first { + first = false + w.RawString(prefix[1:]) + } else { + w.RawString(prefix) + } v.Framework.MarshalFastJSON(w) } if v.Language != nil { - w.RawString(",\"language\":") + const prefix = ",\"language\":" + if first { + first = false + w.RawString(prefix[1:]) + } else { + w.RawString(prefix) + } v.Language.MarshalFastJSON(w) } + if v.Name != "" { + const prefix = ",\"name\":" + if first { + first = false + w.RawString(prefix[1:]) + } else { + w.RawString(prefix) + } + w.String(v.Name) + } if v.Runtime != nil { - w.RawString(",\"runtime\":") + const prefix = ",\"runtime\":" + if first { + first = false + w.RawString(prefix[1:]) + } else { + w.RawString(prefix) + } v.Runtime.MarshalFastJSON(w) } if v.Version != "" { - w.RawString(",\"version\":") + const prefix = ",\"version\":" + if first { + first = false + w.RawString(prefix[1:]) + } else { + w.RawString(prefix) + } w.String(v.Version) } w.RawByte('}') @@ -348,6 +395,16 @@ func (v *Context) MarshalFastJSON(w *fastjson.Writer) { } v.Response.MarshalFastJSON(w) } + if v.Service != nil { + const prefix = ",\"service\":" + if first { + first = false + w.RawString(prefix[1:]) + } else { + w.RawString(prefix) + } + v.Service.MarshalFastJSON(w) + } if v.Tags != nil { const prefix = ",\"tags\":" if first { diff --git a/model/marshal_test.go b/model/marshal_test.go index 7d6f62a62..f7dfeb9e8 100644 --- a/model/marshal_test.go +++ b/model/marshal_test.go @@ -33,6 +33,12 @@ func TestMarshalTransaction(t *testing.T) { "duration": 123.456, "result": "418", "context": map[string]interface{}{ + "service": map[string]interface{}{ + "framework": map[string]interface{}{ + "name": "framework-name", + "version": "framework-version", + }, + }, "request": map[string]interface{}{ "url": map[string]interface{}{ "full": "https://testing.invalid/foo/bar?baz#qux", @@ -539,6 +545,12 @@ func fakeTransaction() model.Transaction { Tags: map[string]string{ "tag": "urit", }, + Service: &model.Service{ + Framework: &model.Framework{ + Name: "framework-name", + Version: "framework-version", + }, + }, }, SpanCount: model.SpanCount{ Started: 99, @@ -597,7 +609,7 @@ func fakeService() *model.Service { Name: "fake-service", Version: "1.0.0-rc1", Environment: "dev", - Agent: model.Agent{ + Agent: &model.Agent{ Name: "go", Version: "0.1.0", }, diff --git a/model/model.go b/model/model.go index 05f930dae..4d72adb8d 100644 --- a/model/model.go +++ b/model/model.go @@ -9,7 +9,7 @@ import ( // Service represents the service handling transactions being traced. type Service struct { // Name is the immutable name of the service. - Name string `json:"name"` + Name string `json:"name,omitempty"` // Version is the version of the service, if it has one. Version string `json:"version,omitempty"` @@ -20,7 +20,7 @@ type Service struct { // Agent holds information about the Elastic APM agent tracing this // service's transactions. - Agent Agent `json:"agent"` + Agent *Agent `json:"agent,omitempty"` // Framework holds information about the service's framework, if any. Framework *Framework `json:"framework,omitempty"` @@ -248,6 +248,9 @@ type Context struct { // Tags holds user-defined key/value pairs. Tags map[string]string `json:"tags,omitempty"` + + // Service holds values to overrides service-level metadata. + Service *Service `json:"service,omitempty"` } // User holds information about an authenticated user. From 9357fb9a849eb6e6d946e2beb9d51234ac605805 Mon Sep 17 00:00:00 2001 From: Andrew Wilkins Date: Wed, 3 Oct 2018 10:28:23 +0800 Subject: [PATCH 2/3] elasticapm: add Context.SetFramework Add a method for instrumentation to set the framework information for a transaction or error context. --- context.go | 44 +++++++++++++++++++++++++++++++++++--------- context_test.go | 49 ++++++++++++++++++++++++++++++++++++++----------- utils.go | 2 +- 3 files changed, 74 insertions(+), 21 deletions(-) diff --git a/context.go b/context.go index ac7de213b..37bfc2a04 100644 --- a/context.go +++ b/context.go @@ -11,15 +11,17 @@ import ( // Context provides methods for setting transaction and error context. type Context struct { - model model.Context - request model.Request - requestBody model.RequestBody - requestHeaders model.RequestHeaders - requestSocket model.RequestSocket - response model.Response - responseHeaders model.ResponseHeaders - user model.User - captureBodyMask CaptureBodyMode + model model.Context + request model.Request + requestBody model.RequestBody + requestHeaders model.RequestHeaders + requestSocket model.RequestSocket + response model.Response + responseHeaders model.ResponseHeaders + user model.User + service model.Service + serviceFramework model.Framework + captureBodyMask CaptureBodyMode } func (c *Context) build() *model.Context { @@ -27,6 +29,7 @@ func (c *Context) build() *model.Context { case c.model.Request != nil: case c.model.Response != nil: case c.model.User != nil: + case c.model.Service != nil: case len(c.model.Custom) != 0: case len(c.model.Tags) != 0: default: @@ -75,6 +78,29 @@ func (c *Context) SetTag(key, value string) { } } +// SetFramework sets the framework name and version in the context. +// +// This is used for identifying the framework in which the context +// was created, such as Gin or Echo. +// +// If the name is empty, this is a no-op. If version is empty, then +// it will be set to "unspecified". +func (c *Context) SetFramework(name, version string) { + if name == "" { + return + } + if version == "" { + // Framework version is required. + version = "unspecified" + } + c.serviceFramework = model.Framework{ + Name: truncateKeyword(name), + Version: truncateKeyword(version), + } + c.service.Framework = &c.serviceFramework + c.model.Service = &c.service +} + // SetHTTPRequest sets details of the HTTP request in the context. // // This function relates to server-side requests. Various proxy diff --git a/context_test.go b/context_test.go index 188ccb66e..b70c645a7 100644 --- a/context_test.go +++ b/context_test.go @@ -1,13 +1,15 @@ package elasticapm_test import ( + "context" "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "github.com/elastic/apm-agent-go" + "github.com/elastic/apm-agent-go/apmtest" "github.com/elastic/apm-agent-go/model" - "github.com/elastic/apm-agent-go/transport/transporttest" ) func TestContextUser(t *testing.T) { @@ -31,15 +33,40 @@ func TestContextUser(t *testing.T) { }) } -func testSendTransaction(t *testing.T, f func(tx *elasticapm.Transaction)) model.Transaction { - tracer, r := transporttest.NewRecorderTracer() - defer tracer.Close() - - tx := tracer.StartTransaction("name", "type") - f(tx) - tx.End() - tracer.Flush(nil) +func TestContextFramework(t *testing.T) { + t.Run("name_unspecified", func(t *testing.T) { + tx := testSendTransaction(t, func(tx *elasticapm.Transaction) { + tx.Context.SetFramework("", "1.0") + }) + assert.Nil(t, tx.Context) + }) + t.Run("version_specified", func(t *testing.T) { + tx := testSendTransaction(t, func(tx *elasticapm.Transaction) { + tx.Context.SetFramework("framework", "1.0") + }) + require.NotNil(t, tx.Context) + require.NotNil(t, tx.Context.Service) + assert.Equal(t, &model.Framework{ + Name: "framework", + Version: "1.0", + }, tx.Context.Service.Framework) + }) + t.Run("version_unspecified", func(t *testing.T) { + tx := testSendTransaction(t, func(tx *elasticapm.Transaction) { + tx.Context.SetFramework("framework", "") + }) + require.NotNil(t, tx.Context) + require.NotNil(t, tx.Context.Service) + assert.Equal(t, &model.Framework{ + Name: "framework", + Version: "unspecified", + }, tx.Context.Service.Framework) + }) +} - payloads := r.Payloads() - return payloads.Transactions[0] +func testSendTransaction(t *testing.T, f func(tx *elasticapm.Transaction)) model.Transaction { + transaction, _, _ := apmtest.WithTransaction(func(ctx context.Context) { + f(elasticapm.TransactionFromContext(ctx)) + }) + return transaction } diff --git a/utils.go b/utils.go index 8cfbab51b..8de5564b7 100644 --- a/utils.go +++ b/utils.go @@ -54,7 +54,7 @@ func makeService(name, version, environment string) model.Service { Name: truncateKeyword(name), Version: truncateKeyword(version), Environment: truncateKeyword(environment), - Agent: goAgent, + Agent: &goAgent, Language: &goLanguage, Runtime: &goRuntime, } From 804f997f7b21ed1c25b367de8fdc7678b2c279ee Mon Sep 17 00:00:00 2001 From: Andrew Wilkins Date: Wed, 3 Oct 2018 10:54:12 +0800 Subject: [PATCH 3/3] module/...: update modules to set framework Update apmecho, apmgin, and apmgrpc server middleware to set the framework info. --- module/apmecho/middleware.go | 1 + module/apmecho/middleware_test.go | 6 ++++++ module/apmgin/middleware.go | 1 + module/apmgin/middleware_test.go | 6 ++++++ module/apmgrpc/server.go | 2 ++ module/apmgrpc/server_test.go | 6 ++++++ 6 files changed, 22 insertions(+) diff --git a/module/apmecho/middleware.go b/module/apmecho/middleware.go index 45aa5319f..12b1a8463 100644 --- a/module/apmecho/middleware.go +++ b/module/apmecho/middleware.go @@ -86,6 +86,7 @@ func (m *middleware) handle(c echo.Context) error { } func setContext(ctx *elasticapm.Context, req *http.Request, resp *echo.Response, body *elasticapm.BodyCapturer) { + ctx.SetFramework("echo", echo.Version) ctx.SetHTTPRequest(req) ctx.SetHTTPRequestBody(body) if resp.Committed { diff --git a/module/apmecho/middleware_test.go b/module/apmecho/middleware_test.go index 10d80f5d8..16b9e9559 100644 --- a/module/apmecho/middleware_test.go +++ b/module/apmecho/middleware_test.go @@ -36,6 +36,12 @@ func TestEchoMiddleware(t *testing.T) { assert.Equal(t, "HTTP 4xx", transaction.Result) assert.Equal(t, &model.Context{ + Service: &model.Service{ + Framework: &model.Framework{ + Name: "echo", + Version: echo.Version, + }, + }, Request: &model.Request{ Socket: &model.RequestSocket{ RemoteAddress: "client.testing", diff --git a/module/apmgin/middleware.go b/module/apmgin/middleware.go index 9bb7266a3..ea2c49201 100644 --- a/module/apmgin/middleware.go +++ b/module/apmgin/middleware.go @@ -108,6 +108,7 @@ func (m *middleware) handle(c *gin.Context) { } func setContext(ctx *elasticapm.Context, c *gin.Context, body *elasticapm.BodyCapturer) { + ctx.SetFramework("gin", gin.Version) ctx.SetHTTPRequest(c.Request) ctx.SetHTTPRequestBody(body) if c.Writer.Written() { diff --git a/module/apmgin/middleware_test.go b/module/apmgin/middleware_test.go index 372e70d90..4d165f84d 100644 --- a/module/apmgin/middleware_test.go +++ b/module/apmgin/middleware_test.go @@ -42,6 +42,12 @@ func TestMiddleware(t *testing.T) { assert.Equal(t, "HTTP 2xx", transaction.Result) assert.Equal(t, &model.Context{ + Service: &model.Service{ + Framework: &model.Framework{ + Name: "gin", + Version: gin.Version, + }, + }, Request: &model.Request{ Socket: &model.RequestSocket{ RemoteAddress: "client.testing", diff --git a/module/apmgrpc/server.go b/module/apmgrpc/server.go index 0b1622d97..cf8486817 100644 --- a/module/apmgrpc/server.go +++ b/module/apmgrpc/server.go @@ -40,6 +40,7 @@ func NewUnaryServerInterceptor(o ...ServerOption) grpc.UnaryServerInterceptor { tx := opts.tracer.StartTransaction(info.FullMethod, "grpc") ctx = elasticapm.ContextWithTransaction(ctx, tx) defer tx.End() + tx.Context.SetFramework("grpc", grpc.Version) if tx.Sampled() { p, ok := peer.FromContext(ctx) @@ -61,6 +62,7 @@ func NewUnaryServerInterceptor(o ...ServerOption) grpc.UnaryServerInterceptor { if r != nil { e := opts.tracer.Recovered(r) e.SetTransaction(tx) + e.Context.SetFramework("grpc", grpc.Version) e.Handled = opts.recover e.Send() if opts.recover { diff --git a/module/apmgrpc/server_test.go b/module/apmgrpc/server_test.go index e5286fcbd..80265ffab 100644 --- a/module/apmgrpc/server_test.go +++ b/module/apmgrpc/server_test.go @@ -84,6 +84,12 @@ func testServerTransactionHappy(t *testing.T, p testParams) { assert.Contains(t, grpcContext, "peer.address") delete(grpcContext, "peer.address") assert.Equal(t, &model.Context{ + Service: &model.Service{ + Framework: &model.Framework{ + Name: "grpc", + Version: grpc.Version, + }, + }, Custom: model.IfaceMap{{ Key: "grpc", Value: map[string]interface{}{},