search

slashmo / gsoc-swift-baggage-context

BaggageContext library, extracted from slashmo/gsoc-swift-tracing
https://github.com/slashmo/gsoc-swift-tracing
Apache License 2.0
4 stars 2 forks source link

Revisiting naming #23

Closed ktoso closed 3 years ago

ktoso commented 3 years ago

We should revisit naming again before calling it final.

Considerations:

Baggage header is used to propagate properties not defined in Trace-Parent. There are two common use cases. First is to define a context on trace initiation. Such context will have customer's identity, high-level operation name and other properties like a flight name. Second use case is to pass the caller's name to the next component. This name-value pair will be overridden by every service to it's own name.

ktoso commented 3 years ago

cc @tanner0101 @tomerd

I will ping "everyone" else once this has an acompanying PR, though it may take me a week to get the time to do this.

tanner0101 commented 3 years ago

+1 to just Baggage.

For the protocols, what about something like this:

protocol BaggageContext {
    var baggage: Baggage { get }
}

protocol LoggingContext {
    var logger: Logger { get }
}

struct DefaultContext: BaggageContext, LoggingContext {
    var logger: Logger
    var baggage: Baggage
}
// Everything has ...Context suffix, nice
typealias MyContext = BaggageContext & LoggerContext

One thing that sticks out to me is that Logg_ing_Context stores a Logger but BaggageContext stores just Baggage. I feel like the equivalent name for the baggage context would be something like TracingContext? I don't like that name at all, just to elucidate the interesting break in consistency here.

Finally, I think the module name should be just import Context and the repo called swift-context. This library helps us build our own composable context types so I think it makes sense. Using a concise name like this also makes it clear this is a foundational library that everyone should be using. (If one part of the chain doesn't use it, everything breaks down).

ktoso commented 3 years ago

That's a pretty good idea actually, let's mull over it a little bit but it looks like a good simplification to me 👍

One thing that sticks out to me is that Logg_ing_Context stores a Logg_er_ but BaggageContext stores just Baggage.

I feel like the equivalent name for the baggage context would be something like TracingContext? I don't like that name at all, just to elucidate the interesting break in consistency here.

So TraceContext is also a very specific thing:

So I would not want to jump on that name. Our type is more general and also not necessarily containing tracing information (though that is it's main use case).

I am not sure about separating LoggerContext without a baggage, I'd rather:

protocol BaggageContext {
    var baggage: Baggage { get }
}

protocol BaggageLoggingContext: BaggageContext {
    var logger: Logger { get }
}

struct DefaultContext: BaggageLoggingContext {
    var logger: Logger
    var baggage: Baggage
}

because implementing the Logger is not naive it is smart: 

public struct DefaultContext: BaggageLoggingContext {
    private var baseLogger: Logger
    public var baggage: BaggageContext

    public var logger: Logger {
        get {
            return self.baseLogger.with(context: self.baggage)
        }
        set {
            self._logger = newValue
        }
    }

    public init(logger baseLogger: Logger, baggage: BaggageContext) {
        self.baseLogger = baseLogger
        self.baggage = baggage
    }
}

Given such impl, the "default context" always does the right thing -- includes the latest context when necessary.

Baggage in some tracing implementations is stored inside a tracer's context and they'd encourage everyone to "just use our trace context"; That's slightly backwards but the idea is the same. I.e. Jaeger tracers have a context and offer setBaggageitem however, those are equivalent to our tracing Span attributes, and the same as OTel attributes. While "OTel Baggage" is also referred to and propagated through using CorrelationContext in OTel wording:

While CorrelationContext can be used to prototype other cross-cutting concerns, this mechanism is primarily intended to convey values for the OpenTelemetry observability systems. [...] (exactly what we want to do with our Baggage, not only tracing)

These values can be consumed from CorrelationContext and used as additional dimensions for metrics, or additional context for logs and traces. Some examples:

  • a web service can benefit from including context around what service has sent the request
  • a SaaS provider can include context about the API user or token that is responsible for that request
  • determining that a particular browser version is associated with a failure in an image processing service

[...] For backward compatibility with OpenTracing, Baggage is propagated as CorrelationContext when using the OpenTracing bridge. New concerns with different criteria should consider creating a new cross-cutting concern to cover their use-case; they may benefit from the W3C encoding format but use a new HTTP header to convey data throughout a distributed trace.

https://github.com/open-telemetry/opentelemetry-specification/blob/609a63784b2da7d59efe2a39822a23b09ae5b637/specification/overview.md#correlationcontext

So what we have really with our BaggageContext (present day name) is what one might want to call a CorrelationContext in OTel wording.

Overall this seems like a promising direction 👍

I think it would work out fine if we use Baggage as the type to be honest though and your proposed Context protocols, because we can get from a Baggage to a Span, and a Span has attributes. This aligns pretty well with OTel without being strictly tied to it. We can provide shims for CorerlationContext and similar in extra packages if necessary.

tomerd commented 3 years ago

Random side question; so we want logger not log for that property?

I personally prefer logger

ktoso commented 3 years ago

Sounds good 👍 Since log is more DSLish and if someone wants that they can always add it... :)