*: avoid constructing `AmbientContext` without a constructor function by knz · Pull Request #72912 · cockroachdb/cockroach

knz · 2021-11-18T15:08:23Z

Informs #72815: if we're going to use a mandatory constructor, the first step is to ensure that code doesn't instantiate AC nilly-willy.

cockroach-teamcity · 2021-11-18T15:08:46Z

This change is

tbg

I didn't scrutinize the code line-by-line, but note that you added a lint which is more useful than any close read I could provide anyway.

Sorry to have let this sit.

andreimatei · 2021-12-16T14:36:25Z

pkg/testutils/lint/lint_test.go

+		}
+
+		if err := stream.ForEach(filter, func(s string) {
+			t.Errorf("\n%s <- forbidden; reuse an existing AC when possible, or use one of the constructor functions instead: log.MakeServerAC(), log.MakeClientAC() or testutils.MakeAC()", s)


please make sure there's a pragma that can be used to inhibit the linter at a call site.

andreimatei · 2021-12-16T14:37:18Z

pkg/testutils/lint/lint_test.go

+		}
+
+		if err := stream.ForEach(filter, func(s string) {
+			t.Errorf("\n%s <- forbidden; reuse an existing AC when possible, or use one of the constructor functions instead: log.MakeServerAC(), log.MakeClientAC() or testutils.MakeAC()", s)


please add some comments to this linter, and the commit message, say why you're doing this

andreimatei · 2021-12-16T14:38:50Z

pkg/testutils/lint/lint_test.go

+		}
+
+		if err := stream.ForEach(filter, func(s string) {
+			t.Errorf("\n%s <- forbidden; reuse an existing AC when possible, or use one of the constructor functions instead: log.MakeServerAC(), log.MakeClientAC() or testutils.MakeAC()", s)


please make it more clear what's being prohibited - in a comment on the linter, and in the error message. I don't think it's obvious that constructing the struct is what's being rejected.

andreimatei

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz and @RaduBerinde)

pkg/acceptance/localcluster/cluster.go, line 276 at r18 (raw file):

	rpcCtx := rpc.NewContext(rpc.ContextOptions{
		TenantID:   roachpb.SystemTenantID,
		AmbientCtx: testutils.MakeAmbientCtx(),

I think this testutils.MakeAmbientCtx() is bad news because it creates a Tracer internally. This was always fishy, but it became more brittle recently because I've made it illegal to mix spans created by different tracers within a single trace (I've had my reasons for doing this now that I'm trying to make tracing real; it was always pretty broken). And it's very easy to end up mixing tracers if you have more than one. So we should fight against creating tracers; as much as possible, there should be one tracer passed in everywhere.

pkg/cli/start.go, line 766 at r15 (raw file):

			// Don't use shutdownCtx because this is in a goroutine that may
			// still be running after shutdownCtx's span has been finished.
			drainCtx := ambientCtx.AnnotateCtx(context.Background())

are the other uses of ambientCtx in this method also broken?

pkg/server/server.go, line 2932 at r22 (raw file):

// AmbientCtx retrieves the ambient context for this server.
func (s *Server) AmbientCtx() log.AmbientContext {

it seems bizarre to me to expose an AmbientCtx. The AmbientCtx is kinda by nature internal I think; it's supposed to be used to background operations originating in the inner conmponent. Where are you using all these new methods? If they're indeed needed, consider squashing this commit with some uses.

pkg/util/log/ambient_context.go, line 194 at r16 (raw file):

// MakeClientAmbientContext creates an AmbientContext for use by
// client commands.
func MakeClientAmbientContext(tracer *tracing.Tracer) AmbientContext {

I'm not sure the name Client here deserves to exist. If it does exist, let's explain more what a "client command" is. But I think it'd be better if this was just named MakeAmbientContext. The "client commands" seem to use this only to pass to NewRPCContext, and I think that an RPCContext shouldn't even necessarily need an AmbientCtx (as it does now). I think it should only have a Tracer. AFAICS, the AmbientCtx is used only for this ctx here:

cockroach/pkg/rpc/context.go

Line 449 in 775c2b0

    
           masterCtx, cancel := context.WithCancel(opts.AmbientCtx.AnnotateCtx(context.Background()))

I think for that one, it should just use the ctx passed to NewContext.

pkg/util/log/ambient_context.go, line 199 at r17 (raw file):

// MakeDummyAmbientContext creates an AmbientContext for use in tests.
func MakeDummyAmbientContext(tracer *tracing.Tracer) AmbientContext {

What's "dummy" about it? I think this should be the same as MakeClientAmbientContext - simply MakeClientAmbientContext.

knz · 2021-12-16T19:20:31Z

Before I engage with the review comments, I'd like to make it clear that I'm only doing this work in response to a nudge by @tbg in this comment.

I don't have particular stakes in this game, so if someone doesn't like what's happening in this PR, I will only work further if there's a concrete, actionable suggestion of what needs to happen instead. (i.e. I will not think about what's best and instead close the PR if it's unclear what I should be doing)

Thanks in advance.

knz

Ok I have been able to do a few useful things in response to Andrei's comments. Added some fixup commits at the end to avoid a humongous rebase. RFAL.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei, @gh-casper, and @RaduBerinde)

pkg/acceptance/localcluster/cluster.go, line 276 at r18 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

I think this testutils.MakeAmbientCtx() is bad news because it creates a Tracer internally. This was always fishy, but it became more brittle recently because I've made it illegal to mix spans created by different tracers within a single trace (I've had my reasons for doing this now that I'm trying to make tracing real; it was always pretty broken). And it's very easy to end up mixing tracers if you have more than one. So we should fight against creating tracers; as much as possible, there should be one tracer passed in everywhere.

Yes, I agree! And I am willing to help with that.
However, if at all possible I'd like to avoid doing so in this PR. The code as I'm leaving here is is no way worse than the original. We can take another pass in a later PR to replace all the calls to testutils.MakeAmbientCtx() by something better.

pkg/cli/start.go, line 766 at r15 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

are the other uses of ambientCtx in this method also broken?

I don't understand. The only one is at the beginning, before the Server instance is instantiated. Obviously at that point we don't have a server yet so we can't use s.AnnotateCtx.

We want s.AnnotateCtx here because it may have more details about the server than the ambientCtx at the beginning.

pkg/server/server.go, line 2932 at r22 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

it seems bizarre to me to expose an AmbientCtx. The AmbientCtx is kinda by nature internal I think; it's supposed to be used to background operations originating in the inner conmponent. Where are you using all these new methods? If they're indeed needed, consider squashing this commit with some uses.

This is only used used in tests, every time a test instantiates a component that needs the same AmbientContext.

It's much more readable to write s.AmbientCtx() than s.(*server.TestServer).Cfg.AmbientCtx.

I added a commit to rename this to TestingAmbientCtx to highlight the purpose.

pkg/util/log/ambient_context.go, line 194 at r16 (raw file):
So I think we need to have a separate constructor. The primary reason is that this sentence is false:

I think that an rpc.Context shouldn't even necessarily need an AmbientCtx (as it does now)

It does - the rpc.Context used by servers needs to annotate its contexts with the server identifiers and other background states. So we need an AmbientContext there in any case.

What is sad (but can't be fixed in this PR) is that we use the same rpc.Context type for both servers and clients. It's sad because clients don't need all the complexity we've built into rpc.Context, and would benefit from a much simpler type. We want to fix that, but then again not in this PR. Let's file an issue for this: #73967

However, I am sympathetic to the need to clarify this in ambient_context.go. I added some explanatory comment for this with a reference to the new issue.

pkg/util/log/ambient_context.go, line 199 at r17 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

What's "dummy" about it? I think this should be the same as MakeClientAmbientContext - simply MakeClientAmbientContext.

I agree the word "dummy" was mistaken. I meant to write "Testing" here - this is a constructor for use in tests. Added a commit to fix that. Good call.

pkg/testutils/lint/lint_test.go, line 569 at r29 (raw file):

please make sure there's a pragma that can be used to inhibit the linter at a call site.

I'm not sure that's necessary here. One can always use a constructor and modify the AC afterwards.

please add some comments to this linter, and the commit message, say why you're doing this
please make it more clear what's being prohibited - in a comment on the linter, and in the error message. I don't think it's obvious that constructing the struct is what's being rejected.

Good idea. Also added a reference to the tech note.

Also I added an additional commit, which replaces the linter altogether by a change to the type definition to make it non-constructible. I think it's even better this way.

knz · 2021-12-20T10:50:03Z

Is this good to go now?

Release note: None

… AmbientContext Release note: None

Release note: None

…rface` Release note: None

Release note: None

so that we avoid manual construction of AmbientContext. Release note: None

Release note: None

…iate Release note: None

Release note: None

@andreimatei

Recommended/suggested by @andreimatei. Makes it possible for rpc.Context to ignore the existence of `log.AmbientContext`. Release note: None

knz

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @andreimatei, @gh-casper, @RaduBerinde, and @stevendanna)

-- commits, line 13 at r37:

Previously, andreimatei (Andrei Matei) wrote…

consider dropping this commit now that you have the rpc,*: mandate a context.Context arg to rpc.NewContext one.

Done.

-- commits, line 32 at r40:

Previously, andreimatei (Andrei Matei) wrote…

consider dropping this commit now that you have the rpc,*: mandate a context.Context arg to rpc.NewContext one.

Done.

-- commits, line 57 at r45:

Previously, andreimatei (Andrei Matei) wrote…

I won't make a big deal out of this, but I'm not in favor of most of this commit. You're replacing a bunch of log.AmbientContext{Tracer: tracing.NewTracer()} with testutils.MakeAmbientCtx(). As discussed before, testutils.MakeAmbientCtx() is generally a bad idea because it creates a Tracer under the hood, and if this Tracer ends up actually being used, chances are it's going to crash because it'll end up mixing with some other Tracer. The code that you're modifying has the same problem - so either all these tracers don't actually end up being used (in which case it'd be better if we used the zero value AmbientContext{}), or it does end up being used but, by happenstance, the trace in question doesn't end up having more than one span.
On some other commit you've done a similar replacement on the argument that "it's not making the code any worse" than it already was. On a low scale, I don't care enough to argue, but on a large scale (i.e. this commit), the way I see it, these replacements are at best a lot of churn for no reason (at least I don't see the reason) or, at worse, a lot of churn that makes the code a little worse by slightly hiding the fact that a probably-unintended Tracer is being built.

Understood. I agree on principle, but just cancelling the commit altogether is a bit too much work - to do it properly, I might as well go through all the tests that use the API and figure out which tracer they'd better used.

So instead, I added a new commit which removes testutils.MakeAmbientCtx altogether with a comment about what needs to happen in the future.

pkg/sql/pgwire/fuzz.go, line 32 at r45 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

I think the zero value is better. As discussed elsewhere, testutils.MakeAmbientCtx() is generally bad because it creates a Tracer, and generally you want the Tracer passed in. You've introduced more uses of MakeAmbientCtx() instead of AmbientContext{Tracer: NewTracer()} on the argument that you're not making the code any worse than it was, but in this case you are making it worse in my opinion, as you're introducing a Tracer where there was none. When the zero value is sufficient, I think there's no reason not to use it.

Done.

pkg/testutils/localtestcluster/local_test_cluster.go, line 54 at r46 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

I personally would vote to not embed, and give the field a name instead. Embedding makes its harder to find uses of the thing, and I've found myself needing to find the uses of AmbientContexts - particularly to find people grabbing the Tracer.

Also, how come this is a pointer? That's unusual, instead it? If there's indeed a good reason for that, consider giving it a comment.

Also, I'm confused about why you've done this change. How come the LocalTestCluster? What's supposed to be in this AmbientCtx? Within a minute of complaining above that uses of embedded fields are not easy to find, I wanted to see who uses this, and so I've commented the field out and recompiled all the tests (took 30% of my battery lol). I see that it's only used in LocalTestCLuster.Start(), and so I'm not sure why this needs to be a field on the struct. If it doesn't need to be a field, I think we should avoid it.

It's used in txn_coord_sender_test.go.
But point taken, I moved it to a named field and removed the reference.

pkg/util/log/ambient_context.go, line 198 at r38 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

consider squashing this commit with the rename MakeDummyAmbientContexttoMakeTestingAmbientContext` one.

I would still like this to be called MakeAmbientContext, as there's nothing particularly Testing about it (particularly not from the perspective of this log library`, but I'll let it go.

I tried to squash them, but there's all the intermediate changes to introduce the call that don't play nice with the rebase.

pkg/util/log/ambient_context.go, line 208 at r41 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

for my curiosity, what are the "server identifier containers, if not the ServerIdentificationPayload? Consider adding more words to the comment hinting at why we'd want more than the ServerIdentificationPayload.

This was a stray comment, from before a rebase that added the idProvider to the argument list. Removed.

pkg/kv/kvclient/kvcoord/dist_sender_test.go, line 4578 at r46 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

I think this is backwards. We need to create the Tracer early, but we don't need to create the AmbientCtx early. I think the NewTracer() call was good here, and I think we should use MakeTestingAmbientCtx(tr) below, when creating the DistSenderConfig.

Good catch. Reverted.

pkg/kv/kvserver/allocator_test.go, line 7315 at r47 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

nit: I personally would prefer it if we left tr := NewTracer() here, and used MakeTestingAmbientContext(tr) twice below, once for rpcContext and once for storePool. The creation of those two structs doesn't need to share an AmbientCtx (particularly not an empty one), so I think it's better if they both create their own (so that the code doesn't suggest there's some coupling between them).

Done.

pkg/kv/kvserver/client_raft_test.go, line 3037 at r43 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

I'm not sure this is an improvement. I'm not immediately sure what this test does, but this raft transport runs kinda outside of the first node. Consider giving it a "test" log tag - either in addition to, or instead of, the node's tags.

Done.

pkg/testutils/testcluster/testcluster_test.go, line 210 at r48 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

is this tr used? I think this commit won't compile by itself.

Oops. Good catch. Fixed.

@andreimatei

This was recommended by @andreimatei. The name was hiding too much the fact it's creating a tracer under the hood. Removed in favor of a new `log.MakeTestingAmbientCtxWithNewTracer`, with an explanatory comment that indicates how this really should not be used. Release note: None

knz · 2021-12-27T22:04:48Z

TFYRs!

bors r=tbg,andreimatei

craig · 2021-12-27T22:40:20Z

Build failed:

GitHub CI (Cockroach)

knz · 2021-12-27T22:42:42Z

unrelated fail in logictest

bors r=tbg,andreimatei

craig · 2021-12-27T23:19:50Z

Build succeeded:

GitHub CI (Cockroach)

knz requested review from a team, RaduBerinde and tbg November 18, 2021 15:08

knz requested review from a team as code owners November 18, 2021 15:08

knz requested review from a team November 18, 2021 15:08

knz force-pushed the 20211118-ambient-context branch 3 times, most recently from 30bb583 to cefe9ba Compare November 18, 2021 16:16

tbg approved these changes Dec 14, 2021

View reviewed changes

knz force-pushed the 20211118-ambient-context branch 2 times, most recently from ef2310c to a7eb044 Compare December 16, 2021 13:34

knz mentioned this pull request Dec 16, 2021

*: avoid using background ctx in stopper.Run{Async,}Task{Ex,} #73915

Merged

andreimatei reviewed Dec 16, 2021

View reviewed changes

knz force-pushed the 20211118-ambient-context branch from a7eb044 to 935ed10 Compare December 17, 2021 12:33

knz requested review from a team as code owners December 17, 2021 12:33

knz requested review from gh-casper and removed request for a team December 17, 2021 12:33

knz commented Dec 17, 2021

View reviewed changes

knz force-pushed the 20211118-ambient-context branch 4 times, most recently from a77fe08 to b49a258 Compare December 17, 2021 17:24

knz added 14 commits December 27, 2021 17:08

cli/start: clarify AmbientCtx usage in comment

f271a0a

Release note: None

log: add a dummy ambient ctx constructor for tests

20e111d

Release note: None

acceptance,rpc: use test ambientcontexts where appropriate

ca2d31b

Release note: None

kvclient,kvserver,execinfrapb,server,log: use constructor for initial…

1e8d08e

… AmbientContext Release note: None

testutils: delegate MakeAmbientCtx to an ambientcontext constructor

0fa84f4

Release note: None

server,serverutils: add an AmbientCtx() accessor on `TestTenantInte…

b15a7a0

…rface` Release note: None

sql,kvtenantccl: use existing AmbientContexts where applicable in tests

73aac92

Release note: None

*: use testutils.MakeAmbientCtx() where appropriate

25f2f4b

so that we avoid manual construction of AmbientContext. Release note: None

kvcoord: properly reuse existing AmbientContexts in tests

0f48343

Release note: None

kvserver: reuse AmbientContexts where appropriate in tests

d8821f5

Release note: None

server,testcluster: use client AmbientContexts in tests where appropr…

68cbc56

…iate Release note: None

kvserver: avoid more naked AmbientContexts

9f86e0a

Release note: None

*: rename MakeDummyAmbientContext to MakeTestingAmbientContext

59ef14d

Release note: None

rpc,*: mandate a context.Context arg to rpc.NewContext

3f8c928

Recommended/suggested by @andreimatei. Makes it possible for rpc.Context to ignore the existence of `log.AmbientContext`. Release note: None

knz force-pushed the 20211118-ambient-context branch from 6d3bf16 to 92bdb2a Compare December 27, 2021 17:13

knz requested a review from a team as a code owner December 27, 2021 17:13

knz commented Dec 27, 2021

View reviewed changes

knz force-pushed the 20211118-ambient-context branch 2 times, most recently from ef0a933 to ab0c7b4 Compare December 27, 2021 18:28

knz force-pushed the 20211118-ambient-context branch from ab0c7b4 to ec06e9c Compare December 27, 2021 22:01

craig bot merged commit f3710ae into cockroachdb:master Dec 27, 2021

knz deleted the 20211118-ambient-context branch December 27, 2021 23:20

This was referenced Dec 28, 2021

log: do not require a Tracer in AmbientContext #74291

Closed

log: remove the redundant "eventLog" from AmbientContext #74294

Closed

Conversation

knz commented Nov 18, 2021

Uh oh!

cockroach-teamcity commented Nov 18, 2021

Uh oh!

tbg left a comment

Choose a reason for hiding this comment

Uh oh!

andreimatei Dec 16, 2021

Choose a reason for hiding this comment

Uh oh!

andreimatei Dec 16, 2021

Choose a reason for hiding this comment

Uh oh!

andreimatei Dec 16, 2021

Choose a reason for hiding this comment

Uh oh!

andreimatei left a comment

Choose a reason for hiding this comment

Uh oh!

knz commented Dec 16, 2021

Uh oh!

knz left a comment

Choose a reason for hiding this comment

Uh oh!

knz commented Dec 20, 2021

Uh oh!

knz left a comment

Choose a reason for hiding this comment

Uh oh!

knz commented Dec 27, 2021

Uh oh!

craig bot commented Dec 27, 2021

Uh oh!

knz commented Dec 27, 2021

Uh oh!

craig bot commented Dec 27, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants