✨ Improve check failure/success details #650
Conversation
| ShouldRetry: true, | ||
| Error: scorecarderrors.MakeRetryError(err), | ||
| } | ||
| } | ||
|
|
||
| // TODO: update this function to return a ResultDontKnow | ||
| // if the confidence is low? |
There was a problem hiding this comment.
should this decision be left to the check itself, rather than trying to decide for all checks here? Since checks are very different, I think it's hard to do that in this function in practice.
There was a problem hiding this comment.
We can pass 2 extra args here resultWhenBelowThreshold and resultWhenAboveThreshold.
laurentsimon
requested review from
azeemshaikh38,
naveensrinivasan,
inferno-chromium and
oliverchang
| @@ -20,32 +20,38 @@ import ( | |||
| "github.com/ossf/scorecard/checker" | |||
| ) | |||
|
|
|||
| const CheckAutomaticDependencyUpdate = "Automatic-Dependency-Update" | |||
| const checkAutomaticDependencyUpdate = "Automatic-Dependency-Update" | |||
There was a problem hiding this comment.
Why are choosing to stop exporting these check names?
There was a problem hiding this comment.
these variables are not exported and not used outside check implementations. They are passed only as arguments to registerCheck, so I've renamed them.
There was a problem hiding this comment.
We do use these check names outside:
https://github.com/ossf/scorecard/blob/main/cron/worker/main.go#L183
There was a problem hiding this comment.
thanks. Will revert this hen.
checker/check_result.go
Outdated
| @@ -25,12 +26,44 @@ const MaxResultConfidence = 10 | |||
| // ErrorDemoninatorZero indicates the denominator for a proportional result is 0. | |||
| var ErrorDemoninatorZero = errors.New("internal error: denominator is 0") | |||
|
|
|||
| // Types of details. | |||
There was a problem hiding this comment.
IMO, we can capture all info here (from line 29-58) using custom errors.
type ScorecardError struct {
ErrorLevel int // enum values: Info, Warn, Log
ErrorType int // enum values: Pass, Fail, NA
}
func (e ScorecardError) Error() string {}
func (e ScorecardError) Unwrap() error {}
We then update CheckResult to have a new fieldErrors []ScorecardError or Errors []error. This will contain all failures from a single check.
This is also useful when we expose Scorecard as a library, since the caller can propagate/handle these like regular Golang errors.
There was a problem hiding this comment.
There was a problem hiding this comment.
IMO, we can capture all info here (from line 29-58) using custom errors.
type ScorecardError struct { ErrorLevel int // enum values: Info, Warn, Log ErrorType int // enum values: Pass, Fail, NA
I'm not sure this always works: some warning are intended for the user and don't indicate failures or success per-se. For example, if we log every commit that is reviewed, eg code_review.go: it indicates neither failure, nor success, nor NA. That's why I treated them as an enum.
}
func (e ScorecardError) Error() string {}
func (e ScorecardError) Unwrap() error {}We then update `CheckResult` to have a new field`Errors []ScorecardError` or `Errors []error`. This will contain all failures from a single check. This is also useful when we expose Scorecard as a library, since the caller can propagate/handle these like regular Golang errors.
I did not do it this way because using errors for failed passes conflates runtime errors with results. What I've implemented is equivalent to:
details, err := runCheck()
if err != nil {
// we could not run the check
}
// check was run, we can now inspect the results
AFAIK, errors should indicate a runtime error to perform the check: for example we cannot parse a file, we cannot parse an API response, the repo does not exist, the API response indicates an error, etc
In contrast, check failures/passes are just results the API caller can look at, but there's no need to 'propagate' them like errors (the caller is free to use errors if they want, though). The result has everything needed by the caller:
- runtime error (see below)
Error - result
Pass2:Fail/Pass/NAonly relevant if no runtime error. - details about what failed
Details2, what passed, etc. The caller can inspect the structure if they want more information. The check name and the error name (if there are sub-checks) can be used to show remediation from yaml file, for example.
Notes on runtime errors:
Currently, we propagate all errors we encounter: for example frozen_deps.go, binary_artifact.go, code_review.go But I think this is wrong because it exposes low-level implementation details the caller should not rely upon. This is called out in https://blog.golang.org/go1.13-errors.
If we ever change the implementation, our callers will all break. To be clear, I started doing this myself so I'm the main culprit :-)
I'll send a follow-up PoC PR with suggestions on error handling so we can discuss it more. (by error, I mean runtime errors).
There was a problem hiding this comment.
One high level comment: There's no context passed in between the scorecard run and the proposed --explain apart from an error code.
It seems beneficial for the detailed error/remediation messages to be a template etc, where context from the actual check run is passed through? Some context might be included in the log message with the CLogger.Fail call, but it seems better for UX if we just merge this together with the --explain output to provide a single cohesive set of remediation steps rather than 2 separate ones that a human has to mentally connect?
Do we need a separate step (--explain) to show the detailed error? We can also avoid non-descriptive error codes if we don't have this indirection.
I don't know the answer. Having all in one place has it pros like you mention, but it also bloats the output. About the need for Note that even checks that pass may need some explanation: users don't always understand why the check succeeds or has low confidence. The logging we add is explanatory to the user, and Let's discuss more in the next meeting. |
Yeah, colour coding or some other way of organising info should work here. I'm not sure bloat is a strong argument against this. It seems much more annoying for an end user if they have to issue many commands (1 + N explain commands) and mentally connect the results to get a full picture of what's going on. |
laurentsimon
force-pushed
the
feat/details
branch
from
fb87b01 to
4c98148
Compare
|
@oliverchang @azeemsgoogle I've addressed the comments and implemented what we discussed offline. I'm working on unit tests, but you can review already. Example of output: |
laurentsimon
force-pushed
the
feat/details
branch
from
a76dac7 to
5a6ae44
Compare
| # This is the source of truth for all check descriptions and remediation steps. | ||
| # Run `cd checks/main && go run /main` to generate `checks.json` and `checks.md`. | ||
| checks: | ||
| Binary-Artifacts: |
There was a problem hiding this comment.
@inferno-chromium @oliverchang wdut of the risk levels I gave?
laurentsimon
force-pushed
the
feat/details
branch
from
5a6ae44 to
4bc92a3
Compare
laurentsimon
force-pushed
the
feat/details
branch
from
58cef68 to
cd1a68a
Compare
|
closing since it's superseded by other commits. |
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
feature
What is the current behavior? (You can also link to an open issue here)
it's hard for user to have information about failures, and where to find remediation steps.
What is the new behavior (if this is a feature change)?
WARNING: I'madding everyone as reviewer to get enough feedback!
This PR adds more information for each check failure. This PR only shows a PoC for 4 of our current checks. The functions that needed change have been duplicated as
funcName2; and variables changed tovarName2for this PoC. The changes will happen progressively. Once all the checks have been updated, we can rename the variables to their original names.For each failure, an error number is shown. The user can use this number to get more information about the failure via, for example:
scorecard --explain Automatic-Dependency-Update-E01(This is inspired by the Rust toolchain).
I have not implemented this
--explainfunctionality yet. I'll do that when we are in agreement about everything else in this PR.An error number is returned for every failure in a check: this allows us to have different error numbers for sub-checks: dockerfile pinning, curl | bash, etc within a single check. Note that some checks have a single failure. Checks that use
MultiCheckAndwill have multiple sub-checks.Note that if the check only perform one check, it may feel award to have specific error numbers. We could have
for certain checks. The current code already supports this. it suffices to never add log details in the check, and the error message above will automatically be updated (see
pkg/scorecard_result.gofile). The yaml file, as currently updated, will also make this work automatically.Example of output:
It's easy to adapt the code to save into a SARIF format or Json, since there is enough information available in the details and in the yaml file. If we want to display full remediation steps, e.g. for
scorecard --explain Automatic-Dependency-Update-E01, we can load the info from the yaml file.Please read some of the comments I added in the PR.