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

Bug: LogFormatter not forwarded to child loggers #1264

Closed
2 tasks done
ghdoergeloh opened this issue Jan 30, 2023 · 6 comments · Fixed by #1267
Closed
2 tasks done

Bug: LogFormatter not forwarded to child loggers #1264

ghdoergeloh opened this issue Jan 30, 2023 · 6 comments · Fixed by #1267
Assignees
Labels
bug Something isn't working completed This item is complete and has been merged/shipped logger This item relates to the Logger Utility

Comments

@ghdoergeloh
Copy link

Use case

I found out, that I need to pass the LogFormatter to every child. It would be nice to forward the parents LogFormatter as the default to a child logger (via createChild) if no other LogFormatter was provided in that method.

Solution/User Experience

So if logger.createChild({}) is called without logFormatter it could just get the same instance, the parent is using.

Alternative solutions

No response

Acknowledgment

@ghdoergeloh ghdoergeloh added triage This item has not been triaged by a maintainer, please wait feature-request This item refers to a feature request for an existing or new utility labels Jan 30, 2023
@shdq
Copy link
Contributor

shdq commented Feb 2, 2023

Hello @ghdoergeloh, thank you for opening the issue.

I think it is a bug since the child should have all the parent's attributes, except you overwrite them with options in createChild.

I made a quick fix, but I want to revisit other attributes that may be also missed for children. Then I open a PR.

@dreamorosi assign me, please. This is also part of #483.

@saragerion saragerion added the logger This item relates to the Logger Utility label Feb 3, 2023
@saragerion
Copy link
Contributor

While I see the validity of the arguments presented, I'd like us to evaluate first if this would be a breaking change and the potential impact for developers by changing this behaviour now in their production environments. Let us look into it first.

@shdq
Copy link
Contributor

shdq commented Feb 3, 2023

@saragerion sure! For context, we changed the implementation of child logger in #1178 last year

@dreamorosi
Copy link
Contributor

dreamorosi commented Feb 7, 2023

So, I have been thinking about this and after reading the documentation & docstrings of the method (createChild), I think this is a bug.

On a high level, and without context, when we create a child logger we want to create a new instance of this logger that is exactly the same as its parent, except for any override that is explicitly set during creation.

The docstring for the method reads this (emphasis mine):

It creates a separate Logger instance, identical to the current one. It's possible to overwrite the new instance options by passing them.

The docs instead, state this (emphasis mine):

Logger supports quick instance cloning via the createChild method

The wording of both pieces of documentation suggests that the parent and child should be exactly the same, except for any explicit override.

With the above in mind, I would expect the LogFormatter in children to be the same as the parent and I think it's safe to consider this a bug and not a breaking change as there's no wording nor indication that the formatter nor any other setting/option should not be persisted.

I do see however an opportunity to further flash out the wording in the docs to make it extra clear that all attributes and settings are forwarded - as identical might not be enough.

@dreamorosi dreamorosi added discussing The issue needs to be discussed, elaborated, or refined and removed triage This item has not been triaged by a maintainer, please wait labels Feb 8, 2023
@dreamorosi dreamorosi assigned dreamorosi and shdq and unassigned dreamorosi Feb 8, 2023
@dreamorosi dreamorosi added bug Something isn't working and removed feature-request This item refers to a feature request for an existing or new utility labels Feb 8, 2023
@dreamorosi dreamorosi changed the title Feature request: Forward LogFormatter to Child Logger Bug: LogFormatter not forwarded to child loggers Feb 8, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Feb 9, 2023

⚠️ COMMENT VISIBILITY WARNING ⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

@github-actions github-actions bot added the pending-release This item has been merged and will be released soon label Feb 9, 2023
@dreamorosi dreamorosi added completed This item is complete and has been merged/shipped and removed discussing The issue needs to be discussed, elaborated, or refined labels Feb 9, 2023
@dreamorosi dreamorosi removed the pending-release This item has been merged and will be released soon label Mar 2, 2023
@dreamorosi
Copy link
Contributor

A fix was released as part of the latest v1.6.0 release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working completed This item is complete and has been merged/shipped logger This item relates to the Logger Utility
Projects
None yet
4 participants