Guarantee LoggingBaggageContextCarrier Logger metadata includes baggage

[adam-fowler]

With the current implementation of LoggingBaggageContextCarrier there is no guarantee that the Logger it holds includes the baggage in it's metadata. It is down to the library author to ensure this is the case.

If I accept a LoggingBaggageContextCarrier as a parameter to an interface function I cannot guarantee when I ask for the logger I will get a logger with metadata augmented with the baggage. Maybe it would be better to make it clearer and add a loggerWithBaggage variable as an extension instead of assuming the baggage will be added to the Logger metadata.

extension LoggingBaggageContextCarrier {
    var loggerWithBaggage: Logger { logger.with(context: self.baggage) }
}

[ktoso]

I know what you mean, it is up to the context's implementor to implement the context object "right" and you don't trust "them" (whoever implements the context object). That's fair but making the dance more and more explicit esp if we have logger + loggerWithBaggage we make it harder for users they have to think what they call etc.

End users should not be forced to think, ever, they should have only one thing to call and it should do the right thing. This is the rationale behind offering only var logger on the protocol.

It is better for force "do the right thing" on a few library authors, than every single use-site of such library. There are many users of swift who are new to a) programming b) swift or c) server systems, and they already have a lot to think about -- let's make the "i got some logger here, i'll just use it" the obvious and right thing to do. No more thinking if it's with or without baggage etc. The contexts should do the right thing.

[adam-fowler]

What is the end user going to be doing more though? Is it accessing the contents of a BaggageContext or creating BaggageContext to be passed down to another system. If it is the second then there is more of a chance they will create their own BaggageContext struct and then we have to trust they will set it up correctly. I'm not sure we can limit the creation of these objects to a few library authors.

One other issue to take into account would be if I have a BaggageContextCarrier generated by library A and I want to generate the BaggageContextCarrier required for library B which includes additional context (maybe an EventLoop). I only have access to the Logger augmented with additional metadata from the BaggageContext in library A. So I would end up creating the library B BaggageContextCarrier with an augmented Logger plus the original BaggageContext. Library B BaggageContextCarrier would be carrying library A BaggageContext in both the Logger metadata and its BaggageContext. And when you ask for the Logger from library B it will attempt to augment an already augmented Logger. I hope that makes sense.

[ktoso]

What is the end user going to be doing more though? Is it accessing the contents of a BaggageContext or creating BaggageContext to be passed down to another system.

Most often? Neither really: just keep passing it along.

You get a baggage context on the "outermost" layer of some "call" or from a framework and you keep passing it around.

At the "edge", meaning in an http request "handler" or in the "cli app main()" or somewhere like that you make a decision:

• okey, I'm in some "background" or "context less" setting, thus i need to create a context
  • that's where DefaultContext and .background and .TODO come into play, see tickets Revisiting naming #23 and Semantically helpfully named empty baggage factory functions #26 (26 specifically for this one)
• oh, I'm in some "call" already thus I was given a context: I'm in an http request handler, grpc request handler, queue handler, some callback that has context etc etc.
  • thus I get that and pass it through functions I call.

If it is the second then there is more of a chance they will create their own BaggageContext struct and then we have to trust they will set it up correctly.

The BaggageLogging library would ship with DefaultContext which does this dance, so users should not write their own unless good reasons to.

They could pass that one and the name DefaultContext kind of implies what it is meant for.

// defined in BaggageLogging
struct DefaultContext: BaggageContext, LoggingContext {
    private let _logger: Logger
    var logger: Logger { self._logger.with(baggage) }
    var baggage: Baggage
    init(logger: Logger, context: BaggageContext) { ... } 
}

Please also see and chime in here: #23

---

Disclaimer: My personal opinion is that using context for anything "required" as such to smuggle values between libraries is abusing the API, but there has been enough requests to consider doing this for EventLoop specifically that for it we should explore the topic. Logging and BaggageContext are "special", and have to travel together, which is the entire reason for the Carriers. If we did not insist that logging has to be "passed through APIs" then carriers would have no reason to exist IMHO, but we have to because people want to configure log level in a logger and pass it through, rather than have a powerful backend handle this. Maybe we should revisit how logging works in Swift because it is what makes all this harder than it should be, but that's a separate discussion I guess.

With that in mind, let's explore your case:

One other issue to take into account would be if I have a BaggageContextCarrier generated by library A and I want to generate the BaggageContextCarrier required for library B which includes additional context (maybe an EventLoop).

To be honest this is where the abuse of the context parameters starts to creep in, but let's explore this in depth first.

One other issue to take into account would be if I have a BaggageContextCarrier generated by library A and I want to generate the BaggageContextCarrier required for library B which includes additional context (maybe an EventLoop).

You say "generated", but can we be specific what you mean? I suspect you mean "declared a specific context type and it accepts it in APIs"? This is wrong, it will not compose, specifically this:

generate the BaggageContextCarrier required for library B which includes additional context (maybe an EventLoop)

Sounds to me like you are talking about "specific context types" This is not what APIs should accept. They can only accept the "reusable" ones, and declare what they want as typealias MyContext = LoggingBaggageCarrier & EventLoopBaggageCarrier (names to be polished, please chime in in the linked tickets).

