Skip to content

Add support for Logger metadataProvider#4

Merged
jordanebelanger merged 3 commits intobrainfinance:masterfrom
Sherlouk:patch-1
Jan 30, 2023
Merged

Add support for Logger metadataProvider#4
jordanebelanger merged 3 commits intobrainfinance:masterfrom
Sherlouk:patch-1

Conversation

@Sherlouk
Copy link
Copy Markdown
Contributor

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.

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 ?? [:])
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 👍

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
@jordanebelanger jordanebelanger merged commit eecf479 into brainfinance:master Jan 30, 2023
@jordanebelanger
Copy link
Copy Markdown
Collaborator

@Sherlouk Thank you sir

@Sherlouk
Copy link
Copy Markdown
Contributor Author

Has to be one of the fastest PRs I've done in a while 😅 thanks for the attentiveness Jordane!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants