Support for introducing custom tags to gRPC/HTTP metrics

[semistrict]

Original discussion at: https://github.com/census-instrumentation/opencensus-go/issues/746

Consider the example of a server that serves results either from database or a cache. One important dimension you might want to capture as part of all your HTTP/gRPC metrics is whether a particular request was served from cache or not.

Currently it isn't possible to augment the set of tags applied to the stats.Record call made by ocgrpc or ochttp. In OpenCensus Java I believe it is possible to accomplish this by changing the thread-local tags in the thread processing of the request but in Go, Context is immutable so a similar approach is not possible.

[semistrict]

I think this is really critical to making the HTTP and gRPC measures useful. Without this, especially the HTTP measures have too little context to really make them useful for debugging issues.

[bogdandrutu]

Do you have a proposal for this API? I agree that we need the functionality.

[semistrict]

We could make the tag map mutable e.g.

tag.FromContext(ctx).Mutate(tag.Insert(key, val), tag.Upsert(key2, val2))

I think this is already the case in Java, right? You can replace the Thread-local tags at any time?

[bogdandrutu]

The tag map is not mutable in java because that causes a lot of issues. also in java you cannot replace the Thread-Local tags at any time unless you use a scope mechanism which means you cannot change backwards which is what you need.

The reason why you cannot is because the grpc.Context will log.sever if detach an unexpected context. See https://github.com/grpc/grpc-java/blob/master/context/src/main/java/io/grpc/ThreadLocalContextStorage.java#L46

[semistrict]

I see. Well, in that case I propose that we make the tag map mutable in all languages. The only alternative I can think of would be to store the tags used for recording RPC/HTTP stats somewhere else and make that mutable.

[semistrict]

So something like:

ochttp.MutateTags(ctx, tag.Upsert(...), tag.Upsert(...))

or

ocgrpc.MutateTags(ctx, tag.Upsert(...), tag.Upsert(...))

[bogdandrutu]

That comes with tons of problems, imagine 2 threads (go routines) sharing the same context. Thread 1 changes the TagMap, thread 2 records measurements. You will have undefined which is very bad.

[semistrict]

The concurrency problems with the second approach are no worse than those that come with updating Span attributes (for example).

[semistrict]

/cc @dinooliva

[rakyll]

Mutations of context value introduces race conditions. Even though, Go sets no restrictions on value lifecycle, allowing user to mutate them breaks the concept of derived contexts.

MutateTags cannot be a public API. User cannot arbitrarily call this API and introduce race conditions.

Instead of mutating, we may use hooks so they can extend the current context.

[rakyll]

The concurrency problems with the second approach are no worse than those that come with updating Span attributes

This is not the entire picture. Context APIs have some patterns and anti-patterns. Mutating the value is introducing an anti-pattern to satisfy a feature. When we are mutating an object of our own type, we are not suffering from the earlier presumptions community might have about the type.

[codefromthecrypt]

question: what about the idea of scheduling an update for the next scope application. for example, instead of mutating what is there, appending a change (understanding this itself implies some concurrency work)

from a user POV, if the api is obvious.. they know that to make it effective now, they should immediately re-scope.

such a change list could be tossed if the scope is never reapplied. in writing this I am thinking that it sounds inelegant but anyway for devil's advocate sake.

[semistrict]

Agree that we should drop the idea of mutating tags in general - let's forget about that.

@rakyll regarding the second approach (just adding tags to the top-level Record), I do see your point about context anti-patterns and immutability. In my opinion this is outweighed by the usefulness of this feature.

When we are mutating an object of our own type, we are not suffering from the earlier presumptions community might have about the type.

I don't mean that we should necessarily support directly mutating it. It could be behind a method or even (as you seem to be suggesting below) a closure over the state being mutated that we store in the context.

Instead of mutating, we may use hooks so they can extend the current context.

I don't know if I am understanding this suggesting correctly. Do you mean that we would put a func into the context that internally would mutate something it closes over? If so, this sounds like potentially a nice API for this feature and I think it would work. But I would leave this choice up to what's idiomatic in the language.

[semistrict]

@adriancole I don't really understand what you're suggesting. Do you mean that we somehow signal upwards to "restart" request processing just because we want to add a tag?

Also: we need to consider how this will be implemented in languages other than just Go. I think the concerns are analogous but would be helpful to have comments from someone who knows better (@bogdandrutu?)