storage: propagate tracing info through Raft proposals#39431
storage: propagate tracing info through Raft proposals#39431andreimatei merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
nvb
left a comment
There was a problem hiding this comment.
I guess it's a little harder to pull out tracing spans at that stage because we don't decode the Raft command. Still, I'm curious whether there are any plans to do something like that?
Reviewed 5 of 5 files at r1.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @andreimatei)
pkg/storage/replica_application_decoder.go, line 139 at r1 (raw file):
for it.init(&d.cmdBuf); it.Valid(); it.Next() { cmd := it.cur() parentCtx := ctx
Get rid of this variable.
pkg/storage/replica_application_decoder.go, line 142 at r1 (raw file):
if cmd.IsLocal() { parentCtx = cmd.proposal.ctx cmd.ctx, cmd.sp = tracing.ForkCtxSpan(parentCtx, "raft application")
Pull "raft application" into const opName = "raft application"
andreimatei
left a comment
There was a problem hiding this comment.
bors r+
I guess it's a little harder to pull out tracing spans at that stage because we don't decode the Raft command. Still, I'm curious whether there are any plans to do something like that?
Now there is :P
Once this merges, I'll file an issue for what you want.
Reviewable status:
pkg/storage/replica_application_decoder.go, line 139 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Get rid of this variable.
done
pkg/storage/replica_application_decoder.go, line 142 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Pull "raft application" into
const opName = "raft application"
done
Build failed |
bd5451a to
85dda1b
Compare
andreimatei
left a comment
There was a problem hiding this comment.
hey @nvanbenschoten and @ajwerner , check out the increase in proposal size showed by TestProposalOverhead. We all still cool on this? (I am)
Reviewable status:
50 bytes of overhead to 133 bytes per Raft entry is a major regression. We need a lot to justify that kind of change. I thought this was only going to hurt when tracing is enabled. |
andreimatei
left a comment
There was a problem hiding this comment.
I thought this was only going to hurt when tracing is enabled.
yeah now that I think about it, that's the intention. Maybe getTraceData() needs to handle the noop span... I'll see.
Reviewable status:
andreimatei
left a comment
There was a problem hiding this comment.
note to self: TestProposalOverhead fails because tracing is enabled for its purposes because nobody sets trace.debug.enable for the test's testContext.
Reviewable status:
andreimatei
left a comment
There was a problem hiding this comment.
note to self: let's default trace.debug.enable to false and stop this nonsense
Reviewable status:
This patch propagates the SpanContext of the proposer to all the replicas, which then decode it and create a "follows from" child span for the application. The local replica does not use this, since it already had direct access to the proposer's ctx and span. Now tracing utilities will show us all the command applications for a proposal tied to each other and we'll profit. Release note: None
andreimatei
left a comment
There was a problem hiding this comment.
Now trace.debug.enable defaults to false and the test correctly shows no overhead when tracing is off.
bors r+
Reviewable status:
This patch propagates the SpanContext of the proposer to all the
replicas, which then decode it and create a "follows from" child span
for the application. The local replica does not use this, since it
already had direct access to the proposer's ctx and span.
Now tracing utilities will show us all the command applications for a
proposal tied to each other and we'll profit.
Release note: None