Tweak abort logging and counting script, log shutdown using logger #3919
Conversation
This file contains hidden or 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
* distinguishes definedness, matching, and conditions as different abort reasons * avoids miscounting matching attempts on unsimplified config which later succeed after simplification * omits function and simplification-related failures (for now) to keep the log smaller * includes basic proxy messages about handling aborts for more information
|
Most importantly, the script was previously counting match-related aborts that could be recovered from by simplifying the configuration. The amount of data is also greatly reduced by not logging For comparison, two runs with the same tarball produce different results because fo the mis-count: |
jberthold
requested review from
geo2a and
goodlyrottenapple
and removed request for
geo2a
jberthold
changed the title
Comment on lines
-287
to
299
| withContext "abort" $ logPretty sortError | ||
| withContext "error" $ logPretty sortError | ||
| failRewrite $ RewriteSortError rule pat.term sortError | ||
| MatchFailed err@ArgLengthsDiffer{} -> do | ||
| withContext "abort" $ logPretty err | ||
| withContext "error" $ logPretty err | ||
| failRewrite $ InternalMatchError $ renderText $ pretty err | ||
| MatchFailed reason -> do | ||
| withContext "failure" $ logPretty reason | ||
| fail "Rule matching failed" | ||
| MatchIndeterminate remainder -> do | ||
| withContext "abort" $ | ||
| withContext "indeterminate" $ | ||
| logMessage $ | ||
| WithJsonMessage (object ["remainder" .= (bimap externaliseTerm externaliseTerm <$> remainder)]) $ | ||
| renderOneLineText $ |
There was a problem hiding this comment.
We need to add these new contexts to the docs to keep them up to date, especially as we now have (off the top of my head):
- match,error
- match,indeterminate
- definedness,abort
- condition,abort
- failure
- failure,break
- failure,continue
Would it make sense to make abort always signify that we fall back? so that instead of match,error we could have something like match,internal-error,abort and match,indeterminate would be match,indeterminate,abort ? Or would this break the filtering?
There was a problem hiding this comment.
I agree we should try to keep a complete list of used context strings - or better, use a type, as we discussed yesterday. I did not start implementing type-based contexts just yet because I just wanted to get things into a state that gives us the same data as before first.
The condition (agree, let's rename to constraint) and definedness contexts were added to distinguish from the context alone what the reason was.
The error logs above are something we never want to see, actual server errors IMHO (inconsistent terms with the definition, despite having been internalised).
The case of match,abort. (now match,indeterminate.) is special because in performRewrite we have a mechanism to simplify (with booster only) and re-try the rewrite, which will often succeed. The log entry that is actually counted (rewrite*,match,abort.) is emitted there when we know we will indeed abort the rewrite.
There was a problem hiding this comment.
I have added the new contexts to the documentation and adapted the explanations there.
goodlyrottenapple
approved these changes
Jun 5, 2024
Co-authored-by: Samuel Balco <goodlyrottenapple@gmail.com>
…logging-and-abort-analysis
jberthold
force-pushed
the
HOTFIX-tweak-abort-logging-and-abort-analysis
branch
from
e9a79dc to
96b3bb3
Compare
rv-jenkins
pushed a commit
that referenced
this pull request
Jun 18, 2024
The context string was changed in prior PR #3919 but the configuration options were not, leading to incomplete data for abort analysis..
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Changes to abort logging and counting tool:
Related: #3865
Changes to shutdown