Cockroach: all: proliferation of background contexts is awkward

[tamird]

This has been discussed ad-hoc before, but it's been bothering me so I'd like to start a real discussion.

Background:

• Logging functions now take a context as their first argument. This is nice, because it allows the logger to emit interesting information from the context, and also emit to the context's span, if one is present.
• Much of our existing code didn't (or doesn't) plumb contexts, so to support this new logging paradigm, we've been introducing "background" contexts - struct members which can be used from any method on the structure.

This issue is about the latter point above: these background contexts, while convenient storage for certain "background" information, cause some pretty awkward patterns. Namely:

• These contexts can never include a tracing span (since they aren't associated with any execution path specifically). This means that various functions may end up with a context devoid of a tracing span, which is currently "solved" by creating a span (and a tracer) from thin air in various random places (e.g. (*DistSender).sendChunk().
• The existence of these contexts make it impossible, in some places, to know if the context in hand is a "background" context or if it is one that is associated with the particular action. In other words, when do I need to call awkward methods like tracing.EnsureContext? It's anyone's guess.

I think we should strongly consider moving away from this pattern of getting contexts from thin air in random places. I propose we replace each of these stored contexts with a method which takes a context, annotates it, and returns a new context containing interesting metadata. This method can be optimized to return some internal context if context.TODO() is passed, but the internal context must be opaque to the caller.

Tagging people with opinions: @tschottdorf @RaduBerinde @andreimatei

[RaduBerinde]

I think this is a useful discussion. There is probably a way to improve our pattern and we should find it.

I am not quite sure what the proposed change would achieve though.

• In various components (by "component" I mean things like dist-sender, gossip, sql-server, etc), there are background tasks that are associated with the component itself and not any particular operation. For these operations, it _is_ legitimate to "start with" the component's background context. Yes, that makes things harder - it would be nice if everything we did was directly related to some operation that had a proper context, but that's just not the case. I am not sure how setting these operations up would look in your proposal, but it sounds like you would just call this new method and pass context.Background (because what else can you pass?) and then the method would (effectively) return the component's background context. If that's the case, this is equivalent with the status-quo, just with some extra fluff..
• For the cases where we do have a context, we already have established a pattern: use of a logContext method that annotates that context accordingly.

Starting spans in arbitrary places is indeed a problem, but I don't see how this proposal would help address that. To fix that we need to open spans as needed for any non-trivial "background" operations; this can't happen automatically as the caller has to Finish() the span as necessary. Even so, perhaps this is what we need: a model where the _only_ allowed way to get your hands on a "background" context also implies that a span is getting opened (and you are responsible for finishing it properly). Something like that will require a lot of changes, but is worth investigating.

If I am misunderstanding your proposal, it would be useful to write up the proposed signature and semantics of the proposed method, along with examples of how it would be used, especially for the cases of background tasks (one concrete example that is easy to think about is poll() in ts/db.go).

[tamird]

In various components (by "component" I mean things like dist-sender, gossip, sql-server, etc), there are background tasks that are associated with the component itself and not any particular operation. For these operations, it is legitimate to "start with" the component's background context. Yes, that makes things harder - it would be nice if everything we did was directly related to some operation that had a proper context, but that's just not the case. I am not sure how setting these operations up would look in your proposal, but it sounds like you would just call this new method and pass context.Background (because what else can you pass?) and then the method would (effectively) return the component's background context. If that's the case, this is equivalent with the status-quo, just with some extra fluff..

This is not equivalent to the status quo, because at least I can then easily grep around for context.Background() and find all the callers which create contexts out of thin air. Today, this is impossible.

For the cases where we do have a context, we already have established a pattern: use of a logContext method that annotates that context accordingly.

This pattern is fine, and if we only ever used that, I'd be happy, but the existence (and prolific presence, in callsites) of the "background" contexts makes it difficult to know what "non-static" payload that context can be expected to carry. In other words, it is impossible to depend on the property that passed contexts always contain tracers, because the caller could pass foo.Ctx (and that's much less greppable than context.{Background,TODO}() because of how often it is used).

Starting spans in arbitrary places is indeed a problem, but I don't see how this proposal would help address that. To fix that we need to open spans as needed for any non-trivial "background" operations; this can't happen automatically as the caller has to Finish() the span as necessary. Even so, perhaps this is what we need: a model where the only allowed way to get your hands on a "background" context also implies that a span is getting opened (and you are responsible for finishing it properly). Something like that will require a lot of changes, but is worth investigating.

Yeah, this is roughly where I think we should end up, and using the grpc interceptors will help with this (since we'll no longer manually need to create tracers there).

In any case, I'd like this discussion to focus on the contentious point which is that we should move away from the pattern of using ambient contexts everywhere; either you are passed a context, or you must call one of a narrow list of methods to get it.

[RaduBerinde]

The "greppable" issue is legitimate. In many cases we already use a Ctx() method, we could make that the standard everywhere (and rename it to a more particular name if Ctx is too short/common). Your proposal is ok as well.

In any case, I'd this discussion like to focus on the contentious point which is that we should move away from the pattern of using ambient contexts everywhere; either you are passed a context, or you must call one of a narrow list of methods to get it.

I agree with this.

[tbg]

I also agree. Background contexts came in handy and the world is better off for it right now, but I don't think we want to keep them for the above reasons mentioned. Any context that I can get my hands on should either come from the context package (and should promptly be filled with a span if one is desired) or is assumed one that the caller has suitably instrumented.

That dictates the pattern func Something(ctx context.Context, myStruct) context.Context), i.e. instead of (*Store).Ctx() (or (*Replica).ctx) code should read (mod names, etc)

