Merge branch 'main' into fix-net-http-host

Author: wu-sheng

Diff for: 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
 * Fix http plugin cannot provide peer name when optional Host is empty.
 
 #### Issues and PR

Diff for: 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 for: 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 for: 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 for: 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 for: 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 for: 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 for: 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 for: plugins/http/client_intercepter.go

@@ -55,6 +55,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 for: plugins/http/client_intercepter_test.go

@@ -69,3 +69,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 for: 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 for: 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 for: 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 for: 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 for: 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 for: 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: []

Diff for: 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 for: toolkit/trace/api.go

@@ -86,3 +86,6 @@ func SetCorrelation(key string, value string) {
 
 func SetComponent(componentID int32) {
 }
+
+func Error(...string) {
+}