Gracefully continue if createStatusReportBase throws - #2225
angelapwen merged 1 commit into
Conversation
Sorry, something went wrong.
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.
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.
Sorry, something went wrong.
| 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.
Sorry, something went wrong.
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
Sorry, something went wrong.
| ...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), |
Sorry, something went wrong.
| 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)
Sorry, something went wrong.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
Sure. I'd say it's low priority since it's just changing a name.
Sorry, something went wrong.
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.
Merge / deployment checklist