Add support for Logger metadataProvider#4
Add support for Logger metadataProvider#4jordanebelanger merged 3 commits intobrainfinance:masterfrom
metadataProvider#4Conversation
As of version 1.5.0 of `swift-log` (https://github.com/apple/swift-log/releases/tag/1.5.0), a new `metadataProvider` was added. Not implementing this new value results in a number of warnings and missing functionality which this commit hopefully begins to address.
In order to support `metadataProvider` this requires us to increment the minimum required version to 1.5.0 (which this feature was added in). This is a minor bump upstream, but is a breaking change to users of StackdriverLogging.
| } else { | ||
| entryMetadata = self.metadata | ||
| } | ||
| let entryMetadata: Logger.Metadata = (metadata ?? [:]) |
There was a problem hiding this comment.
Good job on keeping the right metadata order priority.
One thing, I didn't do a one liner before as I don't want to instantiate an empty dictionary whenever something is being logged without receiving parameter metadata which happens very often.
I'd like that optimization to be kept in, and to go even further with that new optional metadata provider.
If you don't mind, I suggest using a Switch statement on both optional values and handling all 4 cases individually.
There was a problem hiding this comment.
No worries, updated the implementation to follow this pattern.
With regards to metadata order, there does not appear to be any documented priority on logger-level metadata vs provider. I've given priority to provider given it's likely to be used with observability metadata which should be rarely overridden (but ultimate priority still goes to the parameter passthrough).
Hopefully this works for you 👍
There was a problem hiding this comment.
Actually, now that I think about it some more, the logical order should be Parameter > Instance > Provider.
My reasoning is, I would presume providers will usually be set right away at the loggers instantiation or very early on before the logger is used in business logic code.
As such, providers are essentially passive attributors of metadata, if one does take the time to explicitly set metadata on a logger instance down the line, I would say that should have priority. One can always fix a conflict in this case by NOT setting that instance metadata if the provider is taking care of it.
There was a problem hiding this comment.
You can absolutely set the provider earlier in the process, but much like instance level metadata the provider can change its values and the metadata it defines right until the final moments.
They're essentially dynamic values, constantly changing regardless of when you set initially define it.
In contrast instance level is set once and never changed without users explicit interaction.
I'm satisfied with the order as is, but equally happy for you to overrule and change. In practice it's unlikely there would be common conflicts here.
There was a problem hiding this comment.
Agreed, let's leave it as is 👍
As per code review comment, re-introduced intentional optimisation which looks to avoid creating an empty dictionary when parameter-level metadata is not defined.
|
@Sherlouk Thank you sir |
|
Has to be one of the fastest PRs I've done in a while 😅 thanks for the attentiveness Jordane! |
As of version 1.5.0 of
swift-log(https://github.com/apple/swift-log/releases/tag/1.5.0), a newmetadataProviderwas added.Not implementing this new value results in a number of warnings and missing functionality which this commit hopefully begins to address.