Final API proposal - Baggage + Context #34
Conversation
ktoso
force-pushed
the
simple-is-good-proposal
branch
2 times, most recently
from
cd44924 to
7be716f
Compare
|
over much nicer and clearer. some clarification question and feedback inline. |
ktoso
changed the title
| /// | ||
| /// This allows frameworks and library authors to offer APIs which compose more easily. | ||
| /// Please refer to the "Reference Implementation" notes on each of the requirements to know how to implement this protocol correctly. | ||
| public protocol Context { |
There was a problem hiding this comment.
Bringing this to the main PR...
What's the thought on making this more tailored to the intended semantics of naming this more like TracingContext, LoggingContext, or something similar?
A lot of the "confusion" I've had is that Context is very generic, and we're writing in several places that this is not a general execution context object and more specifically a "execution metadata" context with baggage metadata being passed around.
There was a problem hiding this comment.
A lot of the "confusion" I've had is that Context is very generic, and we're writing in several places that this is not a general execution context object and more specifically a "execution metadata" context with baggage metadata being passed around.
@Mordil
I agree ☝️ I'd also like to throw InstrumentationContext into the mix, as it will be used for many kinds of instrumentation, not only tracing.
LoggingContext, in my opinion, sounds more like something internal to swift-log.
There was a problem hiding this comment.
Ah yes, I do like InstrumentationContext quite a lot
There was a problem hiding this comment.
Hey yeah, thanks for the thought - it's a valid concern. I really don't know what a different good name would be other than BaggageContext.
LoggingContextis problematic IMHO but if people really love it we could... it does after all concern it self a lot about binding the baggage to logging.InstrumentationContextpulls in a word into this library that is not from this library -- we can't do that IMHO. We have to be very strict about the words we use and the word instrument is not defined in this library, and never will be really. It just so happens that instrumentation is what uses this, but this is valuable even if instruments didn't exist...TracingContextis a very specific thing and I really don't want to tie this to tracing per se. It also is the exact name in a few tracing APIs which makes people think it's the same (but it's not)- Some tracing APIs have called this or similar things
PropagationContextbut actually they're now moved back and just call itBaggage(huh) - Baggage is our "container" so... this really can only be
ContextorBaggageContextIMHO.
Since we want to have this "basically everywhere" a least long name is preferable thus Context...
If I may, I'd propose either sticking with Context or BaggageContext, with a mild/strong preference to Context as we're likely to secure a proper namespace for it as swift-context, and there are teams internally in Apple which know this concept as "swift context" as well as it relating directly to what people in Go use such things for -- and that the general term is known as "context propagation".
There was a problem hiding this comment.
I agree with the concern about calling it general-purpose, but having it be the context used for propagation. I think the confusion lies in there being different interpretations of "Context". With this library, we only want to expose a context used for propagation, not one (mis-)used e.g. for dependency injection. I'm +1 on Baggage and BaggageContext as type names as they have prior art in the tracing, which relies on context propagation.
module
swift-baggage-context/import SwiftBaggageContext
@ktoso
What I'd suggest for the module name instead of prefixing with Swift is to make it extra clear what this is about and call it ContextPropagation. The package/repo name could then be swift-context-propagation.
There was a problem hiding this comment.
Thanks for chiming in Adam, the package naming is also important indeed. I would like to avoid "plural names" though. There technically exists precedence for Swift... prefixed modules because SwiftProtobuf does so, but yes you're right it's not a pattern we follow.
I very much like @slashmo proposal where the package name is more "functional" rather than just repeating the same words again, we brainstormed this a bit more and I would like to propose the following which I think addresses all concerns:
.package("https://github.com/apple/swift-context-propagation.git", from: "1.0.0")- module name:
import ContextPropagation - includes
protocol BaggageContext { Baggage Logger }andDefaultContext, and friends (the log handler etc) - depends on
SwiftLog
- module name:
.package("https://github.com/apple/swift-context-propagation-baggage.git", from: "1.0.0")- libraries which do not want to depend on logging can use this if they still need to carry a baggage somehow (e.g. NIO this way is able to not have to depend on logging, but offer
context.baggageitself) - module name:
import ContextPropagationBaggage - includes
struct Baggageand the related key types etc - zero dependencies
- libraries which do not want to depend on logging can use this if they still need to carry a baggage somehow (e.g. NIO this way is able to not have to depend on logging, but offer
Please note that repository names are subject to input from Apple teams and management etc, so I can't commit to those, but those would seem a very good balanced tradeoff. Not squatting the general term "context" and not looking weird "swift-baggage" which is a bit "what does this repo even do?" for the uninitiated. It also solves our module name != main type requirement mentioned up-thread here.
There was a problem hiding this comment.
via @Mordil
Are we still sure there needs to be two repos instead of just one?
Yes, I've quadruple checked this today and consulted with Cory as well.
If we don't, then there's an artificial link between a "i dont want to pull logging" projects that would fetch and enforce a version requirement even though they don't use logging in any module.
Imagining such package:
dependencies: [
.package(url: the repo),
],
targets: [
.target(name: "SotoCore", dependencies: [
.product(name: "Baggage", package: "swift-context"), // "i dont want logging"
]),
]
we're STILL both fetching and including in version resolution the logging package:
-> % swift package update
Fetching https://github.com/ktoso/gsoc-swift-baggage-context.git
Fetching https://github.com/apple/swift-log.git
Cloning https://github.com/apple/swift-log.git
Resolving https://github.com/apple/swift-log.git at 1.4.0
Cloning https://github.com/ktoso/gsoc-swift-baggage-context.git
Resolving https://github.com/ktoso/gsoc-swift-baggage-context.git at simple-is-good-proposal
A project which does not use logging at any point (e.g. nio but there can be others), should not force that artificial link between logging and the package. It does not matter that such package does not depend on the module that actually depends on logging for these parts; it helps that logging would not be in the binary result, but for purposes of package management it makes no difference and the logging package still is considered.
There was a problem hiding this comment.
Ah, that’s a little disappoint that SwiftPM does that. I thought it didn’t consider dependencies not used in the target graph.
Ok then. I’m still on board 💯
There was a problem hiding this comment.
Let's get those ✅ rolling in 🏁 (I hope?)
(Feel free to confirm a review even if not repo owner/comitter)
|
Other than my latest comment, I think I'm on board with this PR 💯 |
ktoso
force-pushed
the
simple-is-good-proposal
branch
from
9b2d9a1 to
38374be
Compare
|
Thank you for your time and reviews @Mordil ! |
|
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:
This has large (good for applications which log a lot) performance implications which can be seen here (and in the benchmarks added in f0f91b7):
Next up:
|
|
All things applied...
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. |
rather than each time, which would lead to terrible logging performance if many log statements are made. Generally, there are fewer baggage changes than there are log statements so this tradeoff makes sense. The logger modification amortizes over time if it is not changed again and again.
yet keeps the "huh?" effect of "this likely is wrong" if it appears in the middle of a codebase
metadata itself also just works
privileged authors "those who create keys"
ktoso
force-pushed
the
simple-is-good-proposal
branch
from
33bc17c to
2e7a9ec
Compare
There was a problem hiding this comment.
LGTM, thanks for the countless number of iterations on this 👌
To bring more attention to the last remaining bit, I'm also quoting @ktoso comment regarding naming and package structure. In my opinion, this is a good compromise.
(Requesting changes because this, or an alternative naming scheme, has not been added yet)
.package("https://github.com/apple/swift-context-propagation.git", from: "1.0.0")- module name:
import ContextPropagation - includes
protocol BaggageContext { Baggage Logger }andDefaultContext, and friends (the log handler etc) - depends on
SwiftLog
- module name:
.package("https://github.com/apple/swift-context-propagation-baggage.git", from: "1.0.0")- libraries which do not want to depend on logging can use this if they still need to carry a baggage somehow (e.g. NIO this way is able to not have to depend on logging, but offer
context.baggageitself) - module name:
import ContextPropagationBaggage - includes
struct Baggageand the related key types etc - zero dependencies
- libraries which do not want to depend on logging can use this if they still need to carry a baggage somehow (e.g. NIO this way is able to not have to depend on logging, but offer
|
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. |
|
Okey folks, so it seems we'll need two repositories. The exact package and module naming we have to run through some reviews... |
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:
LoggeraroundAs such I propose the following simplification of the APIs:
Modules & Types
The package:
swift-contextThe baggage module:
import BaggageBaggage- the "carrier" of baggage items; it is used by instrumentation and tracing to instrument systemsThe 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 BaggageContextSwiftLogContextContextis EXACTLYLogger + Baggage, nothing more, nothing lessWe also offer:
BaggageProtocolin moduleBaggage.traceIDbe accessible DIRECTLY on it's contextand
ContextProtocolin moduleBaggageContext.baggagebut does NOT enforce allowing to mutate itThere are NO other types. The
EventLoopwe 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: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:
ContextProtocolContextProtocolOptional:
BaggageProtocolexists to allow certain types (framework contexts) get the.traceIDproperties via extensions defined in other librariesBaggageProtocolMUST NOT be required when accepting a context valueAnd finally:
Baggagestruct, if they require no logging.BaggageMAY be stored ONLY in especial cases, such as NIO offering it on it's Context in order to pass it between handlersBaggageProtocolas 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)
MyFrameworkContexttoContextProtocoland ensure theloggeris implemented correctly (documentation on the property explains how)MyFrameworkContexttoBaggageProtocolBaggageitself.extension BaggageProtocol { var traceID: ... {} }thistraceIDwill be accessible on YOUR framework context directly:myFrameworkContext.traceIDrather thanmyFrameworkContext.baggage.traceIDContextProtocol-- i.e. be "wide" in set of accepted contexts when possibleAs library author of a library which does not have it's own "context" and/or is mostly a client e.g. HTTPClient:
ContextProtocolin your API functionsAs library/framework user
"keep passing along the context you were given"
If unable to get a context, prefer using the
.TODO("Not sure where to get a context from?")rather than the.backgroundfactory for the contextDefine any additional baggage keys using
private enum MyKey: Baggage.Key { ... }and theextension BaggageContext { var my: MyKey.Value { ... }pattern.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