From 915244a661586807e56d6f556299e6043919b629 Mon Sep 17 00:00:00 2001 From: csuzhang <48237831+zhyChesterCheung@users.noreply.github.com> Date: Wed, 24 Nov 2021 01:13:47 +0800 Subject: [PATCH] add new function to exclude span kind 4xx for status code (#2339) * add new function to exclude span kind 4xx for status code * add changelog * make precommit * add unit test * feat: redo by reusing validateHTTPStatusCode * feat: make precommit * feat: move the judgement to SpanStatusFromHTTPStatusCodeAndSpanKind * feat: increase unittest coverage * feat: fix logic * feat: fix unittest * fix code style * Update CHANGELOG.md Co-authored-by: Tyler Yahn --- CHANGELOG.md | 1 + semconv/v1.4.0/http.go | 16 ++++++++++++++++ semconv/v1.4.0/http_test.go | 26 ++++++++++++++++++++++++-- semconv/v1.5.0/http.go | 16 ++++++++++++++++ semconv/v1.5.0/http_test.go | 26 ++++++++++++++++++++++++-- semconv/v1.6.1/http.go | 16 ++++++++++++++++ semconv/v1.6.1/http_test.go | 25 +++++++++++++++++++++++-- semconv/v1.7.0/http.go | 17 +++++++++++++++++ semconv/v1.7.0/http_test.go | 26 ++++++++++++++++++++++++-- 9 files changed, 161 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 17278d88f..4896732a6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - The following interface types simply moved from `metric` to `metric/sdkapi`: `Descriptor`, `MeterImpl`, `InstrumentImpl`, `SyncImpl`, `BoundSyncImpl`, `AsyncImpl`, `AsyncRunner`, `AsyncSingleRunner`, and `AsyncBatchRunner` - The following struct types moved and are replaced with type aliases, since they are exposed to the user: `Observation`, `Measurement`. - The No-op implementations of sync and async instruments are no longer exported, new functions `sdkapi.NewNoopAsyncInstrument()` and `sdkapi.NewNoopSyncInstrument()` are provided instead. (#2271) +- Add `SpanStatusFromHTTPStatusCodeAndSpanKind` to all `semconv` packages to return a span status code similar to `SpanStatusFromHTTPStatusCode`, but exclude `4XX` HTTP errors as span errors if the span is of server kind. (#2296) - Update the SDK `BatchSpanProcessor` to export all queued spans when `ForceFlush` is called. (#2080, #2335) - Change `resource.Default` to be evaluated the first time it is called, rather than on import. This allows the caller the option to update `OTEL_RESOURCE_ATTRIBUTES` first, such as with `os.Setenv`. (#2371) diff --git a/semconv/v1.4.0/http.go b/semconv/v1.4.0/http.go index 7340b229e..d9537ff39 100644 --- a/semconv/v1.4.0/http.go +++ b/semconv/v1.4.0/http.go @@ -23,6 +23,7 @@ import ( "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/codes" + "go.opentelemetry.io/otel/trace" ) // HTTP scheme attributes. @@ -269,6 +270,21 @@ func SpanStatusFromHTTPStatusCode(code int) (codes.Code, string) { return spanCode, "" } +// SpanStatusFromHTTPStatusCodeAndSpanKind generates a status code and a message +// as specified by the OpenTelemetry specification for a span. +// Exclude 4xx for SERVER to set the appropriate status. +func SpanStatusFromHTTPStatusCodeAndSpanKind(code int, spanKind trace.SpanKind) (codes.Code, string) { + spanCode, valid := validateHTTPStatusCode(code) + if !valid { + return spanCode, fmt.Sprintf("Invalid HTTP status code %d", code) + } + category := code / 100 + if spanKind == trace.SpanKindServer && category == 4 { + return codes.Unset, "" + } + return spanCode, "" +} + // Validates the HTTP status code and returns corresponding span status code. // If the `code` is not a valid HTTP status code, returns span status Error // and false. diff --git a/semconv/v1.4.0/http_test.go b/semconv/v1.4.0/http_test.go index 945ae3cee..5d707be5d 100644 --- a/semconv/v1.4.0/http_test.go +++ b/semconv/v1.4.0/http_test.go @@ -20,6 +20,8 @@ import ( "strings" "testing" + "go.opentelemetry.io/otel/trace" + "github.com/google/go-cmp/cmp" "github.com/stretchr/testify/assert" @@ -854,7 +856,7 @@ func TestHTTPAttributesFromHTTPStatusCode(t *testing.T) { func TestSpanStatusFromHTTPStatusCode(t *testing.T) { for code := 0; code < 1000; code++ { - expected := getExpectedCodeForHTTPCode(code) + expected := getExpectedCodeForHTTPCode(code, trace.SpanKindClient) got, msg := SpanStatusFromHTTPStatusCode(code) assert.Equalf(t, expected, got, "%s vs %s", expected, got) @@ -867,7 +869,24 @@ func TestSpanStatusFromHTTPStatusCode(t *testing.T) { } } -func getExpectedCodeForHTTPCode(code int) codes.Code { +func TestSpanStatusFromHTTPStatusCodeAndSpanKind(t *testing.T) { + for code := 0; code < 1000; code++ { + expected := getExpectedCodeForHTTPCode(code, trace.SpanKindClient) + got, msg := SpanStatusFromHTTPStatusCodeAndSpanKind(code, trace.SpanKindClient) + assert.Equalf(t, expected, got, "%s vs %s", expected, got) + + _, valid := validateHTTPStatusCode(code) + if !valid { + assert.NotEmpty(t, msg, "message should be set if error cannot be inferred from code") + } else { + assert.Empty(t, msg, "message should not be set if error can be inferred from code") + } + } + code, _ := SpanStatusFromHTTPStatusCodeAndSpanKind(400, trace.SpanKindServer) + assert.Equalf(t, codes.Unset, code, "message should be set if error cannot be inferred from code") +} + +func getExpectedCodeForHTTPCode(code int, spanKind trace.SpanKind) codes.Code { if http.StatusText(code) == "" { return codes.Error } @@ -886,6 +905,9 @@ func getExpectedCodeForHTTPCode(code int) codes.Code { if category > 0 && category < 4 { return codes.Unset } + if spanKind == trace.SpanKindServer && category == 4 { + return codes.Unset + } return codes.Error } diff --git a/semconv/v1.5.0/http.go b/semconv/v1.5.0/http.go index 77719b6ed..1ab89a93a 100644 --- a/semconv/v1.5.0/http.go +++ b/semconv/v1.5.0/http.go @@ -23,6 +23,7 @@ import ( "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/codes" + "go.opentelemetry.io/otel/trace" ) // HTTP scheme attributes. @@ -269,6 +270,21 @@ func SpanStatusFromHTTPStatusCode(code int) (codes.Code, string) { return spanCode, "" } +// SpanStatusFromHTTPStatusCodeAndSpanKind generates a status code and a message +// as specified by the OpenTelemetry specification for a span. +// Exclude 4xx for SERVER to set the appropriate status. +func SpanStatusFromHTTPStatusCodeAndSpanKind(code int, spanKind trace.SpanKind) (codes.Code, string) { + spanCode, valid := validateHTTPStatusCode(code) + if !valid { + return spanCode, fmt.Sprintf("Invalid HTTP status code %d", code) + } + category := code / 100 + if spanKind == trace.SpanKindServer && category == 4 { + return codes.Unset, "" + } + return spanCode, "" +} + // Validates the HTTP status code and returns corresponding span status code. // If the `code` is not a valid HTTP status code, returns span status Error // and false. diff --git a/semconv/v1.5.0/http_test.go b/semconv/v1.5.0/http_test.go index 945ae3cee..af8567e1d 100644 --- a/semconv/v1.5.0/http_test.go +++ b/semconv/v1.5.0/http_test.go @@ -23,6 +23,8 @@ import ( "github.com/google/go-cmp/cmp" "github.com/stretchr/testify/assert" + "go.opentelemetry.io/otel/trace" + "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/codes" ) @@ -854,7 +856,7 @@ func TestHTTPAttributesFromHTTPStatusCode(t *testing.T) { func TestSpanStatusFromHTTPStatusCode(t *testing.T) { for code := 0; code < 1000; code++ { - expected := getExpectedCodeForHTTPCode(code) + expected := getExpectedCodeForHTTPCode(code, trace.SpanKindClient) got, msg := SpanStatusFromHTTPStatusCode(code) assert.Equalf(t, expected, got, "%s vs %s", expected, got) @@ -867,7 +869,24 @@ func TestSpanStatusFromHTTPStatusCode(t *testing.T) { } } -func getExpectedCodeForHTTPCode(code int) codes.Code { +func TestSpanStatusFromHTTPStatusCodeAndSpanKind(t *testing.T) { + for code := 0; code < 1000; code++ { + expected := getExpectedCodeForHTTPCode(code, trace.SpanKindClient) + got, msg := SpanStatusFromHTTPStatusCodeAndSpanKind(code, trace.SpanKindClient) + assert.Equalf(t, expected, got, "%s vs %s", expected, got) + + _, valid := validateHTTPStatusCode(code) + if !valid { + assert.NotEmpty(t, msg, "message should be set if error cannot be inferred from code") + } else { + assert.Empty(t, msg, "message should not be set if error can be inferred from code") + } + } + code, _ := SpanStatusFromHTTPStatusCodeAndSpanKind(400, trace.SpanKindServer) + assert.Equalf(t, codes.Unset, code, "message should be set if error cannot be inferred from code") +} + +func getExpectedCodeForHTTPCode(code int, spanKind trace.SpanKind) codes.Code { if http.StatusText(code) == "" { return codes.Error } @@ -886,6 +905,9 @@ func getExpectedCodeForHTTPCode(code int) codes.Code { if category > 0 && category < 4 { return codes.Unset } + if spanKind == trace.SpanKindServer && category == 4 { + return codes.Unset + } return codes.Error } diff --git a/semconv/v1.6.1/http.go b/semconv/v1.6.1/http.go index 40443d7a9..838e5135b 100644 --- a/semconv/v1.6.1/http.go +++ b/semconv/v1.6.1/http.go @@ -23,6 +23,7 @@ import ( "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/codes" + "go.opentelemetry.io/otel/trace" ) // HTTP scheme attributes. @@ -269,6 +270,21 @@ func SpanStatusFromHTTPStatusCode(code int) (codes.Code, string) { return spanCode, "" } +// SpanStatusFromHTTPStatusCodeAndSpanKind generates a status code and a message +// as specified by the OpenTelemetry specification for a span. +// Exclude 4xx for SERVER to set the appropriate status. +func SpanStatusFromHTTPStatusCodeAndSpanKind(code int, spanKind trace.SpanKind) (codes.Code, string) { + spanCode, valid := validateHTTPStatusCode(code) + if !valid { + return spanCode, fmt.Sprintf("Invalid HTTP status code %d", code) + } + category := code / 100 + if spanKind == trace.SpanKindServer && category == 4 { + return codes.Unset, "" + } + return spanCode, "" +} + // Validates the HTTP status code and returns corresponding span status code. // If the `code` is not a valid HTTP status code, returns span status Error // and false. diff --git a/semconv/v1.6.1/http_test.go b/semconv/v1.6.1/http_test.go index 945ae3cee..017bd1350 100644 --- a/semconv/v1.6.1/http_test.go +++ b/semconv/v1.6.1/http_test.go @@ -25,6 +25,7 @@ import ( "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/codes" + "go.opentelemetry.io/otel/trace" ) type tlsOption int @@ -854,7 +855,7 @@ func TestHTTPAttributesFromHTTPStatusCode(t *testing.T) { func TestSpanStatusFromHTTPStatusCode(t *testing.T) { for code := 0; code < 1000; code++ { - expected := getExpectedCodeForHTTPCode(code) + expected := getExpectedCodeForHTTPCode(code, trace.SpanKindClient) got, msg := SpanStatusFromHTTPStatusCode(code) assert.Equalf(t, expected, got, "%s vs %s", expected, got) @@ -867,7 +868,24 @@ func TestSpanStatusFromHTTPStatusCode(t *testing.T) { } } -func getExpectedCodeForHTTPCode(code int) codes.Code { +func TestSpanStatusFromHTTPStatusCodeAndSpanKind(t *testing.T) { + for code := 0; code < 1000; code++ { + expected := getExpectedCodeForHTTPCode(code, trace.SpanKindClient) + got, msg := SpanStatusFromHTTPStatusCodeAndSpanKind(code, trace.SpanKindClient) + assert.Equalf(t, expected, got, "%s vs %s", expected, got) + + _, valid := validateHTTPStatusCode(code) + if !valid { + assert.NotEmpty(t, msg, "message should be set if error cannot be inferred from code") + } else { + assert.Empty(t, msg, "message should not be set if error can be inferred from code") + } + } + code, _ := SpanStatusFromHTTPStatusCodeAndSpanKind(400, trace.SpanKindServer) + assert.Equalf(t, codes.Unset, code, "message should be set if error cannot be inferred from code") +} + +func getExpectedCodeForHTTPCode(code int, spanKind trace.SpanKind) codes.Code { if http.StatusText(code) == "" { return codes.Error } @@ -886,6 +904,9 @@ func getExpectedCodeForHTTPCode(code int) codes.Code { if category > 0 && category < 4 { return codes.Unset } + if spanKind == trace.SpanKindServer && category == 4 { + return codes.Unset + } return codes.Error } diff --git a/semconv/v1.7.0/http.go b/semconv/v1.7.0/http.go index 15738cfbd..9b430fac0 100644 --- a/semconv/v1.7.0/http.go +++ b/semconv/v1.7.0/http.go @@ -21,6 +21,8 @@ import ( "strconv" "strings" + "go.opentelemetry.io/otel/trace" + "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/codes" ) @@ -269,6 +271,21 @@ func SpanStatusFromHTTPStatusCode(code int) (codes.Code, string) { return spanCode, "" } +// SpanStatusFromHTTPStatusCodeAndSpanKind generates a status code and a message +// as specified by the OpenTelemetry specification for a span. +// Exclude 4xx for SERVER to set the appropriate status. +func SpanStatusFromHTTPStatusCodeAndSpanKind(code int, spanKind trace.SpanKind) (codes.Code, string) { + spanCode, valid := validateHTTPStatusCode(code) + if !valid { + return spanCode, fmt.Sprintf("Invalid HTTP status code %d", code) + } + category := code / 100 + if spanKind == trace.SpanKindServer && category == 4 { + return codes.Unset, "" + } + return spanCode, "" +} + // Validates the HTTP status code and returns corresponding span status code. // If the `code` is not a valid HTTP status code, returns span status Error // and false. diff --git a/semconv/v1.7.0/http_test.go b/semconv/v1.7.0/http_test.go index 945ae3cee..5d707be5d 100644 --- a/semconv/v1.7.0/http_test.go +++ b/semconv/v1.7.0/http_test.go @@ -20,6 +20,8 @@ import ( "strings" "testing" + "go.opentelemetry.io/otel/trace" + "github.com/google/go-cmp/cmp" "github.com/stretchr/testify/assert" @@ -854,7 +856,7 @@ func TestHTTPAttributesFromHTTPStatusCode(t *testing.T) { func TestSpanStatusFromHTTPStatusCode(t *testing.T) { for code := 0; code < 1000; code++ { - expected := getExpectedCodeForHTTPCode(code) + expected := getExpectedCodeForHTTPCode(code, trace.SpanKindClient) got, msg := SpanStatusFromHTTPStatusCode(code) assert.Equalf(t, expected, got, "%s vs %s", expected, got) @@ -867,7 +869,24 @@ func TestSpanStatusFromHTTPStatusCode(t *testing.T) { } } -func getExpectedCodeForHTTPCode(code int) codes.Code { +func TestSpanStatusFromHTTPStatusCodeAndSpanKind(t *testing.T) { + for code := 0; code < 1000; code++ { + expected := getExpectedCodeForHTTPCode(code, trace.SpanKindClient) + got, msg := SpanStatusFromHTTPStatusCodeAndSpanKind(code, trace.SpanKindClient) + assert.Equalf(t, expected, got, "%s vs %s", expected, got) + + _, valid := validateHTTPStatusCode(code) + if !valid { + assert.NotEmpty(t, msg, "message should be set if error cannot be inferred from code") + } else { + assert.Empty(t, msg, "message should not be set if error can be inferred from code") + } + } + code, _ := SpanStatusFromHTTPStatusCodeAndSpanKind(400, trace.SpanKindServer) + assert.Equalf(t, codes.Unset, code, "message should be set if error cannot be inferred from code") +} + +func getExpectedCodeForHTTPCode(code int, spanKind trace.SpanKind) codes.Code { if http.StatusText(code) == "" { return codes.Error } @@ -886,6 +905,9 @@ func getExpectedCodeForHTTPCode(code int) codes.Code { if category > 0 && category < 4 { return codes.Unset } + if spanKind == trace.SpanKindServer && category == 4 { + return codes.Unset + } return codes.Error }