democluster: improve logging/tracing #72642
Conversation
|
This change is |
knz
force-pushed
the
20211111-demo-phases
branch
from
33fb324 to
c43c825
Compare
knz
force-pushed
the
20211111-demo-phases
branch
from
c43c825 to
8fdea3c
Compare
knz
force-pushed
the
20211111-demo-phases
branch
from
8fdea3c to
1a5e571
Compare
pkg/cli/democluster/demo_cluster.go
Outdated
| @@ -228,10 +228,11 @@ func (c *transientCluster) Start( | |||
| // Step 1: create the first node. | |||
| { | |||
| phaseCtx := logtags.AddTag(ctx, "phase", 1) | |||
| c.infoLog(phaseCtx, "creating the first node") | |||
| ctx := phaseCtx // shadow the original ctx; folk prefer this name to "phaseCtx". | |||
There was a problem hiding this comment.
nit (for all): folk prefer this name to "phaseCtx" didn't make sense to me as to what class of error this was preventing until i read the commit message. maybe shadow the original ctx to prevent accidentally using the original ctx instead of the phaseCtx which can be a easy reach as a mistake or something?
alternatively, make all these steps individual functions passing in the phase context and having the function take in ctx and avoid the shadowing entirely. hopefully the refactor for that isn't too nasty.
There was a problem hiding this comment.
yeah, the individual functions seems to be the better choice. I'll work on that next.
There was a problem hiding this comment.
(the linter doesn't like the shadowing anyway)
knz
force-pushed
the
20211111-demo-phases
branch
from
1a5e571 to
eb766b0
Compare
|
RFAL |
Release note: None
We've seen folk make mistakes because they instinctly reach to the name "ctx" in their text editor. Prevent those mistakes by ensuring the phase context is also called "ctx". Release note: None
Release note: None
knz
force-pushed
the
20211111-demo-phases
branch
from
eb766b0 to
d3ce8d3
Compare
|
TFYR! bors r=otan |
|
Build failed: |
|
bors r=otan |
|
Build succeeded: |
All commits but the last 3 from #72644
(Reviewers: only review last 3 commits)
Informs #58938