Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow retrieval of previously added fields from the Core #1395

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

adrianlungu
Copy link

As per the feature requested opened here:

  • extended the Core interface with Fields() which can be used to retrieve fields that have been previously added to the core
  • updated the ioCore.With to store the fields in an internal private array
  • updated the ioCore.Clone to clone the fields as well
  • added unit tests to cover these cases
  • updated the types implementing Core to also have Fields()

Adrian Lungu added 2 commits December 13, 2023 00:42
…ured context from the core, and implemented it in the types that implement it; plus tests
…ate the clone to also clone the fields; update tests to cover this additional case
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Adrian Lungu seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@JacobOaks
Copy link
Contributor

Hey @adrianlungu, thanks for the PR.

As it stands currently, this is a breaking change. Adding a method to an exported interface will cause all custom implementations to no longer satisfy the interface.

I'm also curious about your use case. Usually I see observer used in-place of a normal logger, passed into some functionality where you want to test what messages get logged. I don't usually see it used in combination with another logger. If you want to test what fields get added to a logger, is there any way you can pass an observer before the fields get added?

Maybe you can create a separate implementation of Core that wraps an underlying core but keeps track of the fields you add (similar to what this PR is having ioCore do), and use that in your code instead of baking it into ioCore by default.

@adrianlungu
Copy link
Author

Hello @JacobOaks , thank you for the review and insight!

I have a logger built on top of zap which has some mandatory fields that should be present, populated from the environment, but the logger can also be customized with additional fields via context or options.

While trying to write a unit test to assert that the required fields are there, and then that the optional fields are there, I stumbled into the Context fields which does not return the fields if they are initialized via the InitialFields property, or if they are added to the core before I get a chance to inject the Observable core.

Using a config Build() and wrapping the core in there also doesn't help because the WrapCore func would be called after the fields would be added to the core so they would not be seen by the observer core.

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

Successfully merging this pull request may close these issues.

3 participants