If you then are:

let c: LibA.Context // LoggingBaggageCarrier

func libB(context: LoggingBaggageCarrier & EventLoopPreferenceCarrier)

then indeed one has to "create" the new one, however we can deal with this:

import EventLoopPreferenceContext // however we'd call this...
// automatically conforms any Carrier

Library B BaggageContextCarrier would be carrying library A BaggageContext in both the Logger metadata and its BaggageContext. And when you ask for the Logger from library B it will attempt to augment an already augmented Logger. I hope that makes sense.

Yes, though... that is expected and has expected semantics as well though, doesn't it?

context.baggage.value = "xxx"

var otherContext = DefaultContext(context.logger, context.baggage)
otherContext.baggage.otherValue = "yyy"

otherContext.logger.info() // value=xxx, other-value=yyy

is the expected and correct outcome IMHO. Do you think otherwise or did I misunderstand your example?

---

As a side note: yeah those "Carrier" things complicate things a bit... if it were up to me I guess we'd a) only carry baggage b) create loggers ad hoc and force them to accept a baggage c) logging to remove metadata; But that's a too drastic change to happen over night, so we're exploring the Carrier patterns to keep existing frameworks happy. I think we can do things well if we limit ourselfes to a few well known "okey, those things are everywhere, thus they're allowed in carriers" and the rest will NOT get such magical carrying and one has to either use baggage and smuggle things -- smells like an anti pattern (in Go it is an anti pattern, but people can do it) -- or one should simply pass things explicitly.

I don't have all the answers here, so thank you very much for the tickets and let's keep the discussion going.
I have not yet had the time to dive deep into #25 (anti patterns) and #22 (carrier protocols), but your feedback is quite helpful to get the requirements in.

[adam-fowler]

Instead of describing what is the issue I'll write some code.

Here is LibA that is my framework. It has it's own Context struct which conforms to LoggingBaggageContextCarrier.

struct LibA {
    struct Context: LoggingBaggageContextCarrier {
        private var baseLogger: Logger
        public var baggage: BaggageContext
        public var logger: Logger {
            get {
                return self.baseLogger.with(context: self.baggage)
            }
            ...
        }

        public init(...) {}
    }

    /// add route
    func route(_ route: String, callback: (EventLoop, LoggingBaggageContextCarrier)->())
}

Here is LibB my service library. Again it has a Context but this time it also conforms to EventLoopPreference as well as LoggingBaggageContextCarrier.

struct LibB {
    struct Context: LoggingBaggageContextCarrier & EventLoopPreference {
        public var baggage: BaggageContext
        private var baseLogger: Logger
        public var eventLoop: EventLoop
        public var logger: Logger {
            get {
                return self.baseLogger.with(context: self.baggage)
            }
            ...
        }

        public init(...) {}
    }

    func interface(context: LoggingBaggageContextCarrier & EventEventLoopPreference) {
        ...
    }
}

Here I create my framework and add a route that uses the interface method of library LibB.

let libA = LibA()
libA.route("get") { eventLoop, context in
    let libB = LibB()
    let newContext = LibB.Context(baggage: context.baggage, logger: context.logger, eventLoop: eventLoop)
    libB.interface(context: newContext)
}

I need to create a new context to send to LibB.interface. The logger I construct LibB.Context is the output of LibA.Context.logger ie LibA.baseLogger.with(context: self.baggage). So when I call LibB.logger I get the equivalent of LibA.baseLogger.with(context: self.baggage).with(context: self.baggage). Not sure if this is considered an issue.

Maybe you should just disallow the construction of context carriers with other variables outside of the BaggageContext and Logger. It would mean we wouldn't hit the above issue. In effect it doesn't really make sense to include EventLoop in with the BaggageContext and Logger. That'd resolve the above issue.

By providing a protocol instead of a struct though you are implying that the BaggageContextCarrier object can contain whatever you want (as long as it conforms to protocol). Also if you provide a struct (instead of, not in addition) you can resolve the original issue where you have to trust the creator of the BaggageContextCarrier object has set it up correctly.

[ktoso]

Sorry for being slow to come back here, digesting and looking at some other related things :)

At the very heart of I'd I'd be absolutely on board with:

Maybe you should just disallow the construction of context carriers with other variables outside of the BaggageContext and Logger.

Indeed! As just context and logger must be married to oneother and pushing anything else into carriers is more an outcome of some frameworks who'd like to do that, but really it becomes hard to tame and "where does it end?" then... I think that would perhaps be our way... I will think some more soon, though likely only next week -- thank you for the use cases and discussions!

[slashmo]

Maybe you should just disallow the construction of context carriers with other variables outside of the BaggageContext and Logger.

+1

[ktoso]

I think I've clarified a bit in my head and through discussions with people and this thread as well what we should be doing... really simplifying down to the metal here.

I'll post a proposal PR hopefully tomorrow, thanks!

[ktoso]

Please also see here @Mordil since you are proposing the keeping of this style in #22 rather than the latest "final" approach proposal.

I'm reviewing your PR / idea again but feel it will break down in the same ways in real usage.