Final API proposal - Baggage + Context

[ktoso]

This is a large PR, but in it I hope to finalize the API of this package.

It first and foremost acknowledges issues that we all felt already with the Carrier design -- it's too "wide open" and by being such, it actually hurts composability of libraries.

---

Short discussion:

In general we need to balance a number of needs here:

• some people ALWAYS want to pass a Logger around
• libraries generally "just" need a way to get a logger passed to them
• many libraries, esp client libraries, need context / baggage information to propagate distributed tracing information
• if those are used, a logger MUST be tied to and be passed along with such baggage
• baggage MUST NOT contain a logger; baggage items are intended to be serialized and sent across process/network boundaries

As such I propose the following simplification of the APIs:

Modules & Types

The package: swift-context

The baggage module:

• import Baggage
• dependencies: none
  • main type: Baggage - the "carrier" of baggage items; it is used by instrumentation and tracing to instrument systems

The only user of this I really see being NIO, the team said it would not want to depend on SwiftLog.

And finally, tieing logging and context together in:

• import BaggageContext
• dependencies: SwiftLog
  • Context
  • A Context is EXACTLY Logger + Baggage, nothing more, nothing less

We also offer:

• BaggageProtocol in module Baggage
  • any (framework) context MAY conform to this if it wants .traceID be accessible DIRECTLY on it's context
  • a baggage protocol MUST allow setting a baggage and its properties

and

• ContextProtocol in module BaggageContext
  • allows access to .baggage but does NOT enforce allowing to mutate it
  • frameworks SHOULD conform to this type and MAY offer the baggage as mutable property if it makes sense for them
  • e.g. the framework's context is a struct and has no worries about concurrently mutating / sharing the baggage with others

There are NO other types. The EventLoop we really tried hard to find a place for but we quickly devolve into complexities of various libraries wanting to accept or not wanting to accept it etc. The "...Carriers" design falls flat #28 on delivering good compositionality, in general remaining true to the:

Liberties constrain, constraints liberate.

In this proposal we choose MORE constraints what a context and baggage is, and thus end up with a more composable API.

The short version is:

• Library APIs SHOULD ACCEPT ContextProtocol
• Frameworks SHOULD CONFORM to ContextProtocol
• End users SHOULD keep passing along whichever context they are given

Optional:

• The BaggageProtocol exists to allow certain types (framework contexts) get the .traceID properties via extensions defined in other libraries
• The BaggageProtocol MUST NOT be required when accepting a context value

And finally:

• The most simple APIs MAY simply accept a Baggage struct, if they require no logging.
• Baggage MAY be stored ONLY in especial cases, such as NIO offering it on it's Context in order to pass it between handlers
  • Storing a baggage in such way DOES NOT imply having to conform to BaggageProtocol as these are separate concerns -- "passing around things" (the baggage struct) vs. "be a nice DSLy type where keys get autocompleted" (the protocol conformance)

---

As framework author e.g. "http server" or similar, where a widely used context object exists that holds many many values already (e.g. Vapor, Lambda Runtime)

• At minimum conform your MyFrameworkContext to ContextProtocol and ensure the logger is implemented correctly (documentation on the property explains how)
• If able to, and it suits the style of your framework: optionally conform MyFrameworkContext to BaggageProtocol
  • this is optional and needs careful consideration; it makes your context "seem as-if" it was a Baggage itself.
  • this means that when other libraries do extension BaggageProtocol { var traceID: ... {} } this traceID will be accessible on YOUR framework context directly: myFrameworkContext.traceID rather than myFrameworkContext.baggage.traceID
  • this has potential for naming conflicts but some libraries may like this style
  • [x] TODO: should we remove this protocol or not? It allows other types that contain a baggage to act as if they were one, it's a flexible convenient trick, at the cost of potential minor confusion why these properties are available?
• As a framework you are likely mostly dependent on "your" context, and as such pass it explicitly, using it's strict type
  • If parts of the API may have to be called into from other libraries which do not know about your framework types -- accept a ContextProtocol -- i.e. be "wide" in set of accepted contexts when possible

As library author of a library which does not have it's own "context" and/or is mostly a client e.g. HTTPClient:

