stop,log: tracing improvements

[andreimatei]

Improvements around making the traces of long-running background processes look better. See individual commits.

[cockroach-teamcity]

This change is 

[andreimatei]

cc @aayushshah15

[tbg]

Going to leave the review to @abarganier but feel free to loop me into specific parts of the review where I can be helpful.

[abarganier]

The tracing changes overall lgtm, just a couple small comments, but I'd also like to hear from someone better versed in KV about the changes to the Stopper usage.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @abarganier and @andreimatei)

---

pkg/util/stop/stopper.go, line 161 at r3 (raw file):

	tracer   *tracing.Tracer   // tracer used to create spans for tasks

Does this need to be protected by a mutex if we're going to expose methods like SetTracer()?

---

pkg/util/tracing/tracer.go, line 293 at r3 (raw file):

Quoted 12 lines of code…

type registry struct {
	mu struct {
		syncutil.Mutex
		spansPerTrace map[uint64]traceInfo
	}
}

type traceInfo struct {
	rootName   string
	remoteRoot bool
	spans      int
}

nit: can we add some comments about the purpose of this registry?

[andreimatei]

First two commits are #70850.

I've reworked much of this; decided to go a different way. I've reduced the control one has at the point where it creates a child span, in favor of affording a new option at the point where a parent span is created. I've introduced the concept of a span that cannot have any children, to model a long-running task. Long-running operations getting children leads to too many problems.
This time, I'd like to kindly ask @aayushshah15 for a review.

[aayushshah15]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @aayushshah15, @abarganier, and @andreimatei)

---

pkg/kv/kvserver/replica_range_lease.go, line 359 at r15 (raw file):

			// acquisition's trace in all of them, but let's at least include it in
			// the request that triggered it.
			SpanOpt: stop.ChildSpan,

This isn't necessarily related to this patch, but could you explain why this isn't a FollowsFromSpan relationship instead?

Your comment above the FollowsFromSpan SpanOption says the following:

	// Use this when the caller will not wait for the task to finish, but a
	// relationship between the caller and the task might still be useful to
	// visualize in a trace collector. Generally avoid using this for tasks

---

pkg/util/tracing/span.go, line 270 at r14 (raw file):

	// Note that this field is unlike all the other in that it doesn't make it
	// across the wire through a carrier. As can be seen in
	// Tracer.InjectMetaInto(carrier), is sterile is set, then we don't propagate

nit: s/is sterile/if sterile

[andreimatei]

I've verified with BenchmarkTracing that this patch does not regress performance. In fact, it seems to marginally help it.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @aayushshah15 and @abarganier)

---

pkg/kv/kvserver/replica_range_lease.go, line 359 at r15 (raw file):

Previously, aayushshah15 (Aayush Shah) wrote…

This isn't necessarily related to this patch, but could you explain why this isn't a FollowsFromSpan relationship instead?

Your comment above the FollowsFromSpan SpanOption says the following:

	// Use this when the caller will not wait for the task to finish, but a
	// relationship between the caller and the task might still be useful to
	// visualize in a trace collector. Generally avoid using this for tasks

FollowsFromSpan is not appropriate here because, even though the parent doesn't necessarily wait for the child, it does wait for the child in the normal path. And if you're looking at a trace for the parent, you want to see the lease acquisition in it. That's what the comment above is trying to say.

---

pkg/util/stop/stopper.go, line 161 at r3 (raw file):

Previously, abarganier (Alex Barganier) wrote…

	tracer   *tracing.Tracer   // tracer used to create spans for tasks

Does this need to be protected by a mutex if we're going to expose methods like SetTracer()?

Well SetTracer is documented as not allowing calls concurrent with starting tasks.
The tracer is supposed to be passed before the stopper starts being generally used. I have tried making WithTracer the only way to supply it but, alas, I've failed because of the intricate way in which our Server is built up; the Stopper is instantiated by the cli layer, etc. I've tried to untangle the spaghetti, but failed.

---

pkg/util/tracing/span.go, line 270 at r14 (raw file):

Previously, aayushshah15 (Aayush Shah) wrote…

nit: s/is sterile/if sterile

done

---

pkg/util/tracing/tracer.go, line 293 at r3 (raw file):

Previously, abarganier (Alex Barganier) wrote…

type registry struct {
	mu struct {
		syncutil.Mutex
		spansPerTrace map[uint64]traceInfo
	}
}

type traceInfo struct {
	rootName   string
	remoteRoot bool
	spans      int
}

nit: can we add some comments about the purpose of this registry?

This was unused debugging code. Deleted, thanks.

[andreimatei]

bors r+

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @aayushshah15 and @abarganier)

[craig]

Build succeeded:

• GitHub CI (Cockroach)