Metadata Providers (e.g. for Distributed Tracing) in LogHandlers

[ktoso]

This is the third, and hopefully final, revision of MetadataProvider. I believe this achieves everything we could come up with in the multiple rounds of reviews and calls, and introduces zero dependencies or api breaks to the project.

Thank you to @slashmo, the NIO and SSWG teams for the continued discussions 👍

This approach is summarized as:

• log handlers gain metadata providers
• providers, and whole of logging, remain unaware of Baggage
• a provider implementation, e.g. metadataProvider: .otel can and will pick up baggage from task-locals, since they do depend on baggage and tracing APIs.
• swift-distributed-tracing is able to implement provideMetadata(from: Baggage)

Updated proposal text: https://github.com/ktoso/swift-log/blob/wip-baggage-in-handlers-but-not-api/proposals/0001-metadata-providers.md

---

Distributed tracing is able to provide the following integration, in the Instrumentation module:

#if swift(>=5.5.0) && canImport(_Concurrency)
@available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *)
extension Logger {

    /// Extract any potential ``Logger/Metadata`` that might be obtained by extracting the
    /// passed ``InstrumentationBaggage/Baggage`` to this logger's configured ``Logger/MetadataProvider``.
    ///
    /// Use this when it is necessary to "materialize" contextual baggage metadata into the logger, for future use,
    /// and you cannot rely on using the task-local way of passing Baggage around, e.g. when the logger will be
    /// used from multiple callbacks, and it would be troublesome to have to restore the task-local baggage in every callback.
    ///
    /// Generally prefer to set the task-local baggage using `Baggage.current` or `Tracer.withSpan`.
    public mutating func provideMetadata(from baggage: Baggage?) {
        guard let baggage = baggage else {
            return
        }

        Baggage.withValue(baggage) {
            let metadata = self.metadataProvider.provideMetadata()
            for (k, v) in metadata {
                self[metadataKey: k] = v
            }
        }
    }
}
#endif

which allows us to "capture all baggage into metadata ONCE" and then keep using this logger with the metadata; which was a feature request from NIO-heavy libraries.

---

The API break reported is:

22:45:48 /* Protocol Requirement Change */
22:45:48 Var LogHandler.metadataProvider has been added as a protocol requirement
22:45:48 

which is accounted for by a default implementation:

public extension LogHandler {
    /// Default implementation for `metadataProvider` which defaults to a "no-op" provider.
    var metadataProvider: Logger.MetadataProvider {
        get {
            return .noop
        }
        set {
            self.log(level: .warning, message: "Attempted to set metadataProvider on \(Self.self) that did not implement support for them. Please contact the log handler maintainer to implement metadata provider support.", metadata: nil, source: "Logging", file: #file, function: #function, line: #line)
        }
    }
}

and this will never be triggered unless someone very actively in their end user library tries to make use of this, and then they'll get this warning. Existing libraries continue to work, but SHOULD implement support for invoking the metadata providers as otherwise the metadata will not be included in log statements. They can do so at their own pace though.

---

Since I wanted to include tests showing how task-locals are to be used with this, but Swift 5.0 cannot lex these properly even (confirmed this compiler bug with the compiler team; this will not be fixed), I had to make a new module for them.

This was quite painful, so either we keep this hacky separate module, or we drop those tests, or we drop 5.0 support. Either is fine with me.

[ktoso]

According to everyone chiming in I've removed the 5.0 workaround: ad2fd8f2f1bebe67813131de443c69386cad2de7 & 3e58207 (#238)

I believe we are done here.

Please have a final look @tomerd, @weissi, @FranzBusch, @yim-lee @slashmo and I believe we could cut a minor with this. I could then cut 1.0.0-beta1 releases of tracing libraries and we could do a similar "last call" before 1.0 on those libraries.

[ktoso]

Both good points, thanks 👍

[ktoso]

Addressed both in https://github.com/apple/swift-log/pull/238/commits/1c3507c4895ad78787e7ca76fb75b3b768a07edb

[ktoso]

Thanks for the comments @FranzBusch, @weissi - addressed!

[ktoso]

@swift-server-bot test this please

[ktoso]

This is ready and received a number of LGTMs -- the 5.0 failure is expected, we're dropping 5.0 (as discussed in the PR thread), the API break is the checker not realizing the new requirement has a default implementation (it is not an source/API break).

Want to give it a final look @tomerd? I'd love to merge and bump impls depending on this :-)

[ktoso]

Thank you, merged the suggestions! :)

[ktoso]

Followed up on your comments in https://github.com/apple/swift-log/pull/238/commits/91437c0bf675feb0987eec60bb23968379414f5b @tomerd -- the reordering of the params I'm doubtful about, see here: https://github.com/apple/swift-log/pull/238#discussion_r1034150298

but I could flip those around still... that's the final thing i guess?

[ktoso]

Discussed offline with @tomerd and I did another pass and removed as much new public API as possible.

We're done! I'm going to merge and release a minor version with this. This will cause an all our tracing repos to "fall into place" and we're soon going to 1.0-beta there 🥳

[ktoso]

To make sure folks don't panic: The API breakage detector is wrong here; we add a protocol requirement, but we add a default impl as well, this is source-compatible and purely additive.