From 2c558327616b4a9da5c79851d525f0d7dde7edea Mon Sep 17 00:00:00 2001 From: Charles Korn Date: Tue, 24 Oct 2023 19:49:36 +1100 Subject: [PATCH] Fix issue where traces for server streaming gRPC calls incorrectly show the call as failing due to context cancellation (#6470) * Switch to fork of opentracing-contrib/go-grpc * Add changelog entry. --- CHANGELOG.md | 1 + go.mod | 3 +++ go.sum | 4 ++-- .../opentracing-contrib/go-grpc/client.go | 21 +++++++++++-------- vendor/modules.txt | 3 ++- 5 files changed, 20 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 738470c72f4..113bb0e6765 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -61,6 +61,7 @@ * [BUGFIX] Querier: don't try to query further ingesters if ingester query request minimization is enabled and a query limit is reached as a result of the responses from the initial set of ingesters. #6402 * [BUGFIX] Ingester: Don't cache context cancellation error when querying. #6446 * [BUGFIX] Ingester: don't ignore errors encountered while iterating through chunks or samples in response to a query request. #6469 +* [BUGFIX] All: fix issue where traces for some inter-component gRPC calls would incorrectly show the call as failing due to cancellation. #6470 ### Mixin diff --git a/go.mod b/go.mod index 710d6c40692..92b711c3afd 100644 --- a/go.mod +++ b/go.mod @@ -270,3 +270,6 @@ replace github.com/munnerz/goautoneg => github.com/grafana/goautoneg v0.0.0-2023 // Replace opentracing-contrib/go-stdlib with a fork until https://github.com/opentracing-contrib/go-stdlib/pull/68 is merged. replace github.com/opentracing-contrib/go-stdlib => github.com/grafana/opentracing-contrib-go-stdlib v0.0.0-20230509071955-f410e79da956 + +// Replace opentracing-contrib/go-grpc with a fork until https://github.com/opentracing-contrib/go-grpc/pull/16 is merged. +replace github.com/opentracing-contrib/go-grpc => github.com/charleskorn/go-grpc v0.0.0-20231024023642-e9298576254f diff --git a/go.sum b/go.sum index 16b0356a383..09db23f7258 100644 --- a/go.sum +++ b/go.sum @@ -177,6 +177,8 @@ github.com/cespare/xxhash v1.1.0/go.mod h1:XrSqR1VqqWfGrhpAt58auRo0WTKS1nRRg3ghf github.com/cespare/xxhash/v2 v2.1.1/go.mod h1:VGX0DQ3Q6kWi7AoAeZDth3/j3BFtOZR5XLFGgcrjCOs= github.com/cespare/xxhash/v2 v2.2.0 h1:DC2CZ1Ep5Y4k3ZQ899DldepgrayRUGE6BBZ/cd9Cj44= github.com/cespare/xxhash/v2 v2.2.0/go.mod h1:VGX0DQ3Q6kWi7AoAeZDth3/j3BFtOZR5XLFGgcrjCOs= +github.com/charleskorn/go-grpc v0.0.0-20231024023642-e9298576254f h1:P1GSPnbxmhUafKGBcaY2qqx34mBdC4GVDm/RN3iKKuE= +github.com/charleskorn/go-grpc v0.0.0-20231024023642-e9298576254f/go.mod h1:DYR5Eij8rJl8h7gblRrOZ8g0kW1umSpKqYIBTgeDtLo= github.com/chromedp/cdproto v0.0.0-20210526005521-9e51b9051fd0/go.mod h1:At5TxYYdxkbQL0TSefRjhLE3Q0lgvqKKMSFUglJ7i1U= github.com/chromedp/cdproto v0.0.0-20210706234513-2bc298e8be7f/go.mod h1:At5TxYYdxkbQL0TSefRjhLE3Q0lgvqKKMSFUglJ7i1U= github.com/chromedp/cdproto v0.0.0-20230802225258-3cf4e6d46a89 h1:aPflPkRFkVwbW6dmcVqfgwp1i+UWGFH6VgR1Jim5Ygc= @@ -800,8 +802,6 @@ github.com/opencontainers/go-digest v1.0.0 h1:apOUWs51W5PlhuyGyz9FCeeBIOUDA/6nW8 github.com/opencontainers/go-digest v1.0.0/go.mod h1:0JzlMkj0TRzQZfJkVvzbP0HBR3IKzErnv2BNG4W4MAM= github.com/opencontainers/image-spec v1.0.2 h1:9yCKha/T5XdGtO0q9Q9a6T5NUCsTn/DrBg0D7ufOcFM= github.com/opencontainers/image-spec v1.0.2/go.mod h1:BtxoFyWECRxE4U/7sNtV5W15zMzWCbyJoFRP3s7yZA0= -github.com/opentracing-contrib/go-grpc v0.0.0-20210225150812-73cb765af46e h1:4cPxUYdgaGzZIT5/j0IfqOrrXmq6bG8AwvwisMXpdrg= -github.com/opentracing-contrib/go-grpc v0.0.0-20210225150812-73cb765af46e/go.mod h1:DYR5Eij8rJl8h7gblRrOZ8g0kW1umSpKqYIBTgeDtLo= github.com/opentracing/opentracing-go v1.1.0/go.mod h1:UkNAQd3GIcIGf0SeVgPpRdFStlNbqXla1AfSYxPUl2o= github.com/opentracing/opentracing-go v1.2.1-0.20220228012449-10b1cf09e00b h1:FfH+VrHHk6Lxt9HdVS0PXzSXFyS2NbZKXv33FYPol0A= github.com/opentracing/opentracing-go v1.2.1-0.20220228012449-10b1cf09e00b/go.mod h1:AC62GU6hc0BrNm+9RK9VSiwa/EUe1bkIeFORAMcHvJU= diff --git a/vendor/github.com/opentracing-contrib/go-grpc/client.go b/vendor/github.com/opentracing-contrib/go-grpc/client.go index c9496c85aff..d764506717b 100644 --- a/vendor/github.com/opentracing-contrib/go-grpc/client.go +++ b/vendor/github.com/opentracing-contrib/go-grpc/client.go @@ -124,15 +124,11 @@ func OpenTracingStreamClientInterceptor(tracer opentracing.Tracer, optFuncs ...O clientSpan.Finish() return cs, err } - return newOpenTracingClientStream(cs, method, desc, clientSpan, otgrpcOpts), nil + return newOpenTracingClientStream(ctx, cs, method, desc, clientSpan, otgrpcOpts), nil } } -func newOpenTracingClientStream(cs grpc.ClientStream, method string, desc *grpc.StreamDesc, clientSpan opentracing.Span, otgrpcOpts *options) grpc.ClientStream { - // Grab the client stream context because when the finish function or the goroutine below will be - // executed it's not guaranteed cs.Context() will be valid. - csCtx := cs.Context() - +func newOpenTracingClientStream(ctx context.Context, cs grpc.ClientStream, method string, desc *grpc.StreamDesc, clientSpan opentracing.Span, otgrpcOpts *options) grpc.ClientStream { finishChan := make(chan struct{}) isFinished := new(int32) @@ -152,7 +148,7 @@ func newOpenTracingClientStream(cs grpc.ClientStream, method string, desc *grpc. SetSpanTags(clientSpan, err, true) } if otgrpcOpts.decorator != nil { - otgrpcOpts.decorator(csCtx, clientSpan, method, nil, nil, err) + otgrpcOpts.decorator(ctx, clientSpan, method, nil, nil, err) } } go func() { @@ -160,8 +156,15 @@ func newOpenTracingClientStream(cs grpc.ClientStream, method string, desc *grpc. case <-finishChan: // The client span is being finished by another code path; hence, no // action is necessary. - case <-csCtx.Done(): - finishFunc(csCtx.Err()) + case <-ctx.Done(): + // Why use ctx rather than cs.Context()? Two reasons: + // 1. According to its docs, cs.Context() should not be used until after the first Header() or + // RecvMsg() call has returned. + // 2. ClientStream implementations cancel their context as soon as an error is received in Header(), + // RecvMsg(), SendMsg() or CloseSend(). This causes a race between the interceptor logging the + // returned error and this method logging the cancelled context. + // Using ctx avoids both of these issues. + finishFunc(ctx.Err()) } }() otcs := &openTracingClientStream{ diff --git a/vendor/modules.txt b/vendor/modules.txt index bcecbfde584..1469187a9eb 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -783,7 +783,7 @@ github.com/open-telemetry/opentelemetry-collector-contrib/pkg/translator/prometh # github.com/open-telemetry/opentelemetry-collector-contrib/pkg/translator/prometheusremotewrite v0.84.0 ## explicit; go 1.20 github.com/open-telemetry/opentelemetry-collector-contrib/pkg/translator/prometheusremotewrite -# github.com/opentracing-contrib/go-grpc v0.0.0-20210225150812-73cb765af46e +# github.com/opentracing-contrib/go-grpc v0.0.0-20210225150812-73cb765af46e => github.com/charleskorn/go-grpc v0.0.0-20231024023642-e9298576254f ## explicit; go 1.11 github.com/opentracing-contrib/go-grpc # github.com/opentracing-contrib/go-stdlib v1.0.0 => github.com/grafana/opentracing-contrib-go-stdlib v0.0.0-20230509071955-f410e79da956 @@ -1472,3 +1472,4 @@ sigs.k8s.io/yaml # github.com/grafana/regexp => github.com/grafana/regexp v0.0.0-20221005093135-b4c2bcb0a4b6 # github.com/munnerz/goautoneg => github.com/grafana/goautoneg v0.0.0-20231010094147-47ce5e72a9ae # github.com/opentracing-contrib/go-stdlib => github.com/grafana/opentracing-contrib-go-stdlib v0.0.0-20230509071955-f410e79da956 +# github.com/opentracing-contrib/go-grpc => github.com/charleskorn/go-grpc v0.0.0-20231024023642-e9298576254f