Add withMetadata() convenience function for adding multiple metadata k/v pairs to a Logger

[gabbifish]

Adds withMetadata() convenience function for adding multiple metadata k/v pairs to a Logger.

Motivation:

Addresses #321

Modifications:

Note that in the spirit of the Logger[metadataKey: key] = value subscript mutating a logger, this PR also mutates the logger in-place.

Result:

Provides users a convenience function to add or modify multiple Logger.Metadata values in a Logger in place.

[ktoso]

@swift-server-bot test this please

[ktoso]

Pretty good convenience func, I like it. Thanks for the PR

[gabbifish]

@swift-server-bot test this please

[ktoso]

@swift-server-bot test this please

[glbrntt]

Thanks @gabbifish – this looks handy! I've proposed a slightly different name but otherwise this looks great.

[glbrntt]

Can I propose we spell this slightly differently?

with is typically used in names that provide scoped access to something (withUnsafePointer, withCheckedContinuation, withTaskGroup etc.) so doesn't feel quite right here (although I appreciate it is the spelling used for loggers in other languages).

How about addMetadata? Could we also drop the metadata: label to avoid the repetition?

[t089]

Yes, fully agree. Also from "other languages" you would expect a "withMetadata" method to be "not mutable" but return a new logger with the metadata replaced with the new value.

[FranzBusch]

Wondering if merging is the right term here since we merge the new values over the old values if keys are in both metadata "bags". Could we also add a doc comment for the parameter that we pass here please

[t089]

I think merging should not be mutable and return a new Logger.
merge would be the mutable counterpart.

[glbrntt]

I think merging should not be mutable and return a new Logger. merge would be the mutable counterpart.

Agreed. merge is the correct spelling for this context. If we do this we should allow the caller to specify how to handle non-unique keys too. Dictionary has a spelling for this: https://developer.apple.com/documentation/swift/dictionary/merge(_:uniquingkeyswith:)-m2ub

[t089]

since Metadata is actually a Dictionary, this is already available as logger.metadata.merge(_:uniquingKeysWith:) 🤷‍♂️

[glbrntt]

You have to go via the handler (logger.handler.metadata.merge(...)) which is non-obvious so adding API directly on the logger is valuable (especially as you can already set individual values via the subscripts)

[gabbifish]

I do think mergeMetadata, paired with the dropping a required metadata argument label, is the clearest option here--will proceed with that.

[FranzBusch]

Wondering if merging is the right term here since we merge the new values over the old values if keys are in both metadata "bags". Could we also add a doc comment for the parameter that we pass here please

[FranzBusch]

Could we add three more test cases:

1. Where the logger has some existing metadata and we add new metadata without any overlapping keys
2. Same as above but with overlapping keys
3. Adding more than once with overlapping keys

[gabbifish]

Added to existing testMergeMetadata function.

[FranzBusch]

I know this might open up a larger question since @t089 rightly pointed out that Metadata really is just a type alias for Dictionary<String: MetadataValue> what if we expose the metadata that a logger has via a var so that users could just call metadata.merge via the standard method on dictionary.

@weissi @ktoso do you know if there is historical reasons why we don't expose the metadata on the logger? LogHandler has it as a get/set requirement on the protocol so it seems very easy to wire through safely.

[gabbifish]

I would anticipate there are some concerns about exposing the logger handler's internal state so directly? Having simple functions for getting/setting logger metadata provides a simple, easier-to-secure abstraction.

[FranzBusch]

I would anticipate there are some concerns about exposing the logger handler's internal state so directly? Having simple functions for getting/setting logger metadata provides a simple, easier-to-secure abstraction.

I am personally not concerned about exposing the underlying metadata of the log hander directly on the logger. Specifically because the next feature request will be "Can I access the metadata of the logger?" which we would then have to add a getter on the Logger. That's why I think we should have this discussion before we add this specialised method here.

[gabbifish]

One more thought: Exposing metadata directly means we do get a mix of APIs for interactions with metadata. Users now will have two options for setting metadata--logger[metadataKey: "something"] = "else" and logger.metadata["something"] = "else". I suppose that would be a departure from the simple, single convenience subscript approach taken previously. This enables code that could use a mix of the two setters above (or their corresponding getters). Maybe that isn't a bad thing, but my personal take is that it could be confusing UX and promote less consistent logger usage patterns.

I guess this already exists today, though? With logger.handler.metadata? the suggestion above for logger.metadata would just add a third path.

[weissi]

I know this might open up a larger question since @t089 rightly pointed out that Metadata really is just a type alias for Dictionary<String: MetadataValue> what if we expose the metadata that a logger has via a var so that users could just call metadata.merge via the standard method on dictionary.

@weissi @ktoso do you know if there is historical reasons why we don't expose the metadata on the logger? LogHandler has it as a get/set requirement on the protocol so it seems very easy to wire through safely.

Yes, that's a performance reason. The LogHandler doesn't need to store the metadata as a dictionary, it can use whatever data structure it sees fit. It needs to be able to accept it as Dictionary<...> though.

[weissi]

One more thought: Exposing metadata directly means we do get a mix of APIs for interactions with metadata. Users now will have two options for setting metadata--logger[metadataKey: "something"] = "else" and logger.metadata["something"] = "else". I suppose that would be a departure from the simple, single convenience subscript approach taken previously. This enables code that could use a mix of the two setters above (or their corresponding getters). Maybe that isn't a bad thing, but my personal take is that it could be confusing UX and promote less consistent logger usage patterns.

I guess this already exists today, though? With logger.handler.metadata? the suggestion above for logger.metadata would just add a third path.

Yes, this was also a reason we do not offer Logger.metadata. Properties should also have O(1) access and that's impossible to achieve because the LogHandler could be storing the metadata in a different data structure. Or more likely, it could have multiple sources of metadata that it never wants to merge.

[weissi]

One more thought: Exposing metadata directly means we do get a mix of APIs for interactions with metadata. Users now will have two options for setting metadata--logger[metadataKey: "something"] = "else" and logger.metadata["something"] = "else". I suppose that would be a departure from the simple, single convenience subscript approach taken previously. This enables code that could use a mix of the two setters above (or their corresponding getters). Maybe that isn't a bad thing, but my personal take is that it could be confusing UX and promote less consistent logger usage patterns.
I guess this already exists today, though? With logger.handler.metadata? the suggestion above for logger.metadata would just add a third path.

Yes, this was also a reason we do not offer Logger.metadata. Properties should also have O(1) access and that's impossible to achieve because the LogHandler could be storing the metadata in a different data structure. Or more likely, it could have multiple sources of metadata that it never wants to merge.

So yes, please keep not exposing Logger.metadata, that wouldn't be good for performance or extensibility.

[FranzBusch]

Thanks for the context @weissi. This looks good to me. @glbrntt can you give your thumbs up as well then we can merge it.

[FranzBusch]

@swift-server-bot test this please

[glbrntt]

Can we add the uniquingKeysWith parameter as well à la Dictionary?

[glbrntt]

I suspect in many (all?) handler implementations this will acquire and release a lock for each value we update. Can we call merge directly on the handler.metadata to avoid this? i.e. self.handler.metadata.merge(metadata)