-
Notifications
You must be signed in to change notification settings - Fork 43
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
Implement the ability to more logically share level hierarchies #134
Merged
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 I find this behaviour surprising.
My expectation would be that if some sub-logger (in this case
b
) has had its level specifically overridden (e.g. by an operator enabling more detailed level on a sub-component), than a subsequent change to the global level should not override the more specific, explicitly set level. It should cascade to all sub-loggers that haven't had an explicitly set level though.I'm thinking of a UX where a peice of software allows operators to specify a targetted log level e.g via ENV var. Something like
FOO_SUBCOMPONENT_LOG=TRACE
while investigating an issue.If they subsequently choose to reconfigure the application e.g. via config file to global log at
DEBUG
instead ofINFO
I'd expect that to apply to everything except the component that has already been overridden.In this example, unlike the comment here, the override is for more detail. There could be an argument that you might want different behaviour depending on whether the override is for more or less detail, but I feel like it's less surprising in all cases to continue to honour more specific sub-logger overrides when changing the parent/global level.
Is there a use-case I'm not seeing where this behavior seems better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Evan and I chatted about this briefly and we contemplated the principle of least surprise. I don't feel strongly either way, but I think there are a couple of things to consider for Vault's implementation -- specifically around the distinction between
/sys/loggers
and your environment variable example.In my mind, our implementation is slightly different from the environment variable case, since logger level state (at least for Vault) is entirely in program memory. With environment variables, you have state outside the scope of the program that would be honored on restart; whereas in Vault, that's not the case. I thought it would be less surprising to keep reload and restart behavior consistent and advertise the fact that
/sys/loggers
is really for transient maintenance tasks.The flip side of that is the maintenance tasks might include reload, at which point this UX could become burdensome (edit: This may not actually be true. I think that log level is only sync'ed on reload if an operator changes the server's HCL, so someone would have to intentionally reset all levels.)
Like I said, I don't feel too strongly either way, but just some thoughts to consider.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Funny you ask this, I asked @mpalmi the same question. Effectively the idea here is that parent logger changes win over any previous decisions the sub-loggers made. @mpalmi thoughts on this? I think this is probably the biggest behavior that we need to sort out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea put forth was that if, at runtime, a decision to put a hierarchy into TRACE mode is requested, everything in the hierarchy should get that change. If a specific component in hierarchy had been set to ERROR at program start, the desire to set the whole hierachry into TRACE would need to override ERROR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@banks In your specific example, the user would have to be careful to configure the toplevel first, then components down the hierarchy second. In that way, they could achieve the same results.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't feel super strongly. At least to me I feel like I'd expect more specific override to take precedence and yes - for it's whole subtree. I've not thought through the actual use-cases or UX of vault /sys/loggers though.
But I think if I'd bothered to make an API call to set a logging level on a specific component, I'd not expect a general change of the logging level via some other mechanism to override that. I don't think I'd lose sleep over it, but it at least wouldn't be my expectation 🤷 .
If you thought this through Mike and this is the preferred UX then I'm fine as long as we document that clearly (both here and in our product docs etc.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually that's a bit more nuanced. I guess what I'd expect would be that if the component specific override for b is removed, then b and it's whole subtree are "re-attached" to the parent and start to respect that setting again.
I have no idea if anyone cares enough to warrant implementing that though! It would probably need an API change too so that it was possible to set a sub-logger back to "follow parent" level rather than some specific level...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, the ability to specify "hey, this is an override" and then "hey, reset this back to not an override" isn't available (and it would be probably a bit confusing).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I put in a vote for "parent always changing level for children is less surprising".
It's possible to achieve the detached logging levels with
IndependentLevels
and some use of state + sublogger hooks (I have a related poc in Consul: hashicorp/consul#16665)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, the current approach seems to be the least surprising -- especially if overrides cannot be reset.