From 7100618d4da2a4c8648c345d7dac02c69dfaee3e Mon Sep 17 00:00:00 2001 From: Alex Unger Date: Fri, 10 Dec 2021 15:52:36 +0100 Subject: [PATCH 1/5] prevented from logging traceid when no trace is available --- internal/grpc/interceptors/appctx/appctx.go | 14 ++++++++++++-- internal/http/interceptors/appctx/appctx.go | 12 +++++++++++- pkg/rgrpc/status/status.go | 4 ++++ 3 files changed, 27 insertions(+), 3 deletions(-) diff --git a/internal/grpc/interceptors/appctx/appctx.go b/internal/grpc/interceptors/appctx/appctx.go index 1bb8238c46..b751f6d9b2 100644 --- a/internal/grpc/interceptors/appctx/appctx.go +++ b/internal/grpc/interceptors/appctx/appctx.go @@ -37,7 +37,7 @@ func NewUnary(log zerolog.Logger) grpc.UnaryServerInterceptor { ctx, span = rtrace.Provider.Tracer("grpc").Start(ctx, "grpc unary") } - sub := log.With().Str("traceid", span.SpanContext().TraceID().String()).Logger() + sub := log.With().Str("traceid", getTraceIDFromSpan(span)).Logger() ctx = appctx.WithLogger(ctx, &sub) res, err := handler(ctx, req) return res, err @@ -57,7 +57,7 @@ func NewStream(log zerolog.Logger) grpc.StreamServerInterceptor { ctx, span = rtrace.Provider.Tracer("grpc").Start(ctx, "grpc stream") } - sub := log.With().Str("traceid", span.SpanContext().TraceID().String()).Logger() + sub := log.With().Str("traceid", getTraceIDFromSpan(span)).Logger() ctx = appctx.WithLogger(ctx, &sub) wrapped := newWrappedServerStream(ctx, ss) @@ -79,3 +79,13 @@ type wrappedServerStream struct { func (ss *wrappedServerStream) Context() context.Context { return ss.newCtx } + +func getTraceIDFromSpan(span trace.Span) string { + traceID := "" + if span.SpanContext().TraceID() == [16]byte{} { + traceID = "" + } else { + traceID = span.SpanContext().TraceID().String() + } + return traceID +} diff --git a/internal/http/interceptors/appctx/appctx.go b/internal/http/interceptors/appctx/appctx.go index 9d34a78981..fb0ead3183 100644 --- a/internal/http/interceptors/appctx/appctx.go +++ b/internal/http/interceptors/appctx/appctx.go @@ -49,9 +49,19 @@ func handler(log zerolog.Logger, h http.Handler) http.Handler { ctx, span = rtrace.Provider.Tracer("http").Start(ctx, "http interceptor") } - sub := log.With().Str("traceid", span.SpanContext().TraceID().String()).Logger() + sub := log.With().Str("traceid", getTraceIDFromSpan(span)).Logger() ctx = appctx.WithLogger(ctx, &sub) r = r.WithContext(ctx) h.ServeHTTP(w, r) }) } + +func getTraceIDFromSpan(span trace.Span) string { + traceID := "" + if span.SpanContext().TraceID() == [16]byte{} { + traceID = "" + } else { + traceID = span.SpanContext().TraceID().String() + } + return traceID +} diff --git a/pkg/rgrpc/status/status.go b/pkg/rgrpc/status/status.go index 8881ba984c..29cd919277 100644 --- a/pkg/rgrpc/status/status.go +++ b/pkg/rgrpc/status/status.go @@ -204,5 +204,9 @@ func NewErrorFromCode(code rpc.Code, pkgname string) error { // internal function to attach the trace to a context func getTrace(ctx context.Context) string { span := trace.SpanFromContext(ctx) + if span.SpanContext().TraceID() == [16]byte{} { + return "" + } + return span.SpanContext().TraceID().String() } From c6555fc3e98e5df6a03801fefb445d1b33b02d0f Mon Sep 17 00:00:00 2001 From: Alex Unger Date: Fri, 10 Dec 2021 15:56:52 +0100 Subject: [PATCH 2/5] add changelog --- changelog/unreleased/fix-traces-logging.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelog/unreleased/fix-traces-logging.md diff --git a/changelog/unreleased/fix-traces-logging.md b/changelog/unreleased/fix-traces-logging.md new file mode 100644 index 0000000000..3053db0f84 --- /dev/null +++ b/changelog/unreleased/fix-traces-logging.md @@ -0,0 +1,5 @@ +Bugfix: If a trace is not available do not log default trace value + +Prevent from logging `traceid=0000000000000000` if there is no traceid for the given span. + +https://github.com/cs3org/reva/pull/2352 \ No newline at end of file From 53e25c462284d36f0a5d301fb5ae0d462f8fef40 Mon Sep 17 00:00:00 2001 From: Alex Unger Date: Fri, 10 Dec 2021 16:34:21 +0100 Subject: [PATCH 3/5] adjusted tests expectations --- .../services/appregistry/appregistry_test.go | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/internal/grpc/services/appregistry/appregistry_test.go b/internal/grpc/services/appregistry/appregistry_test.go index 4230a1ffe8..7837128aee 100644 --- a/internal/grpc/services/appregistry/appregistry_test.go +++ b/internal/grpc/services/appregistry/appregistry_test.go @@ -76,7 +76,7 @@ func Test_ListAppProviders(t *testing.T) { want: ®istrypb.ListAppProvidersResponse{ Status: &rpcv1beta1.Status{ Code: 1, - Trace: "00000000000000000000000000000000", + Trace: "", Message: "", }, Providers: []*registrypb.ProviderInfo{ @@ -98,7 +98,7 @@ func Test_ListAppProviders(t *testing.T) { want: ®istrypb.ListAppProvidersResponse{ Status: &rpcv1beta1.Status{ Code: 1, - Trace: "00000000000000000000000000000000", + Trace: "", }, Providers: []*registrypb.ProviderInfo{}, }, @@ -112,7 +112,7 @@ func Test_ListAppProviders(t *testing.T) { want: ®istrypb.ListAppProvidersResponse{ Status: &rpcv1beta1.Status{ Code: 1, - Trace: "00000000000000000000000000000000", + Trace: "", Message: "", }, Providers: []*registrypb.ProviderInfo{}, @@ -218,7 +218,7 @@ func Test_GetAppProviders(t *testing.T) { want: ®istrypb.GetAppProvidersResponse{ Status: &rpcv1beta1.Status{ Code: 1, - Trace: "00000000000000000000000000000000", + Trace: "", Message: "", }, Providers: []*registrypb.ProviderInfo{ @@ -235,7 +235,7 @@ func Test_GetAppProviders(t *testing.T) { want: ®istrypb.GetAppProvidersResponse{ Status: &rpcv1beta1.Status{ Code: 1, - Trace: "00000000000000000000000000000000", + Trace: "", Message: "", }, Providers: []*registrypb.ProviderInfo{ @@ -252,7 +252,7 @@ func Test_GetAppProviders(t *testing.T) { want: ®istrypb.GetAppProvidersResponse{ Status: &rpcv1beta1.Status{ Code: 15, - Trace: "00000000000000000000000000000000", + Trace: "", Message: "error looking for the app provider", }, Providers: nil, @@ -264,7 +264,7 @@ func Test_GetAppProviders(t *testing.T) { want: ®istrypb.GetAppProvidersResponse{ Status: &rpcv1beta1.Status{ Code: 15, - Trace: "00000000000000000000000000000000", + Trace: "", Message: "error looking for the app provider", }, Providers: nil, @@ -276,7 +276,7 @@ func Test_GetAppProviders(t *testing.T) { want: ®istrypb.GetAppProvidersResponse{ Status: &rpcv1beta1.Status{ Code: 15, - Trace: "00000000000000000000000000000000", + Trace: "", Message: "error looking for the app provider", }, Providers: nil, @@ -288,7 +288,7 @@ func Test_GetAppProviders(t *testing.T) { want: ®istrypb.GetAppProvidersResponse{ Status: &rpcv1beta1.Status{ Code: 15, - Trace: "00000000000000000000000000000000", + Trace: "", Message: "error looking for the app provider", }, Providers: nil, From 368db6e7db25ff1088fd264781951f352bea58f6 Mon Sep 17 00:00:00 2001 From: Alex Unger Date: Fri, 10 Dec 2021 16:40:24 +0100 Subject: [PATCH 4/5] improve readability --- internal/grpc/interceptors/appctx/appctx.go | 9 +++------ internal/http/interceptors/appctx/appctx.go | 10 ++++------ pkg/rgrpc/status/status.go | 6 +++--- 3 files changed, 10 insertions(+), 15 deletions(-) diff --git a/internal/grpc/interceptors/appctx/appctx.go b/internal/grpc/interceptors/appctx/appctx.go index b751f6d9b2..45ab8511d3 100644 --- a/internal/grpc/interceptors/appctx/appctx.go +++ b/internal/grpc/interceptors/appctx/appctx.go @@ -81,11 +81,8 @@ func (ss *wrappedServerStream) Context() context.Context { } func getTraceIDFromSpan(span trace.Span) string { - traceID := "" - if span.SpanContext().TraceID() == [16]byte{} { - traceID = "" - } else { - traceID = span.SpanContext().TraceID().String() + if span.SpanContext().TraceID() != [16]byte{} { + return span.SpanContext().TraceID().String() } - return traceID + return "" } diff --git a/internal/http/interceptors/appctx/appctx.go b/internal/http/interceptors/appctx/appctx.go index fb0ead3183..9fb8638515 100644 --- a/internal/http/interceptors/appctx/appctx.go +++ b/internal/http/interceptors/appctx/appctx.go @@ -57,11 +57,9 @@ func handler(log zerolog.Logger, h http.Handler) http.Handler { } func getTraceIDFromSpan(span trace.Span) string { - traceID := "" - if span.SpanContext().TraceID() == [16]byte{} { - traceID = "" - } else { - traceID = span.SpanContext().TraceID().String() + if span.SpanContext().TraceID() != [16]byte{} { + return span.SpanContext().TraceID().String() } - return traceID + + return "" } diff --git a/pkg/rgrpc/status/status.go b/pkg/rgrpc/status/status.go index 29cd919277..f204a7c7f1 100644 --- a/pkg/rgrpc/status/status.go +++ b/pkg/rgrpc/status/status.go @@ -204,9 +204,9 @@ func NewErrorFromCode(code rpc.Code, pkgname string) error { // internal function to attach the trace to a context func getTrace(ctx context.Context) string { span := trace.SpanFromContext(ctx) - if span.SpanContext().TraceID() == [16]byte{} { - return "" + if span.SpanContext().TraceID() != [16]byte{} { + return span.SpanContext().TraceID().String() } - return span.SpanContext().TraceID().String() + return "" } From 6c4826d6cd85649eed31310ab426cac1c1f593c2 Mon Sep 17 00:00:00 2001 From: Alex Unger Date: Mon, 13 Dec 2021 15:29:44 +0100 Subject: [PATCH 5/5] apply review changes --- internal/grpc/interceptors/appctx/appctx.go | 4 ++-- internal/http/interceptors/appctx/appctx.go | 4 ++-- pkg/rgrpc/status/status.go | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/internal/grpc/interceptors/appctx/appctx.go b/internal/grpc/interceptors/appctx/appctx.go index 45ab8511d3..d47dc0c48e 100644 --- a/internal/grpc/interceptors/appctx/appctx.go +++ b/internal/grpc/interceptors/appctx/appctx.go @@ -81,8 +81,8 @@ func (ss *wrappedServerStream) Context() context.Context { } func getTraceIDFromSpan(span trace.Span) string { - if span.SpanContext().TraceID() != [16]byte{} { - return span.SpanContext().TraceID().String() + if b := span.SpanContext().TraceID(); b != [16]byte{} { + return b.String() } return "" } diff --git a/internal/http/interceptors/appctx/appctx.go b/internal/http/interceptors/appctx/appctx.go index 9fb8638515..5b089e3c9a 100644 --- a/internal/http/interceptors/appctx/appctx.go +++ b/internal/http/interceptors/appctx/appctx.go @@ -57,8 +57,8 @@ func handler(log zerolog.Logger, h http.Handler) http.Handler { } func getTraceIDFromSpan(span trace.Span) string { - if span.SpanContext().TraceID() != [16]byte{} { - return span.SpanContext().TraceID().String() + if b := span.SpanContext().TraceID(); b != [16]byte{} { + return b.String() } return "" diff --git a/pkg/rgrpc/status/status.go b/pkg/rgrpc/status/status.go index f204a7c7f1..9fed7f06ac 100644 --- a/pkg/rgrpc/status/status.go +++ b/pkg/rgrpc/status/status.go @@ -204,8 +204,8 @@ func NewErrorFromCode(code rpc.Code, pkgname string) error { // internal function to attach the trace to a context func getTrace(ctx context.Context) string { span := trace.SpanFromContext(ctx) - if span.SpanContext().TraceID() != [16]byte{} { - return span.SpanContext().TraceID().String() + if b := span.SpanContext().TraceID(); b != [16]byte{} { + return b.String() } return ""