Gracefully continue if createStatusReportBase throws#2225
Conversation
788a476 to
17bf96c
Compare
Previously, we weren't catching any possible exceptions in `createStatusReportBase` and runs would fail if any of the telemetry sub-items threw exceptions. As telemetry should not block the analysis, we continue here even if the status report throws.
17bf96c to
9e8cae9
Compare
aeisenberg
left a comment
There was a problem hiding this comment.
Nice. A few comments inline. If the answer to my question is "no", then this can go in as-is.
| await statusReport.sendStatusReport(trapCacheUploadStatusReport); | ||
| } else { | ||
| await statusReport.sendStatusReport(report); | ||
| if (config && didUploadTrapCaches) { |
There was a problem hiding this comment.
Do we ever reasonably expect config to be falsy, but didUploadTrapCaches to be true? If so, we might want to still upload in that case:
| if (config && didUploadTrapCaches) { | |
| if (didUploadTrapCaches) { |
And the change below. But only if this is not already an error state.
There was a problem hiding this comment.
I don't think so — if config is undefined, we should be erroring out much earlier:
codeql-action/src/analyze-action.ts
Line 205 in f421cda
| ...report, | ||
| trap_cache_upload_duration_ms: Math.round(trapCacheUploadTime || 0), | ||
| trap_cache_upload_size_bytes: Math.round( | ||
| await getTotalCacheSize(config.trapCaches, logger), |
There was a problem hiding this comment.
Only if also adding above.
| await getTotalCacheSize(config.trapCaches, logger), | |
| await getTotalCacheSize(config?.trapCaches || {}, logger), |
| status === "success" || | ||
| status === "failure" || | ||
| status === "aborted" || | ||
| status === "user-error" |
There was a problem hiding this comment.
Should this change to configuration-error? (Not in this PR)
There was a problem hiding this comment.
We could: the configuration-error was codified in the overall job status (rather than status for each action, which is what this is). It would be nice to have them use the same syntax. But we would want to make sure that everyone consuming the data are aware about the transition, because we likely have existing monitors/queries/dashboards using user-error. I can write up an internal issue for this.
There was a problem hiding this comment.
Sure. I'd say it's low priority since it's just changing a name.
Previously, we weren't catching any possible exceptions in
createStatusReportBaseand runs would fail if any of the telemetry sub-items threw exceptions. As telemetry should not block the analysis, we continue here even if the status report throws.Merge / deployment checklist