From d530114264438eee8fc6a749d13f2c9387d3bd66 Mon Sep 17 00:00:00 2001 From: chris erway Date: Tue, 23 Jun 2020 10:12:30 -0400 Subject: [PATCH 1/3] handle OpenTracing SetTag with "error" --- v1/ao/layer.go | 3 ++- v1/ao/opentracing/tracer.go | 35 +++++++++++++++++++++++++++++++++-- 2 files changed, 35 insertions(+), 3 deletions(-) diff --git a/v1/ao/layer.go b/v1/ao/layer.go index bad4c9ed..d5fb2c9b 100644 --- a/v1/ao/layer.go +++ b/v1/ao/layer.go @@ -6,6 +6,7 @@ import ( "context" "errors" "fmt" + "reflect" "runtime/debug" "sync" @@ -347,7 +348,7 @@ func (s *span) Err(err error) { if err == nil { return } - s.Error("error", err.Error()) + s.Error(reflect.TypeOf(err).String(), err.Error()) } // span satisfies the Extent interface and consolidates common reporting routines used by diff --git a/v1/ao/opentracing/tracer.go b/v1/ao/opentracing/tracer.go index e630b2bd..6e4055ff 100644 --- a/v1/ao/opentracing/tracer.go +++ b/v1/ao/opentracing/tracer.go @@ -3,10 +3,13 @@ package opentracing import ( + "fmt" + "reflect" "sync" "github.com/appoptics/appoptics-apm-go/v1/ao" ot "github.com/opentracing/opentracing-go" + "github.com/opentracing/opentracing-go/ext" "github.com/opentracing/opentracing-go/log" ) @@ -199,16 +202,44 @@ func (s *spanImpl) SetTag(key string, value interface{}) ot.Span { s.Lock() defer s.Unlock() // if transaction name is passed, set on the span - if tagName := translateTagName(key); tagName == "TransactionName" { + tagName := translateTagName(key) + switch tagName { + case "TransactionName": if txnName, ok := value.(string); ok { s.context.span.SetTransactionName(txnName) } - } else { + case string(ext.Error): + s.setErrorTag(value) + default: s.context.span.AddEndArgs(tagName, value) } return s } +// setErrorTag passes an OT error to the AO span.Error method. +func (s *spanImpl) setErrorTag(value interface{}) { + switch v := value.(type) { + case bool: + // OpenTracing spec defines bool value + if v { + s.context.span.Error(string(ext.Error), "true") + } + case error: + // handle error if provided + s.context.span.Err(v) + case string: + // error string provided + s.context.span.Error(string(ext.Error), v) + case fmt.Stringer: + s.context.span.Error(string(ext.Error), v.String()) + case nil: + // no error, ignore + default: + // an unknown error type, assume an error + s.context.span.Error(string(ext.Error), reflect.TypeOf(v).String()) + } +} + // LogEvent logs a event to the span. // // Deprecated: this method is deprecated. From 4a107e4047fbe379dbba71ad746b28a3ebca8344 Mon Sep 17 00:00:00 2001 From: chris erway Date: Tue, 23 Jun 2020 11:24:14 -0400 Subject: [PATCH 2/3] Restore 100% coverage for github.com/appoptics/appoptics-apm-go/v1/ao/opentracing --- v1/ao/layer.go | 3 +-- v1/ao/opentracing/tracer.go | 2 +- v1/ao/opentracing/tracer_test.go | 40 ++++++++++++++++++++++++++++++++ 3 files changed, 42 insertions(+), 3 deletions(-) diff --git a/v1/ao/layer.go b/v1/ao/layer.go index d5fb2c9b..bad4c9ed 100644 --- a/v1/ao/layer.go +++ b/v1/ao/layer.go @@ -6,7 +6,6 @@ import ( "context" "errors" "fmt" - "reflect" "runtime/debug" "sync" @@ -348,7 +347,7 @@ func (s *span) Err(err error) { if err == nil { return } - s.Error(reflect.TypeOf(err).String(), err.Error()) + s.Error("error", err.Error()) } // span satisfies the Extent interface and consolidates common reporting routines used by diff --git a/v1/ao/opentracing/tracer.go b/v1/ao/opentracing/tracer.go index 6e4055ff..0fdfe7f7 100644 --- a/v1/ao/opentracing/tracer.go +++ b/v1/ao/opentracing/tracer.go @@ -226,7 +226,7 @@ func (s *spanImpl) setErrorTag(value interface{}) { } case error: // handle error if provided - s.context.span.Err(v) + s.context.span.Error(reflect.TypeOf(v).String(), v.Error()) case string: // error string provided s.context.span.Error(string(ext.Error), v) diff --git a/v1/ao/opentracing/tracer_test.go b/v1/ao/opentracing/tracer_test.go index efdb2942..6be5f75b 100644 --- a/v1/ao/opentracing/tracer_test.go +++ b/v1/ao/opentracing/tracer_test.go @@ -3,6 +3,7 @@ package opentracing import ( + "fmt" "testing" g "github.com/appoptics/appoptics-apm-go/v1/ao/internal/graphtest" @@ -56,3 +57,42 @@ func TestOTSetTransactionName(t *testing.T) { func TestOTSetResourceName(t *testing.T) { testTransactionName(t, "resource.name", "myTxn2") } + +type customStringer struct{} + +func (customStringer) String() string { return "custom" } + +type weirdType struct{} + +func TestSetErrorTags(t *testing.T) { + // test a bunch of different args to SetTag("error", ...) and how they show up in trace event KVs + for _, tc := range []struct{ errorTagVal, errorClass, errorMsg interface{} }{ + {fmt.Errorf("An error!"), "*errors.errorString", "An error!"}, + {true, "error", "true"}, + {"error string", "error", "error string"}, + {customStringer{}, "error", "custom"}, + {weirdType{}, "error", "opentracing.weirdType"}, + } { + t.Run(fmt.Sprintf("Error tagval %v, errClass %v, errMsg %v", tc.errorTagVal, tc.errorClass, tc.errorMsg), func(t *testing.T) { + r := reporter.SetTestReporter() // set up test reporter + tr := NewTracer() + + span := tr.StartSpan("op") + assert.NotNil(t, span) + span.SetTag("error", tc.errorTagVal) + span.Finish() + + r.Close(3) + g.AssertGraph(t, r.EventBufs, 3, g.AssertNodeMap{ + {"op", "entry"}: {}, + {"op", "error"}: {Edges: g.Edges{{"op", "entry"}}, Callback: func(n g.Node) { + assert.Equal(t, tc.errorClass, n.Map["ErrorClass"]) + assert.Equal(t, tc.errorMsg, n.Map["ErrorMsg"]) + }}, + {"op", "exit"}: {Edges: g.Edges{{"op", "error"}}, Callback: func(n g.Node) { + + }}, + }) + }) + } +} From 27e4e32cac5466b1076fd68ad26083b0d1ae2f5c Mon Sep 17 00:00:00 2001 From: chris erway Date: Tue, 23 Jun 2020 11:44:27 -0400 Subject: [PATCH 3/3] Add test case for OpenTracing SetTag error false/nil --- v1/ao/opentracing/tracer_test.go | 24 +++++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) diff --git a/v1/ao/opentracing/tracer_test.go b/v1/ao/opentracing/tracer_test.go index 6be5f75b..05b53a1c 100644 --- a/v1/ao/opentracing/tracer_test.go +++ b/v1/ao/opentracing/tracer_test.go @@ -89,10 +89,28 @@ func TestSetErrorTags(t *testing.T) { assert.Equal(t, tc.errorClass, n.Map["ErrorClass"]) assert.Equal(t, tc.errorMsg, n.Map["ErrorMsg"]) }}, - {"op", "exit"}: {Edges: g.Edges{{"op", "error"}}, Callback: func(n g.Node) { - - }}, + {"op", "exit"}: {Edges: g.Edges{{"op", "error"}}, Callback: func(n g.Node) {}}, }) }) } + + // test a couple of cases where no error is reported + for _, tc := range []struct{ errorTagVal interface{} }{ + {false}, + {nil}, + } { + r := reporter.SetTestReporter() // set up test reporter + tr := NewTracer() + + span := tr.StartSpan("op") + assert.NotNil(t, span) + span.SetTag("error", tc.errorTagVal) + span.Finish() + + r.Close(2) + g.AssertGraph(t, r.EventBufs, 2, g.AssertNodeMap{ + {"op", "entry"}: {}, + {"op", "exit"}: {Edges: g.Edges{{"op", "entry"}}, Callback: func(n g.Node) {}}, + }) + } }