search

cockroachdb / cockroach

CockroachDB — the cloud native, distributed SQL database designed for high availability, effortless scale, and control over data placement.
https://www.cockroachlabs.com
Other
30.18k stars 3.82k forks source link

tracing: consider removing noop span and sterile spans #135803

Open tbg opened 5 days ago

tbg commented 5 days ago

While working on https://github.com/cockroachdb/cockroach/pull/135146, there were a few instances in which a check sp != nil needed to be upgraded to also check for && sp.IsNoop() to avoid creating additional spans. For example, https://github.com/cockroachdb/cockroach/pull/135146#discussion_r1847373895 and https://github.com/cockroachdb/cockroach/pull/135146#discussion_r1847377968.

There seems to be little clarity on what noop spans are for; they are one more animal in the zoo of different trace spans. From a performance point of view, they are detrimental. A noop span serves no purpose that I can tell, but it is usually returned to clients wrapped in a context, which costs allocations and CPU cycles.

Sterile spans are another animal in the zoo: these are spans that won't have children. To make things extra fun, there is also a sterile noop span. (I'm not sure what the difference between the noop span and the sterile noop span is, but I bet there is one). I think sterile spans are used only by RunAsyncTaskEx here:

https://github.com/cockroachdb/cockroach/blob/85b6c92eb2115491881c4549ca96995ae1bec346/pkg/util/stop/stopper.go#L481

Jira issue: CRDB-44722

RaduBerinde commented 5 days ago

There seems to be little clarity on what noop spans are for

I don't think they make sense anymore. I believe they were used when the span was tied to the tracer and the only way to get a tracer was to have a span in the context (the description in https://github.com/cockroachdb/cockroach/pull/55937 hints at this). Both our infrastructure and the APIs we use have changed significantly since then.