Instrumentation hooks for trace context propagation

[binaryseed]

I'm interested in seeing telemetry extended with an API to enable an additional type of instrumentation - the mutation of an outbound request / query with Distributed Trace context information.

The general API could be implemented as an instrumentation hook that a library could build into their code that enables telemetry handlers to mutate specific data structures.

Taking HTTP requests as an example, we could imagine an HTTP client library adding telemetry hooks:

defmodule HttpClient do
  def request(url, body, headers) do
    extra_headers =
      :telemetry.decorate(
        [:httpoison, :request, :headers],
        %{url: url, body: body, headers: headers}
      )

    validate(extra_headers)
    do_request(url, body, headers ++ extra_headers)
  end
end

Then a handler could attach to [:httpoison, :request, :headers], and do some work in the handler...

defmodule MyTelemetryHandler do
  def handle([:httpoison, :request, :headers], request) do
    context = get_trace_context(request)
    generate_distributed_tracing_headers(context)
  end
end

This would provide a general mechanism for outbound instrumentation, without hard-wiring to any vendor-specific instrumentation details.

Very open to discussions about strategy and naming!

Issues to sort out:

• Clarify naming for the various parts of the puzzle
• Account for collecting annotations from multiple handlers
• Have a mechanism or pattern for validating the data returned from handlers

For context, this issue has been discussed before in #24 and beam-telemetry/telemetry_metrics#46

[binaryseed]

In my mind, telemetry is a great place for this. It's the library that we're asking all other libraries to leverage to instrument themselves. I see is as the most neutral thing possible that could accomplish this. Really it's just a thin event dispatch mechanism that doesn't try to do much beyond being the connection point between the instrumented library and the consumer of that instrumentation.

I think it's great that OpenTelemetry is trying to standardize more in this space but I also think the reality of the situation is that those standards are not stabilized, and who knows what it'll look like in 5 years. Given that, I think it's even more powerful for telemetry to be fairly agnostic about things, and leave most of the work to the handlers themselves.

This gives us the best chance that the instrumentation itself is stable and can live on (since it's the thing that will be most interwoven in numerous codebases across the ecosystem), while the handlers can evolve to match the latest standards and best practices for observability.

For example - imagine the trace-context header changes format in 2 years. I'd much prefer if the open-telemetry handler was the only thing that needed to change to support that, rather than every single HTTP library having to be updated.

[tsloughter]

This opentelemetry RFC might be useful when designing this open-telemetry/oteps#42

I'm not sure it belongs in telemetry, but also don't have a suggested alternative that isn't a new context library which I'm also not sold on.

[binaryseed]

Why wouldn't it belong in telemetry?

I don't think there's much to this API really - it's not trying to solve all the problems regarding context propagation, it's simply providing instrumentation hooks that can be widely adopted such that it's easy for a people to build & evolve solutions to context propagation w/o requiring code changes throughout the ecosystem.

[ludwikbukowski]

So from more technical point of view;
the difference between such :telemetry.decorate and :telemetry.execute would be the fact that the first one returns some modified state?

[binaryseed]

one returns some modified state

Exactly

[ludwikbukowski]

Assuming that such :telemetry.decorate behaves like regular fold function.

1. In case of failure one of the handlers, normal :telemetry.execute call simply logs the error and detach the buggy handler.
   For the :telemetry.decorate, What would I expect:
   a) the whole decorate call fails
   b) the whole decorate returns untouched state and logs error
   c) apply the decoration for all handlers except the buggy one, log error

I think option c) fits the best the tracing use case because it does not break the critical path of the call and still applies other handlers.

2. Telemetry would not guarantee the order of the handlers, right?

[tsloughter]

decorate is "context" functionality. Maybe telemetry should be an abstraction for context that projects can share, but if so I don't think it can just be the addition of decorate. If it is going to become the "context" library for Erlang/Elixir we should discuss how that would look.

[tsloughter]

I misunderstood this at first. Based on the Observability WG meeting discussion I know understand this to be like if an HTTP library had a callback add_headers that you could register with to have it update the headers with trace context instead of first calling a function that updates the headers and then passing those to the HTTP client. Correct?

[josevalim]

My reservation on this proposal is that it would not be a good idea for libraries, such as an HTTP client, to assume that we want to pass context forward. So I think that it has be explicitly passed in. There may still be a need for standardization but I think it would be a bit harmful if libraries start assuming all context is shared.

[bryannaegele]

I'm inclined to agree with José on this. How would the client be able to discern at the library level when you intend to propagate context within your systems versus external calls where you may not want to leak that? It would seem that explicitly declaring behavior at the client implementation level gives the most control. I used the separate clients strategy in our systems and it worked well.

[GregMefford]

I agree that I would not want context injected in all HTTP calls. I would want to have to do it explicitly, or perhaps by using a special client module where I clearly know that it has this behavior. One pretty common pattern is to build a module in your app code that’s specifically for talking to another external service. That gives a pretty easy point at which you can decide whether you want to inject context for that particular call. For example: https://github.com/spandex-project/spandex_example/blob/master/plug_gateway/lib/plug_gateway/backend_client.ex#L8

[binaryseed]

The approach the last few of you point to is definitely workable, but it foregoes any solution that works "out of the box"

New Relic agents try to wire these types of things up for the user to make it such that achieving observability is as easy as possible.

We take care of any security concerns by:

• Not passing anything sensitive in the context header (ie: just a few IDs that are inconsequential to pass to any arbitrary server)
• Validate if the source of the request is "trusted" along each hop before connecting a Distributed Trace

Given the push-back, I'm assuming that with other systems, people are passing sensitive information in the trace context itself? That itself feels dangerous to me but perhaps it's a pattern out there that people use?

Leaving it all up to the user presents challenges as soon as we move outside of HTTP. Take for example database calls. There's no concept of "headers" so what some tracing packages do is add a comment to the SQL query itself that serializes the context (ex: https://docs.vividcortex.com/general-reference/query-tags/). I'm not clear how that would be achieved by the end user...

[GregMefford]

I think most of the time, there's nothing particularly sensitive in the trace context (just trace/span IDs). It's possible to include arbitrary "baggage" data which would probably be more sensitive, so there would need to be a way to only return the non-sensitive information at least, which feels like it adds some complexity.

I think the main issue I have is that I typically don't want magic things to happen that I didn't explicitly ask for and can't easily tell where they're coming from (this is one of the main reasons I like Elixir), so I prefer to have to manually inject trace context where I want it. That said, I think I'm getting off-topic on this thread because that's only one possible use for this proposed API.

My main concern about the proposed API itself (as opposed to the intended use-case), is that it feels weird and potentially hard to troubleshoot what's going on when you're making a blind call to some unknown number of functions that have registered themselves to listen to that hook and have an opportunity to transform the data somehow and in an undefined order. By comparison, it would be a lot more clear what's going on if the HTTP client did something like how Tesla supports middleware for injecting trace context in request headers, or parsing some kind of context or metadata from the response.

[binaryseed]

it would be a lot more clear what's going on if the HTTP client did something like how Tesla supports middleware

That's true but it doesn't seem within the scope of telemetry to try and enforce a middleware pattern on any library that wants to add this kind of instrumentation...

I see that there's a variety of concerns with this, I'm going to ponder this for a while and see if I can come up with another approach