Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the deployment record client’s observability and behavior around GitHub API responses (rate limiting and “no artifacts found”), including new Prometheus metrics and expanded PostOne test coverage.
Changes:
- Add new Prometheus counters for
no_attestationandrate_limitedoutcomes. - Refactor
PostOneresponse handling/logging to distinguish rate limiting and “no artifacts found” 404s. - Add comprehensive
PostOneunit tests and document the new metrics in the README.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/dtmetrics/prom.go | Adds new counters for no_attestation and rate_limited. |
| pkg/deploymentrecord/client.go | Introduces NoArtifactError and refines retry/logging logic for 4xx/5xx, including rate-limited detection. |
| pkg/deploymentrecord/client_test.go | Adds table-driven tests validating PostOne behavior and metric deltas across outcomes. |
| internal/controller/controller.go | Treats NoArtifactError as non-fatal (no requeue) when posting records. |
| README.md | Documents the newly added metrics. |
| go.mod | Adds an indirect dependency entry. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| if resp.Header.Get("retry-after") != "" || resp.Header.Get("x-ratelimit-remaining") == "0" { | ||
| // rate limited — retry with backoff | ||
| // Could be 403 or 429 | ||
| dtmetrics.PostDeploymentRecordRateLimited.Inc() | ||
| slog.Warn("rate limited, retrying", | ||
| "attempt", attempt, | ||
| "status_code", resp.StatusCode, | ||
| "retry_after", resp.Header.Get("Retry-After"), | ||
| "container_name", record.Name, | ||
| "resp_msg", string(respBody), | ||
| ) |
There was a problem hiding this comment.
Will handle this in a seperate change
| // (rate limit) | ||
| if resp.StatusCode >= 400 && resp.StatusCode < 500 && resp.StatusCode != 429 { | ||
| switch { | ||
| case resp.StatusCode == 404 && bytes.Contains(respBody, []byte("no artifacts found")): |
There was a problem hiding this comment.
In case the error message changes from "no artifacts found", what do you think of flipping the logic a bit so if-else logic branch ordering is as follows:
- check if the status code >= 400 and < 500
- check whether the response includes the rate limiting headers
- check if the response status code is specifically 404
Or alternatively just dropping the bytes.Contains(respBody, []byte("no artifacts found")) check?
| wantClientError: 1, | ||
| }, | ||
| { | ||
| name: "400 returns ClientError", |
There was a problem hiding this comment.
We could also throw in a case for 422 responses when the request body is invalid.
This change:
not_savedmetric tono_attestationrate_limitedmetricno artifact foundPostOne