• Accept ContextProtocol in your API functions
  • following the parameter positioning explained in the README of this project
• never "store" or "hide" contexts (!) - it will cause confusion and pain in the years moving forward as we build more tooling and other features around context passing.

As library/framework user

• "keep passing along the context you were given"

  • most of the time you will be invoked by a framework (http server, some background task scheduler, other runtime) which already will have a context object
  • always simply "pass it along"

• If unable to get a context, prefer using the .TODO("Not sure where to get a context from?") rather than the .background factory for the context

  • the TODO is explicit in your intent that you should have passed a context, but were unable to
  • the TODO context may be made to CRASH at runtime if you want to enforce your project always passes context when it is pushed to production, see comments for details

• Define any additional baggage keys using private enum MyKey: Baggage.Key { ... } and the extension BaggageContext { var my: MyKey.Value { ... } pattern.

  • those values are intended to be passed through processess and nodes (!), if they are not meant to do so, you may want to set logger metadata instead.

---

• Resolves #28 Guarantee LoggingBaggageContextCarrier Logger metadata includes baggage
• Resolves #25 by documentation notes how the types should be used, and making it more explicit how APis should accept types
• Resolves #23 Revisiting naming
• Resolves #22 Design composable Carrier protocols (or rather, kills it off)

---

The code is mostly "shuffling around" and naming, and making sure everything "fits".

The general "what is possible to express" remains the same and wider -- I made sure this will fit existing context types such as Vapor, soco (AWSSDK), NIO, HTTPClient, Redis. Yes, it does mean passing the event loop explicitly and I really can't find a truely good way to solve it. We end up in arbitrarily tying things together which should not be together if we try to do so.

Please have a look everyone: @adam-fowler @tomerd @lukasa @slashmo @fabianfett @tanner0101 @pokryfka

[tomerd]

over much nicer and clearer. some clarification question and feedback inline.

[Mordil]

Other than my latest comment, I think I'm on board with this PR 💯

[ktoso]

Thank you for your time and reviews @Mordil !

[ktoso]

Thank you everyone on this PR and privately and internally for the feedback, I really feel we're on the last few adjustments to the API and we'll be able to call if final!

Changes in this batch:

• [x] Based on more discussions background is now topLevel, and I really like it; it is "weird enough" to look bad if used in line randomly (it's supposed to look bad) and conveys enough good meaning: https://github.com/slashmo/gsoc-swift-baggage-context/pull/34/commits/38374be1d4edfb11e384ab3d9d02a3d6039eab68
• [x] I adjusted how DefaultContext (and any Context conformance) deals with updating the metadata

This has large (good for applications which log a lot) performance implications which can be seen here (and in the benchmarks added in https://github.com/slashmo/gsoc-swift-baggage-context/pull/34/commits/f0f91b7ae22f38fcc99ebd164724d7573338a2fc):

Next up:

• [x] I look a bit more at the SwiftUI APIs and see if the subscript we expose can be improved a bit... It's a "dont touch this unless you're a developer of a key" so we can't use _ there... but it's not for general use. I don't know if some fancy ideas like we do with Attributes in Swift Tracing and keypaths like SwiftUI does can help here but I'll investigate.
  • i fear it might not work out because it might need Swift 5.2+ features

[ktoso]

All things applied...

• I'm very confident in the proposed package structure: https://github.com/slashmo/gsoc-swift-baggage-context/pull/34#discussion_r493686612
• And separately proposing "access control" / "privacy" for baggage items that @tomerd requested off-thread:

I also looked at @adam-fowler's adoption inside Soco and other projects and am confident we've arrived at something very good.

We're aiming to settle down the discussions this week and being the new repos shortly. Thank you everyone for your patience and reviews, hopefully we're "done" here soon.

[ktoso]

I'm waiting for some feedback from the SPM team about the package structure, once I get that I think we're ready to go here. Planning to land this end of this week.

[ktoso]

Okey folks, so it seems we'll need two repositories.

The exact package and module naming we have to run through some reviews... Until then I'd like to merge this so we're out of the limbo on the types themselves :-)