From ef154abe2de22d549e74ee9b0b12418e335feb22 Mon Sep 17 00:00:00 2001 From: alexwn <67748901+alexwn@users.noreply.github.com> Date: Wed, 25 Dec 2024 10:34:12 +0800 Subject: [PATCH] Support span in error status when http code >= 400, and add trace#Error in toolkit API (#212) --- CHANGES.md | 2 + plugins/core/span_default.go | 8 ++++ plugins/core/span_noop.go | 3 ++ plugins/core/span_tracing.go | 4 ++ plugins/core/tracing/api.go | 2 + plugins/core/tracing/bridge.go | 5 +++ plugins/core/tracing/span.go | 2 + plugins/gin/intercepter.go | 3 ++ plugins/http/client_intercepter.go | 3 ++ plugins/http/client_intercepter_test.go | 24 ++++++++++++ plugins/http/server_intercepter.go | 3 ++ plugins/http/server_intercepter_test.go | 27 +++++++++++++ plugins/toolkit-activation/instrument.go | 4 ++ .../trace/error_intercepter.go | 39 +++++++++++++++++++ .../plugins/scenarios/gin/config/excepted.yml | 4 +- .../scenarios/http/config/excepted.yml | 37 ++++++++++++++++++ test/plugins/scenarios/http/main.go | 21 ++++++++++ toolkit/trace/api.go | 3 ++ 18 files changed, 192 insertions(+), 2 deletions(-) create mode 100644 plugins/toolkit-activation/trace/error_intercepter.go diff --git a/CHANGES.md b/CHANGES.md index 7a545b06..8eab93ff 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -9,6 +9,7 @@ Release Notes. * support attaching events to span in the toolkit. * support record log in the toolkit. * support manually report metrics in the toolkit. +* support manually set span error in the toolkit. #### Plugins * Support [goframev2](https://github.com/gogf/gf) goframev2. @@ -22,6 +23,7 @@ Release Notes. * Fix wrong docker image name and `-version` command. * Fix redis plugin cannot work in cluster mode. * Fix cannot find file when exec build in test/plugins. +* Fix not set span error when http status code >= 400 #### Issues and PR - All issues are [here](https://github.com/apache/skywalking/milestone/219?closed=1) diff --git a/plugins/core/span_default.go b/plugins/core/span_default.go index 0e53a178..e3c2becc 100644 --- a/plugins/core/span_default.go +++ b/plugins/core/span_default.go @@ -148,6 +148,14 @@ func (ds *DefaultSpan) Error(ll ...string) { ds.Log(ll...) } +func (ds *DefaultSpan) ErrorOccured() { + if ds.InAsyncMode { + ds.AsyncOpLocker.Lock() + defer ds.AsyncOpLocker.Unlock() + } + ds.IsError = true +} + func (ds *DefaultSpan) End(changeParent bool) { ds.EndTime = time.Now() if changeParent { diff --git a/plugins/core/span_noop.go b/plugins/core/span_noop.go index 4fc0beb3..27612874 100644 --- a/plugins/core/span_noop.go +++ b/plugins/core/span_noop.go @@ -89,6 +89,9 @@ func (*NoopSpan) Log(...string) { func (*NoopSpan) Error(...string) { } +func (*NoopSpan) ErrorOccured() { +} + func (n *NoopSpan) enterNoSpan() { n.stackCount++ } diff --git a/plugins/core/span_tracing.go b/plugins/core/span_tracing.go index 978756a2..4a4b6aca 100644 --- a/plugins/core/span_tracing.go +++ b/plugins/core/span_tracing.go @@ -312,6 +312,10 @@ func (s *SnapshotSpan) Error(_ ...string) { panic(fmt.Errorf("cannot add error of span in other goroutine")) } +func (s *SnapshotSpan) ErrorOccured() { + panic(fmt.Errorf("cannot add error of span in other goroutine")) +} + func (s *SnapshotSpan) GetSegmentContext() SegmentContext { return s.SegmentContext } diff --git a/plugins/core/tracing/api.go b/plugins/core/tracing/api.go index 55692a3d..83e2a06e 100644 --- a/plugins/core/tracing/api.go +++ b/plugins/core/tracing/api.go @@ -232,6 +232,8 @@ func (n *NoopSpan) Log(...string) { } func (n *NoopSpan) Error(...string) { } +func (n *NoopSpan) ErrorOccured() { +} func (n *NoopSpan) End() { } func (n *NoopSpan) PrepareAsync() { diff --git a/plugins/core/tracing/bridge.go b/plugins/core/tracing/bridge.go index 1be446b1..8b40b730 100644 --- a/plugins/core/tracing/bridge.go +++ b/plugins/core/tracing/bridge.go @@ -38,6 +38,7 @@ type AdaptSpan interface { Tag(string, string) Log(...string) Error(...string) + ErrorOccured() End() } @@ -89,6 +90,10 @@ func (s *SpanWrapper) Error(v ...string) { s.Span.Error(v...) } +func (s *SpanWrapper) ErrorOccured() { + s.Span.ErrorOccured() +} + func (s *SpanWrapper) End() { s.Span.End() } diff --git a/plugins/core/tracing/span.go b/plugins/core/tracing/span.go index d21c968a..641dcd75 100644 --- a/plugins/core/tracing/span.go +++ b/plugins/core/tracing/span.go @@ -137,6 +137,8 @@ type Span interface { Log(...string) // Error add error log to the Span Error(...string) + // Error and no log to the Span + ErrorOccured() // End end the Span End() } diff --git a/plugins/gin/intercepter.go b/plugins/gin/intercepter.go index 754dd8bc..abbf1e1c 100644 --- a/plugins/gin/intercepter.go +++ b/plugins/gin/intercepter.go @@ -67,6 +67,9 @@ func (h *ContextInterceptor) AfterInvoke(invocation operator.Invocation, result context := invocation.CallerInstance().(*gin.Context) span := invocation.GetContext().(tracing.Span) span.Tag(tracing.TagStatusCode, fmt.Sprintf("%d", context.Writer.Status())) + if context.Writer.Status() >= 400 { + span.ErrorOccured() + } if len(context.Errors) > 0 { span.Error(context.Errors.String()) } diff --git a/plugins/http/client_intercepter.go b/plugins/http/client_intercepter.go index 482a7783..fd28851d 100644 --- a/plugins/http/client_intercepter.go +++ b/plugins/http/client_intercepter.go @@ -51,6 +51,9 @@ func (h *ClientInterceptor) AfterInvoke(invocation operator.Invocation, result . span := invocation.GetContext().(tracing.Span) if resp, ok := result[0].(*http.Response); ok && resp != nil { span.Tag(tracing.TagStatusCode, fmt.Sprintf("%d", resp.StatusCode)) + if resp.StatusCode >= 400 { + span.ErrorOccured() + } } if err, ok := result[1].(error); ok && err != nil { span.Error(err.Error()) diff --git a/plugins/http/client_intercepter_test.go b/plugins/http/client_intercepter_test.go index 51e9c8e8..eaf812a8 100644 --- a/plugins/http/client_intercepter_test.go +++ b/plugins/http/client_intercepter_test.go @@ -57,3 +57,27 @@ func TestClientInvoke(t *testing.T) { assert.Nil(t, spans[0].Refs(), "refs should be nil") assert.Greater(t, spans[0].EndTime(), spans[0].StartTime(), "end time should be greater than start time") } + +func TestClientInvokeError(t *testing.T) { + defer core.ResetTracingContext() + interceptor := &ClientInterceptor{} + request, err := http.NewRequest("GET", "http://localhost/", http.NoBody) + assert.Nil(t, err, "new request error should be nil") + invocation := operator.NewInvocation(nil, request) + err = interceptor.BeforeInvoke(invocation) + assert.Nil(t, err, "before invoke error should be nil") + assert.NotNil(t, invocation.GetContext(), "context should not be nil") + + time.Sleep(100 * time.Millisecond) + + err = interceptor.AfterInvoke(invocation, &http.Response{ + StatusCode: 500, + }, nil) + assert.Nil(t, err, "after invoke error should be nil") + + time.Sleep(100 * time.Millisecond) + spans := core.GetReportedSpans() + assert.NotNil(t, spans, "spans should not be nil") + assert.Equal(t, 1, len(spans), "spans length should be 1") + assert.True(t, spans[0].IsError(), "span should be error") +} diff --git a/plugins/http/server_intercepter.go b/plugins/http/server_intercepter.go index 912f3c95..a20dfac1 100644 --- a/plugins/http/server_intercepter.go +++ b/plugins/http/server_intercepter.go @@ -59,6 +59,9 @@ func (h *ServerInterceptor) AfterInvoke(invocation operator.Invocation, result . span := invocation.GetContext().(tracing.Span) if wrapped, ok := invocation.Args()[0].(*writerWrapper); ok { span.Tag(tracing.TagStatusCode, fmt.Sprintf("%d", wrapped.statusCode)) + if wrapped.statusCode >= 400 { + span.ErrorOccured() + } } span.End() return nil diff --git a/plugins/http/server_intercepter_test.go b/plugins/http/server_intercepter_test.go index 9e5cde1c..c08c73a7 100644 --- a/plugins/http/server_intercepter_test.go +++ b/plugins/http/server_intercepter_test.go @@ -55,6 +55,26 @@ func TestServerInvoke(t *testing.T) { assert.Greater(t, spans[0].EndTime(), spans[0].StartTime(), "end time should be greater than start time") } +func TestServerInvokeError(t *testing.T) { + defer core.ResetTracingContext() + interceptor := &ServerInterceptor{} + request, _ := http.NewRequest("GET", "http://localhost/", http.NoBody) + responseWriter := &testResponseWriter{} + invocation := operator.NewInvocation(nil, responseWriter, request) + _ = interceptor.BeforeInvoke(invocation) + + wrapped, _ := invocation.Args()[0].(*writerWrapper) + + wrapped.WriteHeader(http.StatusInternalServerError) + + time.Sleep(100 * time.Millisecond) + _ = interceptor.AfterInvoke(invocation) + time.Sleep(100 * time.Millisecond) + + spans := core.GetReportedSpans() + assert.True(t, spans[0].IsError(), "span should be error") +} + type testResponseWriter struct { http.ResponseWriter } @@ -71,3 +91,10 @@ func (t *testResponseWriter) Hijack() (net.Conn, *bufio.ReadWriter, error) { } return dial, nil, nil } + +func (t *testResponseWriter) WriteHeader(code int) { +} + +func (t *testResponseWriter) Write(b []byte) (int, error) { + return len(b), nil +} diff --git a/plugins/toolkit-activation/instrument.go b/plugins/toolkit-activation/instrument.go index 105ed78f..23a09dac 100644 --- a/plugins/toolkit-activation/instrument.go +++ b/plugins/toolkit-activation/instrument.go @@ -203,6 +203,10 @@ func tracePoint() []*instrument.Point { PackagePath: "trace", At: instrument.NewStaticMethodEnhance("SetComponent"), Interceptor: "SetComponentInterceptor", }, + { + PackagePath: "trace", At: instrument.NewStaticMethodEnhance("Error"), + Interceptor: "ErrorIntercepter", + }, } } diff --git a/plugins/toolkit-activation/trace/error_intercepter.go b/plugins/toolkit-activation/trace/error_intercepter.go new file mode 100644 index 00000000..947a8afe --- /dev/null +++ b/plugins/toolkit-activation/trace/error_intercepter.go @@ -0,0 +1,39 @@ +// Licensed to Apache Software Foundation (ASF) under one or more contributor +// license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright +// ownership. Apache Software Foundation (ASF) licenses this file to you under +// the Apache License, Version 2.0 (the "License"); you may +// not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package traceactivation + +import ( + "github.com/apache/skywalking-go/plugins/core/operator" + "github.com/apache/skywalking-go/plugins/core/tracing" +) + +type ErrorIntercepter struct { +} + +func (h *ErrorIntercepter) BeforeInvoke(invocation operator.Invocation) error { + span := tracing.ActiveSpan() + if span != nil { + args := invocation.Args()[0].([]string) + span.Error(args...) + } + return nil +} + +func (h *ErrorIntercepter) AfterInvoke(_ operator.Invocation, _ ...interface{}) error { + return nil +} diff --git a/test/plugins/scenarios/gin/config/excepted.yml b/test/plugins/scenarios/gin/config/excepted.yml index e2a3e36e..a9619d51 100644 --- a/test/plugins/scenarios/gin/config/excepted.yml +++ b/test/plugins/scenarios/gin/config/excepted.yml @@ -49,7 +49,7 @@ segmentItems: startTime: nq 0 endTime: nq 0 componentId: 5006 - isError: false + isError: true spanType: Entry peer: '' skipAnalysis: false @@ -85,7 +85,7 @@ segmentItems: startTime: gt 0 endTime: gt 0 componentId: 5005 - isError: false + isError: true spanType: Exit peer: localhost:8080 skipAnalysis: false diff --git a/test/plugins/scenarios/http/config/excepted.yml b/test/plugins/scenarios/http/config/excepted.yml index 2569a5fb..122bc538 100644 --- a/test/plugins/scenarios/http/config/excepted.yml +++ b/test/plugins/scenarios/http/config/excepted.yml @@ -57,6 +57,21 @@ segmentItems: - {key: http.method, value: GET} - {key: url, value: 'localhost:8080/provider'} - {key: status_code, value: '200'} + - operationName: GET:/errortest + parentSpanId: 0 + spanId: 2 + spanLayer: Http + startTime: gt 0 + endTime: gt 0 + componentId: 5005 + isError: true + spanType: Exit + peer: localhost:8080 + skipAnalysis: false + tags: + - {key: http.method, value: GET} + - {key: url, value: 'localhost:8080/errortest'} + - {key: status_code, value: '500'} - operationName: GET:/consumer parentSpanId: -1 spanId: 0 @@ -73,5 +88,27 @@ segmentItems: - {key: url, value: 'service:8080/consumer'} - {key: http.params, value: ''} - {key: status_code, value: '200'} + - segmentId: not null + spans: + - operationName: GET:/errortest + parentSpanId: -1 + spanId: 0 + spanLayer: Http + startTime: nq 0 + endTime: nq 0 + componentId: 5004 + isError: true + spanType: Entry + peer: '' + skipAnalysis: false + tags: + - {key: http.method, value: GET} + - {key: url, value: 'localhost:8080/errortest'} + - {key: http.params, value: ''} + - {key: status_code, value: '500'} + refs: + - {parentEndpoint: 'GET:/consumer', networkAddress: 'localhost:8080', refType: CrossProcess, + parentSpanId: 2, parentTraceSegmentId: not null, parentServiceInstance: not null, + parentService: http, traceId: not null} meterItems: [] logItems: [] \ No newline at end of file diff --git a/test/plugins/scenarios/http/main.go b/test/plugins/scenarios/http/main.go index b59ef377..99da9b8e 100644 --- a/test/plugins/scenarios/http/main.go +++ b/test/plugins/scenarios/http/main.go @@ -44,11 +44,32 @@ func consumerHandler(w http.ResponseWriter, r *http.Request) { return } _, _ = w.Write(body) + + resp, err = http.Get("http://localhost:8080/errortest") + if err != nil { + log.Printf("request errortest error: %v", err) + w.WriteHeader(http.StatusInternalServerError) + return + } + defer resp.Body.Close() + body, err = io.ReadAll(resp.Body) + if err != nil { + log.Print(err) + w.WriteHeader(http.StatusInternalServerError) + return + } + _, _ = w.Write(body) +} + +func errorHandler(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + w.Write([]byte("internal server error")) } func main() { http.HandleFunc("/provider", providerHandler) http.HandleFunc("/consumer", consumerHandler) + http.HandleFunc("/errortest", errorHandler) http.HandleFunc("/health", func(writer http.ResponseWriter, request *http.Request) { writer.WriteHeader(http.StatusOK) diff --git a/toolkit/trace/api.go b/toolkit/trace/api.go index b892359a..c701fe80 100644 --- a/toolkit/trace/api.go +++ b/toolkit/trace/api.go @@ -86,3 +86,6 @@ func SetCorrelation(key string, value string) { func SetComponent(componentID int32) { } + +func Error(...string) { +}