Feels like we're solving something that the ID containers were already supposed to solve. They were designed so that you can use them as log tag values before we have the actual IDs.

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

pkg/server/server.go, line 290 at r3 (raw file):

	// bootstrapped; otherwise a new one is allocated in Node.
	nodeIDContainer := &base.NodeIDContainer{}
	cfg.AmbientCtx.AddSharedLogTag("n", nodeIDContainer)

Why is setting the log tag here too late? What components have used the AmbientCtx already?

Regardless, we can allocate the containers as early as we need - eg in this case we could move this whole thing in MakeBaseConfig and store the container in BaseConfig.

pkg/util/log/ambient_context.go, line 78 at r2 (raw file):

	// 1. the initialization of the **logtags.Buffer pointer, from nil to non-nil.
	// 2. the initialization/update of the tags stored as *logtags.Buffer.
	// 3. accesses to the shared tags.

We modify the pointer concurrently with accessing it. We are relying on loading/storing the pointer being atomic, but if we're not explicit about that it will piss off the race detector. We should use atomic.Value (I doubt it will be slower in any observable way) or LoadPointer/StorePointer.

pkg/util/log/ambient_context.go, line 92 at r2 (raw file):

	// Cached annotated version of context.{TODO,Background}, to avoid annotating
	// these contexts repeatedly.
	backgroundCtx context.Context

The shared tags break this cached value.

we should link the mutable containers for sqli and nodeid into the base config when the root AmbientCtx is created.

What you're not seeing (and what I see) is that once a key/value pair has been pushed into the tags, then its key will be displayed everywhere even if the value is not known yet. I wish to set up the code such that a server has either the tag key n, or the tag key sqli, but not both.

As of today, we use the same server.Config / BaseConfig init code (With the same AmbientContext initialization) for both regular nodes and SQL instances.

One is the global one set up to anchor all the command line arguments (cli/context.go, serverCfg) and the other is for test servers (server/testserver.go, makeTestBaseConfig).

The determination of whether a given server.Config (and its AmbientContext) is used for a regular node or a SQL instance is done much later, when the server is actually instantiated.

We can't pull the AmbientContext field away from BaseConfig to have separate ACs, one in KVConfig and SQLConfig, because server.Config inherits from both and it would become ambiguous.

So what gives? The solution I'm offering here is extremely narrow in terms of code changed (and code to be reviewed), whereas a solution like you suggest would amount to a larger refactoring.

Is that a good use of time right now?

We could also make things even more explicit by making the NodeID/InstanceID required arguments for instantiating an AmbientContext. This runs counter to how the zero value can be used today, but I think what we're seeing is that at least within CRDB we basically don't want to use the zero value.

I've pulled this idea away into a separate issue #72815, and tried starting working on it in #72912, but it's a large amount of work.

Is that a good use of time right now?

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

pkg/server/server.go, line 290 at r3 (raw file):

see my comment above

pkg/util/log/ambient_context.go, line 78 at r2 (raw file):

This is incorrect. The pointer store is done in 1 goroutine before any of the other accessor goroutines ever read from it. There's no race.

pkg/util/log/ambient_context.go, line 92 at r2 (raw file):

That's incorrect - the cached context only uses the private tags, and the annotate methods pick up the shared tags.

What you're not seeing (and what I see) is that once a key/value pair has been pushed into the tags, then its key will be displayed everywhere even if the value is not known yet. I wish to set up the code such that a server has either the tag key n, or the tag key sqli, but not both.

This has a simple solution - we can define an interface in logtags:

type OptionalTagValue struct {
   ShouldShowTag() obol
}

The formatting code can check if values implement that and consult the method, and if it returns false the tag is not shown. The node containers would implement this.

extremely narrow in terms of code changed

It's not just a matter of lines of code changed - this introduces a concept that is much more complicated than the otherwise simple AmbientCtx model. It involves structures that are modified asynchronously and subtrees that share stuff.

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

Alternatively, we could use a single container if we can accept both tag keys to begin with n (eg n1 for nodes and nsql1). Then it's only a matter of the container printing 1 or sql.

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