Add accessor functions for ochttp's addedTagsKey

[basvanbeek]

Is your feature request related to a problem? Please describe.

The current route tagging logic only works for routers supporting http.Handler. Some routers, like the one from gin do not support this. Because of this a large part of ochttp would need to be copied to a specific gin middleware (as demonstrated by @sagikazarmark in https://github.com/sagikazarmark/go-gin-gorm-opencensus).

Describe the solution you'd like
To allow usage of standard ochttp on the outer shell with tag.Mutator access from within gin handlers we could provide standard context accessors for the addedTags slice and make gin integration much easier with our standard plugin.

[sagikazarmark]

I would be happy to provide a PR if you are okay with providing access to the tag mutator.

[semistrict]

I would prefer to generalize this slightly differently. Instead of exposing the general ability to add tags (which is more powerful than required), let's rename WithRouteTag to WithRoute so that we can re-use it for adding the route to the Span as the http.route attribute. We can then provide a GetRoute(context) function.

[sagikazarmark]

That wouldn't solve the problem as the route would only become available later:

ochttp -> custom routing strategy -> add route info to span

[semistrict]

What if we extracted the logic for HTTP request processing, something like:

func (h *ochttp.Handler)  Start(req *http.Request) *http.Request {
...
  return req.WithContext(newCtx)
}

func (h *ochttp.Handler) End(req *http.Request, route string) {
  span := trace.FromContext(req)
  span.AddAttribute(trace.StringAttribute("http.route", route))
  // add route as tag and call stats.RecordWithTag
}

Then frameworks could construct an appropriate http.Request and call Start and End directly (rather than relying on Handle).

[sagikazarmark]

That could work, but it would also mean that ochttp cannot the be most outer layer of the http request, so that the amount of instrumented code would possibly be less.

What if we don't expose the whole tag adding layer, just add a method that adds the route to the tag list in the context?

[semistrict]

Yeah, that's fine I think. We should just note that it isn't safe to call it from a separate goroutine. Can we call it ochttp.SetRoute(req, route) and have it set the http.route attribute on the associated span as well?

[sagikazarmark]

I think it should accept (and return) a context. Context propagation is not necessarily tied to an http request within the framework boundaries.

What should it do if a tag list is not available in the context? Create it?

[semistrict]

Yeah, I can imagine that there would be cases where you might not have convenient access to the http.Request so using context seems fine.

If the tag list is not available there is no point creating it because the context that you return with the newly added tag list will not really be used for anything. Come to think of it, the function should probably not return a context, since it isn't adding anything to the context but rather internally mutating it.

so maybe the signature should just be:

func SetRoute(ctx context.Context, route string)

If the tag list is not in the context that means the ochttp.Handler was not installed correctly. I would be inclined to log an error sync.Once and ignore. The alternative would be to panic which seems too drastic.

[sagikazarmark]

Come to think of it, the function should probably not return a context, since it isn't adding anything to the context but rather internally mutating it.

I agree, it was only an option if we decided to add tag list to the context if it doesn't exist.

The signature looks good.

[sagikazarmark]

Is there anything I can do to move things forward here?