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

Add custom merge function for child logger default metadata #2038

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

FoxxMD
Copy link

@FoxxMD FoxxMD commented Jan 18, 2022

  • Add a parameter to logger.child that accepts a function whose result will be used as the infoClone for that child
  • Provide the current merge functionality as the default argument for this parameter

New PR from suggested changes in #1885

This enables a user to implement their own, arbitrary merge functionality for child loggers.

* Add a parameter to `logger.child` that accepts a function whose result will be used as the infoClone for that child
* Provide the current merge functionality as the default argument for this parameter
@wbt
Copy link
Contributor

wbt commented Jan 20, 2022

This seems related to a collection of related issues centered on #2029, which @maverick1872 is taking the lead on collating.

@maverick1872 maverick1872 added the Feature Request Request for new functionality to support use cases not already covered label Jan 20, 2022
@FoxxMD
Copy link
Author

FoxxMD commented Jan 20, 2022

Sure is..and I was willing to wait until that main issue was resolved before making a PR but maverick said it was unrelated so here we are 🤷

@wbt
Copy link
Contributor

wbt commented Jan 21, 2022

@FoxxMD Sorry for not reading up on the whole chain! I was mainly adding the comment here to make sure that he'd see it. The strategy appears to have been successful.

@maverick1872
Copy link
Member

@FoxxMD I've looked over this implementation and it makes sense to me. I'd like to see test included of course if we were to accept this change. That being said my overarching thought is that a more appropriate implementation would be allowing end users to extend the Logger class itself. There is currently an open issue for this and I believe it would allow for the same functionality, albeit in a different format.

@wbt Do you have any opinions on this or my perspective?

@wbt
Copy link
Contributor

wbt commented Jan 27, 2022

@maverick1872 No strong informed opinions here; I'm deferring to you for in-depth evaluation.

@FoxxMD
Copy link
Author

FoxxMD commented Feb 7, 2022

@maverick1872 extending the class would work fine for me if it solves the same issue 👍 I would also be happy to rebase my PR and get it to pass these tests if you'd like to have this PR as an option, just in case. Your call.

@maverick1872
Copy link
Member

maverick1872 commented Feb 8, 2022

@FoxxMD I'm going to remove this from #2029 as it's not intended to solve the underlying merging of metadata issue. I think long term having a base class (#1902) that can be extended as people see fit is our most ideal solution. I'd like to get the other members opinions on this though before making a hard call on this.

@wbt @fearphage @DABH

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Request Request for new functionality to support use cases not already covered
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants