Support injecting a context in to a response

[nikclayton-dfinity]

The latest W3C draft supports injecting the trace context in to an HTTP response.

https://w3c.github.io/trace-context/#trace-context-http-response-headers-format

This is the same format as the request context, except:

1. There's no tracestate header.
2. The header with the context is called traceresponse

Suggest:

1. Adding an inject_response_context method to TextMapPropagator that implements this.
2. Rename inject_context to inject_request_context
3. Keep inject_context as a (deprecated) wrapper around inject_request_context that can be removed in the next release that allows breaking changes

Quick and dirty implementation (where the common code between this and inject_context should obviously be factored out).

/// Inject the current span context for use in a response.
///
/// This follows the current W3C draft proposal for a `traceresponse`
/// header, see https://w3c.github.io/trace-context/#trace-context-http-response-headers-format
fn inject_response_context(cx: &opentelemetry::Context, injector: &mut dyn opentelemetry::propagation::Injector) {
    use opentelemetry::trace::{TraceContextExt, TRACE_FLAG_SAMPLED};

    const TRACERESPONSE_HEADER: &str = "traceresponse";
    const SUPPORTED_VERSION: u8 = 0;

    let span = cx.span();
    let span_context = span.span_context();
    if span_context.is_valid() {
        let header_value = format!(
            "{:02x}-{:032x}-{:016x}-{:02x}",
            SUPPORTED_VERSION,
            span_context.trace_id().to_u128(),
            span_context.span_id().to_u64(),
            span_context.trace_flags() & TRACE_FLAG_SAMPLED
        );
        injector.set(TRACERESPONSE_HEADER, header_value);
    }
}

[jtescher]

I like the idea of more fully supporting the trace-context spec, the interaction for the text map propagator is fairly explicit in the propagators spec which is a little unfortunate, perhaps we could add an option behind a flag, or come up with a non conflicting (and ideally not confusing) API to add here 🤔

[nikclayton-dfinity]

Having read that doc, I think this could be done in a way that doesn't violate the stability guarantees (as defined in https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/versioning-and-stability.md#stable), in particular, in a way that doesn't violate backwards compatibility.

My detailed argument runs goes like this (and, acknowledging that traceresponse is still a W3C draft):

1a. The first para of that doc says

Cross-cutting concerns send their state to the next process using Propagators, which are defined as objects used to read and write context data to and from messages exchanged by the applications.

I think it's reasonable to assert that the HTTP response is part of the "messages exchanged by the applications", so traceresponse is in scope for this.

1b. Going through the mental exercise of search/replace "request" in that doc with "request or response" (pluralise as appropriate) and nothing seems to break.

1c. The general Carrier type is agnostic about whether the carrier is part of a request or a response.

1d. In the TextMap Propagator section it notes:

TextMapPropagator performs the injection and extraction of a cross-cutting concern value as string key/values pairs into carriers that travel in-band across process boundaries.

An HTTP response fits the definition of "carriers that travel in-band across process boundaries".

The next para is:

The carrier of propagated data on both the client (injector) and server (extractor) side is usually an HTTP request.

"usually" in that sentence is doing a bit of heavy lifting.

1e. Jumping back in the doc slightly, I think the key problem is the definition of the Inject and Extract operations, which don't specify whether the carrier is a request or a response.

So I think you could make a backwards-compatible change to this document by defining InjectRequest, ExtractRequest, InjectResponse, and ExtractResponse operations, and redefine Inject and Extract as deprecated synonyms for InjectRequest and ExtractRequest.

(A non-backwards compatible change would be extend Inject and Extract to require more details about the carrier, but I'm not suggesting doing that).

2. Alternatively, leave the TextMapPropagator spec untouched, copy it to ResponseTextMapPropagator, and s/request/response` throughout the document.

3. Or, leave it completely untouched, but treat it as the specification for an abstract base class, and implement concrete HttpRequestPropagator and HttpResponsePropagator classes instead.

Thinking about that -- option 3 might actually be the best. Apart from anything else, the future work that's described to implement a binary propagator type fits completely in to that framework. Instead of a TextMapPropagator (which in reality is really an HttpPropagator in a trenchcoat and sunglasses) and a BinaryMapPropagator you'd have a Http{Request,Response}Propagator, a GrpcRequestPropagator, a ThriftRequestPropagator, etc.

[TommyCpp]

Thanks for the detailed analysis. Personally, I feel most comfortable adding another ResponseTextMapPropagator to handle the response ingestion. Mostly because I think usually we inject at the request side to make sure the tracing context can be used on HTTP server-side.

Another method could be allowing the user to configure how TraceContextPropagator inject. So basically have a bool configure(or enum) set in TraceContextPropagator to determine whether we gonna ingest into request and response.

[wperron]

Hey folks 👋 A short while ago I implemented the traceresponse propagator for opentelemetry-ruby and I hit the same issue here where Inject and Extract acted on http headers, which is the same data type on both the request and response types. Looking at the HeaderExtractor from the opentelemetry_http crate, this is much the same situation here.

The way we went around it is by doing a new ResponseTextMapPropagator just like @TommyCpp is suggesting. In the case of either a client or a server, you end up needing to use a different propagator to deal with the Request or Response, but on the flip side it avoids the need to change any API and reuses Inject and Extract just like any propagator.

The implementation is available in the sdk_experimental module in opentelemetry-ruby. I'd be more than happy to contribute this here too if that's valuable.

[jtescher]

I think adding this to the contrib package would be reasonable for now as it's not yet worked out in the spec.

[wperron]

Working on a PR right now -- Something else that might be interesting would be to introduce an experimental feature rather than sticking this in the contrib crate, wdyt?

[jtescher]

IMO features should be under an experimental flag if they are experimental in the spec, if there is no spec for them yet they should be in contrib for now.

[wperron]

Maybe that's my misunderstanding of the Editor's Draft version of the spec then, I always assumed it meant "experimental" 🤔

[jtescher]

Ah more specifically a spec in https://github.com/open-telemetry/opentelemetry-specification

[wperron]

@jtescher I've got a PR open here, only needs your rubber stamp!