Add status report for uploading databases to API #3358
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds telemetry reporting for database upload operations to provide better observability into the upload process. The changes introduce a structured way to track upload success, failure, and performance metrics.
Key Changes
- Adds
DatabaseUploadResultinterface to capture upload metadata including language, size, timing, overlay base status, and error information - Changes
cleanupAndUploadDatabasesto return upload results instead of void - Integrates database upload results into the status reporting system
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/database-upload.ts | Introduces DatabaseUploadResult interface and modifies cleanupAndUploadDatabases to collect and return upload telemetry for each language |
| src/analyze-action.ts | Updates status report interfaces and threading to include database upload results throughout the analyze action workflow |
| package-lock.json | Automatic npm metadata updates adding peer dependency markers |
| lib/analyze-action.js | Generated JavaScript code corresponding to TypeScript changes (not reviewed per guidelines) |
mbg
left a comment
mbg
left a comment
There was a problem hiding this comment.
Broadly looks good, just a couple of questions I had looking over the changes and one naming suggestion.
src/database-upload.ts
Outdated
| actionsUtil.getRequiredInput("checkout_path"), | ||
| ); | ||
| try { | ||
| const startTime = Date.now(); |
There was a problem hiding this comment.
Any particular reason for using Date.now() for the measurement here rather than performance.now() that we use elsewhere?
There was a problem hiding this comment.
performance.now() would be better 👍
| logger.warning(`Failed to upload database for ${language}: ${e}`); | ||
| reports.push({ | ||
| language, | ||
| error: util.getErrorMessage(e), |
There was a problem hiding this comment.
If getErrorMessage(e) is useful here, would it also make sense in the log message above rather than just e?
src/database-upload.ts
Outdated
|
|
||
| const cleanupLevel = | ||
| // If config.overlayDatabaseMode is OverlayBase, then we have overlay base databases for all languages. | ||
| const isOverlayBase = |
There was a problem hiding this comment.
I think this name is slightly misleading, because it also depends on the FF. Perhaps uploadOverlayDb would be a better name?
mbg
left a comment
mbg
left a comment
There was a problem hiding this comment.
Thanks for addressing all the suggestions!
|
Fixed a merge conflict. |
This PR adds a status report for uploading databases. This contains:
Risk assessment
For internal use only. Please select the risk level of this change:
Which use cases does this change impact?
Workflow types:
dynamicworkflows (Default Setup, CCR, ...).Products:
analysis-kinds: code-scanning.Environments:
github.com.How did/will you validate this change?
See https://github.com/henrymercer-test-org/overlay-testing/actions/runs/20233483858/job/58082003115#step:5:5392
If something goes wrong after this change is released, what are the mitigation and rollback strategies?
How will you know if something goes wrong after this change is released?
Are there any special considerations for merging or releasing this change?
Merge / deployment checklist