From 10729d8ee8c2599baa52219fa28c9c4a8955ab4c Mon Sep 17 00:00:00 2001 From: Daniel Redondo Date: Thu, 4 Jun 2020 12:26:38 +0200 Subject: [PATCH 1/7] avoiding complex objects --- agent/recorder.go | 18 +++++++-------- instrumentation/gocheck/instrumentation.go | 7 +++++- instrumentation/grpc/client.go | 3 +++ instrumentation/grpc/errors.go | 4 ++-- instrumentation/nethttp/client.go | 5 +++- instrumentation/process/cmd.go | 7 +++++- instrumentation/sql/driver.go | 2 +- instrumentation/testing/benchmark.go | 5 ++-- instrumentation/testing/testing.go | 6 ++++- tags/tags.go | 27 ++++++++++++++++++++++ tracer/span.go | 15 +++++++++++- 11 files changed, 80 insertions(+), 19 deletions(-) diff --git a/agent/recorder.go b/agent/recorder.go index 5d994aad..2815384c 100644 --- a/agent/recorder.go +++ b/agent/recorder.go @@ -324,10 +324,12 @@ func (r *SpanRecorder) getPayloadComponents(span tracer.RawSpan) (PayloadSpan, [ if span.ParentSpanID != 0 { parentSpanID = fmt.Sprintf("%x", span.ParentSpanID) } + traceId := tracer.UUIDToString(span.Context.TraceID) + spanId := fmt.Sprintf("%x", span.Context.SpanID) payloadSpan := PayloadSpan{ "context": map[string]interface{}{ - "trace_id": tracer.UUIDToString(span.Context.TraceID), - "span_id": fmt.Sprintf("%x", span.Context.SpanID), + "trace_id": traceId, + "span_id": spanId, "baggage": span.Context.Baggage, }, "parent_span_id": parentSpanID, @@ -339,16 +341,14 @@ func (r *SpanRecorder) getPayloadComponents(span tracer.RawSpan) (PayloadSpan, [ for _, event := range span.Logs { var fields = make(map[string]interface{}) for _, field := range event.Fields { - fields[field.Key()] = field.Value() - } - eventId, err := uuid.NewRandom() - if err != nil { - panic(err) + value := field.Value() + fields[field.Key()] = value } + eventId := uuid.New() events = append(events, PayloadEvent{ "context": map[string]interface{}{ - "trace_id": tracer.UUIDToString(span.Context.TraceID), - "span_id": fmt.Sprintf("%x", span.Context.SpanID), + "trace_id": traceId, + "span_id": spanId, "event_id": eventId.String(), }, "timestamp": r.applyNTPOffset(event.Timestamp).Format(time.RFC3339Nano), diff --git a/instrumentation/gocheck/instrumentation.go b/instrumentation/gocheck/instrumentation.go index 11b4c17c..db87e875 100644 --- a/instrumentation/gocheck/instrumentation.go +++ b/instrumentation/gocheck/instrumentation.go @@ -15,6 +15,7 @@ import ( "go.undefinedlabs.com/scopeagent/instrumentation/coverage" "go.undefinedlabs.com/scopeagent/instrumentation/logging" "go.undefinedlabs.com/scopeagent/tags" + scopetracer "go.undefinedlabs.com/scopeagent/tracer" chk "gopkg.in/check.v1" ) @@ -103,7 +104,11 @@ func (test *Test) end(c *chk.C) { if testing.CoverMode() != "" { if cov := coverage.EndCoverage(); cov != nil { - test.span.SetTag(tags.Coverage, *cov) + if span, ok := test.span.(scopetracer.Span); ok { + span.UnsafeSetTag(tags.Coverage, *cov) + } else { + test.span.SetTag(tags.Coverage, *cov) + } } } diff --git a/instrumentation/grpc/client.go b/instrumentation/grpc/client.go index f18452a7..e9472091 100644 --- a/instrumentation/grpc/client.go +++ b/instrumentation/grpc/client.go @@ -5,6 +5,7 @@ import ( "github.com/opentracing/opentracing-go/ext" "github.com/opentracing/opentracing-go/log" "go.undefinedlabs.com/scopeagent/instrumentation" + scopetracer "go.undefinedlabs.com/scopeagent/tracer" "golang.org/x/net/context" "google.golang.org/grpc" "google.golang.org/grpc/metadata" @@ -214,6 +215,8 @@ func (cs *openTracingClientStream) Header() (metadata.MD, error) { md, err := cs.ClientStream.Header() if err != nil { cs.finishFunc(err) + } else if span, ok := cs.span.(scopetracer.Span); ok { + span.UnsafeSetTag(Headers, md) } else { cs.span.SetTag(Headers, md) } diff --git a/instrumentation/grpc/errors.go b/instrumentation/grpc/errors.go index 8760ae82..d589379d 100644 --- a/instrumentation/grpc/errors.go +++ b/instrumentation/grpc/errors.go @@ -88,12 +88,12 @@ func SetSpanTags(span opentracing.Span, err error, client bool) { if value, ok := codeStrings[code]; ok { span.SetTag(Status, value) } else { - span.SetTag(Status, code) + span.SetTag(Status, uint32(code)) } if value, ok := classStrings[c]; ok { span.SetTag("response_class", value) } else { - span.SetTag("response_class", c) + span.SetTag("response_class", string(c)) } if err == nil { diff --git a/instrumentation/nethttp/client.go b/instrumentation/nethttp/client.go index 49b49a93..c8636dc6 100644 --- a/instrumentation/nethttp/client.go +++ b/instrumentation/nethttp/client.go @@ -16,6 +16,7 @@ import ( scopeerrors "go.undefinedlabs.com/scopeagent/errors" "go.undefinedlabs.com/scopeagent/instrumentation" + scopetracer "go.undefinedlabs.com/scopeagent/tracer" ) type contextKey int @@ -183,7 +184,9 @@ func (t *Transport) doRoundTrip(req *http.Request) (*http.Response, error) { tracer.start(req) if t.Stacktrace { - tracer.sp.SetTag("stacktrace", scopeerrors.GetCurrentStackTrace(2)) + if span, ok := tracer.sp.(scopetracer.Span); ok { + span.UnsafeSetTag("stacktrace", scopeerrors.GetCurrentStackTrace(2)) + } } ext.HTTPMethod.Set(tracer.sp, req.Method) diff --git a/instrumentation/process/cmd.go b/instrumentation/process/cmd.go index 4710e200..f2603eef 100644 --- a/instrumentation/process/cmd.go +++ b/instrumentation/process/cmd.go @@ -9,6 +9,7 @@ import ( "github.com/opentracing/opentracing-go" "go.undefinedlabs.com/scopeagent/instrumentation" + scopetracer "go.undefinedlabs.com/scopeagent/tracer" ) // Injects the span context to the command environment variables @@ -27,7 +28,11 @@ func InjectToCmd(ctx context.Context, command *exec.Cmd) *exec.Cmd { func InjectToCmdWithSpan(ctx context.Context, command *exec.Cmd) (opentracing.Span, context.Context) { innerSpan, innerCtx := opentracing.StartSpanFromContextWithTracer(ctx, instrumentation.Tracer(), "Exec: "+getOperationNameFromArgs(command.Args)) - innerSpan.SetTag("Args", command.Args) + if sp, ok := innerSpan.(scopetracer.Span); ok { + sp.UnsafeSetTag("Args", command.Args) + } else { + innerSpan.SetTag("Args", command.Args) + } innerSpan.SetTag("Path", command.Path) innerSpan.SetTag("Dir", command.Dir) InjectToCmd(innerCtx, command) diff --git a/instrumentation/sql/driver.go b/instrumentation/sql/driver.go index 5b235f22..3c5a6fdc 100644 --- a/instrumentation/sql/driver.go +++ b/instrumentation/sql/driver.go @@ -146,7 +146,7 @@ func (t *driverConfiguration) newSpan(operationName string, query string, args [ } dbParams[name] = map[string]interface{}{ "type": reflect.TypeOf(item.Value).String(), - "value": item.Value, + "value": fmt.Sprint(item.Value), } } opts = append(opts, opentracing.Tags{ diff --git a/instrumentation/testing/benchmark.go b/instrumentation/testing/benchmark.go index aac0e025..8017a649 100644 --- a/instrumentation/testing/benchmark.go +++ b/instrumentation/testing/benchmark.go @@ -2,6 +2,7 @@ package testing import ( "context" + "go.undefinedlabs.com/scopeagent/tags" "math" "regexp" "runtime" @@ -141,9 +142,9 @@ func startBenchmark(b *testing.B, pc uintptr, benchFunc func(b *testing.B)) { span.SetTag("benchmark.memory.mean_allocations", results.AllocsPerOp()) span.SetTag("benchmark.memory.mean_bytes_allocations", results.AllocedBytesPerOp()) if result { - span.SetTag("test.status", "PASS") + span.SetTag("test.status", tags.TestStatus_PASS) } else { - span.SetTag("test.status", "FAIL") + span.SetTag("test.status", tags.TestStatus_FAIL) } span.FinishWithOptions(opentracing.FinishOptions{ FinishTime: startTime.Add(results.T), diff --git a/instrumentation/testing/testing.go b/instrumentation/testing/testing.go index 0383c3cd..5016b477 100644 --- a/instrumentation/testing/testing.go +++ b/instrumentation/testing/testing.go @@ -214,7 +214,11 @@ func (test *Test) end() { instrumentation.Logger().Printf("CodePath in parallel test is not supported: %v\n", test.t.Name()) coverage.RestoreCoverageCounters() } else if cov := coverage.EndCoverage(); cov != nil { - test.span.SetTag(tags.Coverage, *cov) + if sp, ok := test.span.(tracer.Span); ok { + sp.UnsafeSetTag(tags.Coverage, *cov) + } else { + test.span.SetTag(tags.Coverage, *cov) + } } } diff --git a/tags/tags.go b/tags/tags.go index bc071578..881d71a2 100644 --- a/tags/tags.go +++ b/tags/tags.go @@ -1,5 +1,10 @@ package tags +import ( + "fmt" + "reflect" +) + const ( AgentType = "agent.type" AgentID = "agent.id" @@ -65,3 +70,25 @@ const ( Coverage = "test.coverage" ) + + func GetValidStringValue(value interface{}) (interface{}, bool) { + if value == nil { + return nil, false + } + if vs, ok := value.(fmt.Stringer); ok { + return vs.String(), true + } + rValue := reflect.ValueOf(value) + for { + rKind := rValue.Kind() + if rKind == reflect.Ptr { + rValue = rValue.Elem() + continue + } + if (rKind < 1 || rKind > 16) && rKind != reflect.String { + return fmt.Sprint(value), true + } + break + } + return value, false + } \ No newline at end of file diff --git a/tracer/span.go b/tracer/span.go index 29e81726..2b985043 100644 --- a/tracer/span.go +++ b/tracer/span.go @@ -1,6 +1,8 @@ package tracer import ( + "go.undefinedlabs.com/scopeagent/instrumentation" + "go.undefinedlabs.com/scopeagent/tags" "sync" "time" @@ -28,6 +30,9 @@ type Span interface { // Log fields with timestamp LogFieldsWithTimestamp(t time.Time, fields ...log.Field) + + // Set tag without validation + UnsafeSetTag(key string, value interface{}) opentracing.Span } // Implements the `Span` interface. Created via tracerImpl (see @@ -85,7 +90,7 @@ func (s *spanImpl) SetStart(start time.Time) opentracing.Span { return s } -func (s *spanImpl) SetTag(key string, value interface{}) opentracing.Span { +func (s *spanImpl) UnsafeSetTag(key string, value interface{}) opentracing.Span { defer s.onTag(key, value) s.Lock() defer s.Unlock() @@ -106,6 +111,14 @@ func (s *spanImpl) SetTag(key string, value interface{}) opentracing.Span { return s } +func (s *spanImpl) SetTag(key string, value interface{}) opentracing.Span { + cValue, c := tags.GetValidStringValue(value) + if c { + instrumentation.Logger().Printf("SetTag-ConvertedValue: %v", cValue) + } + return s.UnsafeSetTag(key, cValue) +} + func (s *spanImpl) LogKV(keyValues ...interface{}) { fields, err := log.InterleavedKVToFields(keyValues...) if err != nil { From 5f80aa766fb0bdf29ee2f80e7233e8024f691de7 Mon Sep 17 00:00:00 2001 From: Daniel Redondo Date: Thu, 4 Jun 2020 13:16:35 +0200 Subject: [PATCH 2/7] build fix --- tracer/span.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/tracer/span.go b/tracer/span.go index 2b985043..0430ab23 100644 --- a/tracer/span.go +++ b/tracer/span.go @@ -1,8 +1,6 @@ package tracer import ( - "go.undefinedlabs.com/scopeagent/instrumentation" - "go.undefinedlabs.com/scopeagent/tags" "sync" "time" @@ -11,6 +9,9 @@ import ( "github.com/opentracing/opentracing-go" "github.com/opentracing/opentracing-go/ext" "github.com/opentracing/opentracing-go/log" + + "go.undefinedlabs.com/scopeagent/instrumentation" + scopetags "go.undefinedlabs.com/scopeagent/tags" ) // Span provides access to the essential details of the span, for use @@ -112,7 +113,7 @@ func (s *spanImpl) UnsafeSetTag(key string, value interface{}) opentracing.Span } func (s *spanImpl) SetTag(key string, value interface{}) opentracing.Span { - cValue, c := tags.GetValidStringValue(value) + cValue, c := scopetags.GetValidStringValue(value) if c { instrumentation.Logger().Printf("SetTag-ConvertedValue: %v", cValue) } From 1ffd8a69e9c5660ca885b8cb77ff57616e4049df Mon Sep 17 00:00:00 2001 From: Daniel Redondo Date: Thu, 4 Jun 2020 13:36:23 +0200 Subject: [PATCH 3/7] exclude branch from ITR --- .github/workflows/go.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/go.yml b/.github/workflows/go.yml index 1079f0ff..2823f6ad 100644 --- a/.github/workflows/go.yml +++ b/.github/workflows/go.yml @@ -31,7 +31,7 @@ jobs: SCOPE_LOGGER_ROOT: /home/runner/.scope-results SCOPE_DEBUG: true SCOPE_RUNNER_ENABLED: true - SCOPE_RUNNER_EXCLUDE_BRANCHES: master + SCOPE_RUNNER_EXCLUDE_BRANCHES: master, complex-objects SCOPE_TESTING_FAIL_RETRIES: 3 SCOPE_TESTING_PANIC_AS_FAIL: true From 6eb651383e28ba16b8a527d8b942eb14d1b7781e Mon Sep 17 00:00:00 2001 From: Daniel Redondo Date: Thu, 4 Jun 2020 13:54:45 +0200 Subject: [PATCH 4/7] remove duplicate tags --- errors/handler.go | 18 +++++------------- tags/tags.go | 42 +++++++++++++++++++++--------------------- 2 files changed, 26 insertions(+), 34 deletions(-) diff --git a/errors/handler.go b/errors/handler.go index 6366e5bb..4b1a76dc 100644 --- a/errors/handler.go +++ b/errors/handler.go @@ -16,14 +16,6 @@ import ( "go.undefinedlabs.com/scopeagent/tracer" ) -const ( - EventType = "event" - EventSource = "source" - EventMessage = "message" - EventStack = "stack" - EventException = "exception" -) - type StackFrames struct { File string LineNumber int @@ -136,11 +128,11 @@ func getExceptionLogFields(eventType string, recoverData interface{}, skipFrames } fields := make([]log.Field, 5) - fields[0] = log.String(EventType, eventType) - fields[1] = log.String(EventSource, source) - fields[2] = log.String(EventMessage, errMessage) - fields[3] = log.String(EventStack, getStringStack(err, errStack)) - fields[4] = log.Object(EventException, exceptionData) + fields[0] = log.String(tags.EventType, eventType) + fields[1] = log.String(tags.EventSource, source) + fields[2] = log.String(tags.EventMessage, errMessage) + fields[3] = log.String(tags.EventStack, getStringStack(err, errStack)) + fields[4] = log.Object(tags.EventException, exceptionData) return fields } return nil diff --git a/tags/tags.go b/tags/tags.go index 881d71a2..48c67dbe 100644 --- a/tags/tags.go +++ b/tags/tags.go @@ -71,24 +71,24 @@ const ( Coverage = "test.coverage" ) - func GetValidStringValue(value interface{}) (interface{}, bool) { - if value == nil { - return nil, false - } - if vs, ok := value.(fmt.Stringer); ok { - return vs.String(), true - } - rValue := reflect.ValueOf(value) - for { - rKind := rValue.Kind() - if rKind == reflect.Ptr { - rValue = rValue.Elem() - continue - } - if (rKind < 1 || rKind > 16) && rKind != reflect.String { - return fmt.Sprint(value), true - } - break - } - return value, false - } \ No newline at end of file +func GetValidStringValue(value interface{}) (interface{}, bool) { + if value == nil { + return nil, false + } + if vs, ok := value.(fmt.Stringer); ok { + return vs.String(), true + } + rValue := reflect.ValueOf(value) + for { + rKind := rValue.Kind() + if rKind == reflect.Ptr { + rValue = rValue.Elem() + continue + } + if (rKind < 1 || rKind > 16) && rKind != reflect.String { + return fmt.Sprint(value), true + } + break + } + return value, false +} From 4586a860aa3758ce6b022a6b2e83c08dc3c8662e Mon Sep 17 00:00:00 2001 From: Daniel Redondo Date: Thu, 4 Jun 2020 13:59:19 +0200 Subject: [PATCH 5/7] func name change --- tags/tags.go | 2 +- tracer/span.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tags/tags.go b/tags/tags.go index 48c67dbe..d670d180 100644 --- a/tags/tags.go +++ b/tags/tags.go @@ -71,7 +71,7 @@ const ( Coverage = "test.coverage" ) -func GetValidStringValue(value interface{}) (interface{}, bool) { +func GetValidValue(value interface{}) (interface{}, bool) { if value == nil { return nil, false } diff --git a/tracer/span.go b/tracer/span.go index 0430ab23..63c1379b 100644 --- a/tracer/span.go +++ b/tracer/span.go @@ -113,7 +113,7 @@ func (s *spanImpl) UnsafeSetTag(key string, value interface{}) opentracing.Span } func (s *spanImpl) SetTag(key string, value interface{}) opentracing.Span { - cValue, c := scopetags.GetValidStringValue(value) + cValue, c := scopetags.GetValidValue(value) if c { instrumentation.Logger().Printf("SetTag-ConvertedValue: %v", cValue) } From 0ca8df5c134ff2f6dd22b528867208b928c287b3 Mon Sep 17 00:00:00 2001 From: Daniel Redondo Date: Fri, 5 Jun 2020 17:26:51 +0200 Subject: [PATCH 6/7] remove branch from exclude branches --- .github/workflows/go.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/go.yml b/.github/workflows/go.yml index 2823f6ad..1079f0ff 100644 --- a/.github/workflows/go.yml +++ b/.github/workflows/go.yml @@ -31,7 +31,7 @@ jobs: SCOPE_LOGGER_ROOT: /home/runner/.scope-results SCOPE_DEBUG: true SCOPE_RUNNER_ENABLED: true - SCOPE_RUNNER_EXCLUDE_BRANCHES: master, complex-objects + SCOPE_RUNNER_EXCLUDE_BRANCHES: master SCOPE_TESTING_FAIL_RETRIES: 3 SCOPE_TESTING_PANIC_AS_FAIL: true From 52d91d409154a848e972b48236d5349ada62f204 Mon Sep 17 00:00:00 2001 From: Daniel Redondo Date: Fri, 5 Jun 2020 17:28:22 +0200 Subject: [PATCH 7/7] stacktrace fallback in case other tracer is in use --- instrumentation/nethttp/client.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/instrumentation/nethttp/client.go b/instrumentation/nethttp/client.go index c8636dc6..f1f05083 100644 --- a/instrumentation/nethttp/client.go +++ b/instrumentation/nethttp/client.go @@ -186,6 +186,8 @@ func (t *Transport) doRoundTrip(req *http.Request) (*http.Response, error) { if t.Stacktrace { if span, ok := tracer.sp.(scopetracer.Span); ok { span.UnsafeSetTag("stacktrace", scopeerrors.GetCurrentStackTrace(2)) + } else { + tracer.sp.SetTag("stacktrace", scopeerrors.GetCurrentStackTrace(2)) } }