ctx, done = tracing.Instrument(store.Context(context.Background())) // store.Background()?
defer done()
// go forth into the world, my context
db.Run(ctx, ba)

[RaduBerinde]

We could implement a helper type BackgroundCtx that we embed into server components. This struct holds the background context as a private member (so it's not accessible directly) and exports some methods:

• NewBackgroundCtx(opName string) (context.Context, opentracing.Span) returns a context derived from the "background" context with an open span.
• NewBackgroundCtxNoSpan() context.Context returns a (the?) "background" context. To be used sparingly, only when a span doesn't make sense. The name is intentionally unwieldy :)
• AnnotateCtx(ctx context.Context) context.Context - the equivalent of our current logContext pattern.

Thoughts, ideas?

[tamird]

As Tobias has said:

Any context that I can get my hands on should either come from the context package (and should promptly be filled with a span if one is desired) or is assumed one that the caller has suitably instrumented.

And I agree (and that's what this issue is really about); it should never be possible to get a context which is not derived from another context other than by calling context.{Background,TODO}.

So :-1: on NewBackgroundCtx{,NoSpan} but AnnotateCtx seems reasonable.

[RaduBerinde]

I thought the point of context.Background was that it's greppable, which is also the case with NewBackgroundCtx. Anwyay, I guess we can have two variants of AnnotateCtx, one that opens a span and one that doesn't, and you can pass context.Background to it. Let's first focus on what are the semantics we want and not whether you obtain them by calling X() or Y(context.Background()).

[tamird]

The semantics of AnnotateCtx as you described it above seem reasonable to me. Having a sister function which creates a span also seems good.

[RaduBerinde]

So leaving syntax aside, conceptually we want the following operations:

1. Annotate an existing context with the tags appropriate for the component.
2. Create a context with tags appropriate for the component and with an open span.
3. Create a context with tags appropriate for the component and no open span. (technically this would probably just return what is now the component's context, but that would be an implementation detail).

These are in decreasing order of preference, i.e. we should use 2 only if 1 doesn't make sense, and we should use 3 only if 1,2 don't make sense.

Is this right?

(edit: improved the wording of the above)

[RaduBerinde]

Do we also want: 1a. Annotate the context and open a span; if the context already has a span, the new span is a child of that?

[tamird]

I think the first stage should just tackle 1.. Again, I want to stress that I don't think we should do 2. or .3; the only way to obtain a context should be a function from the context package.

[RaduBerinde]

I am trying to have a functional description of the cases we need to solve. Can we not leave aside for a minute the issue of which exact functions you call for these cases? The cases I described don't need to map to functions, they could use one or two functions and you would pass context.Background() to get the described operation 2 for example.

I just want to first have an understanding of exactly what we need to solve. Or are you saying that you disagree that we need to devise a common pattern for cases 2 and 3 (in which case what do you propose for background operations?)

[tamird]

I see. For the moment, I would leave out the common pattern for background operations; my understanding is that the tools we already have, together with a facility that annotates a context with tags for the component (i.e. 1. above), are sufficient for servicing that use case.

In my mind this issue is about removing the widespread acquisition of ambient contexts; if we agree that this is a goal, subsequent extraction/cleanup/whatever can be discussed separately. Does that sounds reasonable to you?

[RaduBerinde]

Yes, we want to remove the widespread acquisition of ambient contexts (which maps to cases 2 and 3 above). To do this, we have to change the way we perform said acquisition (currently it is a trivial direct use of a stored context). And to change it, we have to decide what the "new" way will look like (i.e. establish a pattern). It sounds like what you are saying is: let's have an AnnotateCtx function, and use this for all the 3 cases above (annotating context.Background() for cases 2,3 above). This IS establishing a pattern, no?

My only (minor) problem with doing it this way is that currently grepping for context.Background (along with TODO) is how I've been identifying places where we haven't yet plumbed contexts at all. So now using context.Background for correctly written code for background operations will muddle that. This is why I was proposing to use some aptly named wrappers (like NewBackgroundCtx) for these cases. Although if in the "new" way, in practice context.Background() would always be an argument of AnnotateCtx, it should be easy to filter these "legitimate" uses out.

[tamird]

Well I think that in the short term the majority of calls to AnnotateCtx will use context.TODO(). In the long term, only background operations will use context.Background(), so there should be only a small number of such cases (i.e. the queues).

Have you found that there are many instances of context.Background() which represent lack of plumbing? If that's the case, we should probably replace all of those with context.TODO() before we go down this road, and then we can rely on the property that context.Background() is the only "legitimate" way of acquiring a context and only used for background operations.

In other words:

Although if in the "new" way, in practice context.Background() would always be an argument of AnnotateCtx, it should be easy to filter these "legitimate" uses out.

is indeed what I had in mind.

[RaduBerinde]

Have you found that there are many instances of context.Background() which represent lack of plumbing? If that's the case, we should probably replace all of those with context.TODO() before we go down this road, and then we can rely on the property that context.Background() is the only "legitimate" way of acquiring a context and only used for background operations.

Yeah, I fixed quite a few (not sure how many are left). Your suggestion makes sense (and addresses my concern).

Well I think that in the short term the majority of calls to AnnotateCtx will use context.TODO().

I think TODO should be used for the cases where there is some plumbing missing. Going back to the concrete example I mentioned before - poll() in ts/db.go, that should use Background, right?

[tamird]

I think TODO should be used for the cases where there is some plumbing missing. Going back to the concrete example I mentioned before - poll() in ts/db.go, that should use Background, right?

Yeah, that's my thinking.

[RaduBerinde]

Great. I think everything's clear and we're in agreement.

[RaduBerinde]

All the major components have been refactored so I am closing this issue. There are a few smaller components where there isn't a clear win in converting; some of these can be converted going forward.