cockroachdb / cockroachdb/cockroach

rpc,tracing: span metadata not propagated properly in local internal client optimization

Open
#135,686 2 comments 0 reactions 0 assignees View on GitHub
A-tracing branch-master C-bug T-observability
Dominant language
Go
Stars
32.5k
Forks
4.1k
PR merge metrics
PR metrics pending

Description

See https://github.com/cockroachdb/cockroach/pull/135682.

When `DistSender.RangeFeed` wants to establish a rangefeed to a node-local replica, it uses the internal client adapter:

https://github.com/cockroachdb/cockroach/blob/e51e52eb33fcb0da96f2013483317b78d6aaee2a/pkg/kv/kvclient/kvcoord/dist_sender_mux_rangefeed.go#L395

The implementation is here:

https://github.com/cockroachdb/cockroach/blob/e51e52eb33fcb0da96f2013483317b78d6aaee2a/pkg/rpc/context.go#L1039-L1047

Note how it runs the request through the streaming server interceptor.

Notably, this includes the tracing interceptor:

https://github.com/cockroachdb/cockroach/blob/e51e52eb33fcb0da96f2013483317b78d6aaee2a/pkg/rpc/context.go#L179-L182

That interceptor expects the parent span information to be present in grpc-style context metadata (see `ExtractSpanMetaFromGRPCCtx`):

https://github.com/cockroachdb/cockroach/blob/e51e52eb33fcb0da96f2013483317b78d6aaee2a/pkg/util/tracing/grpcinterceptor/grpc_interceptor.go#L139-L183

but it is not present there for requests that come in from the local internal server optimization.

As a result, the interceptor may up delegating directly to the handler (the `!SpanInclusionFunc(...)` case) unless tracing is turned on globally.

This has the effect of "passing through" the client span directly to the handler, which, on context cancellation cases, makes it likely that the server span will be used after the client span is finished. In tests, this may trigger the "span use after finish" assertion, but it would also have implications on production builds, which could be more prone to crashing as a result.

It turns out that on master at the time of writing, tracing _is_ globally enabled (the "active span registry" is on) and so we were always hitting the `StartSpanCtx` call in the snippet above, which would "bypass" the problem by making a new (and erroneously wholly unconnected span). This is more an accident than design, so once we fix that, we're going to have a problem.

Jira issue: CRDB-44677

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.