Improve the log level filtering heuristics, but also discourage their use #23577
Conversation
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
In order to make this work reasonably we can't avoid using some funny heuristics, which are somewhat reasonable to apply within the context of Terraform itself but would not be good to add to the general "logutils". Specifically, this is adding the additional heuristic that lines starting with spaces are continuation lines and so should inherit the log level of the most recent non-continuation line.
The filtering for other log levels is unreliable and glitchy because it's trying to infer information from the log stream that isn't reliably represented. Although the previous commit has improved the situation somewhat, it is still a tricky and unreliable heuristic, so worth a warning to anyone who is reading such a log that if they see something confusing it could be a result of the heuristic not working fully.
apparentlymart
added
bug
cli
v0.11
ghost
added
the
sdkv1
Now we'll consider any message that doesn't start with a digit as a continuation. This is _definitely_ in loose, best-effort territory now since a continuation line could very well start with a digit, but ultimately this is still better than the totally broken behavior that preceded it. Just to make sure that none of these heuristics interfere with producing complete, accurate logs for TRACE, we'll skip the LevelFilter altogether in that case.
apparentlymart
force-pushed
the
b-log-filtering
branch
from
fbef225
to
fa302f7
Compare
jbardin
approved these changes
Dec 5, 2019
|
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
ghost
locked and limited conversation to collaborators
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
The log filtering behavior was previously completely broken because it was assuming log messages would arrive framed neatly with one message per call to
Write, and that isn't true in Terraform due to the indirection throughpanicwrap.While we can't get this perfect because we're losing too much information, this updated set of heuristics seem to experimentally improve the situation and make it produce closer to the correct result. Because these heuristics are relying on some conventions in Terraform itself, I've brought the
LevelFilterimplementation into Terraform itself so that we can avoid putting these odd heuristics in the sharedlogutilspackage.With that said, it still can't be perfect because we don't have enough information to be filter reliably, and so this also includes an explicit warning so that anyone relying on filtered logs might get an explanation for any oddities they see and we can give them a specific course of action to improve the situation: use
TF_LOG=trace.Previously we were using the filter in all cases, but since we know it's not 100% dependable, and these additional heuristics might make certain situations more quirky, this also includes a special case to bypass the filter when the
TRACElevel is selected, thus ensuring that we keep our promise of producing complete log output in that case, bugs in the filtering logic notwithstanding.It is very likely that this will still misbehave. The goal is only to misbehave less than the previous approach.