feat(build-index): count manifest downloads that succeed after a retry by Thanatos9404 · Pull Request #723 · uber/kraken

Thanatos9404 · 2026-10-16T06:11:25Z

Fixes #507

#502 added retries to the manifest download that build-index does when it resolves a tag's dependencies, and its review asked for a way to see how many downloads only succeed thanks to a retry. This adds a counter for that.

tagtype.NewMap now takes the build-index tally.Scope (passed from build-index/cmd/cmd.go) and tags it with module: tagtype, like the tagserver and other components do.
dockerResolver.downloadManifest increments manifest_download_retry_successes when the download succeeds on an attempt after the first. Downloads that succeed on the first attempt, and downloads that still fail after the last retry, are not counted.

Tests

TestDockerResolver_DownloadManifest_RetrySuccessMetric in build-index/tagtype/docker_resolver_test.go checks the counter after a download that succeeds on the first attempt (0), after one or two retries (1), and when every attempt fails (0).
TestMapDockerRetrySuccessMetric in build-index/tagtype/map_test.go goes through NewMap and Resolve with one retry and checks for manifest_download_retry_successes+module=tagtype = 1.
The existing NewMap callers in map_test.go pass tally.NoopScope.

go test --tags unit ./build-index/tagtype/ passes on Linux and go vet is clean for the package.

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

CLAassistant · 2026-10-16T06:12:07Z

All committers have signed the CLA.

sambhav-jain-16 · 2026-10-19T09:54:14Z

Hi @Thanatos9404
Thanks for the contribution. Can you stack this PR onto your older PR? Github stacking is enabled for this repo so you can create stacked PRs.

Thanatos9404 · 2026-10-19T10:59:43Z

Thanks! #723 is already built on #722 (its first commit is #722's), but GitHub's stacked PRs need both branches in the same repository, and these come from my fork, so I can't link them as a stack. Once #722 is merged I'll rebase this so it only has its own commit. If you'd prefer, I can close #722 and keep both changes here instead.

The unit_tests failure is in lib/blobrefresh (TestDedupSameBlobWithDifferentNamespaces, an unlinkat ... directory not empty during temp-dir cleanup), which these changes don't touch.

thijmv

Could you keep this PR to the metric addition only? That is, drop the configurable backoff commit from this PR (or rebase this PR onto Thanatos9404:build-index-configurable-backoff).

thijmv · 2026-10-25T11:38:32Z

+	return 0
+}
+
+func TestDockerResolver_DownloadManifest_RetrySuccessMetric(t *testing.T) {


Can we add a case where the download never succeeds after retrying, and assert that the metric is not emitted?

thijmv · 2026-10-25T11:39:59Z

+		failures int
+		want     int64
+	}{
+		{"succeeds on the first attempt", 0, 0},


nit: Can we use field names for the test cases? It may not be immediately clear to a new reader what, e.g. 0, 0 refers to.

Copilot

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

Thanatos9404 · 2026-10-25T13:57:44Z

Thanks for the review! I dropped the backoff commit, so this PR is now only the metric change on top of master. The resolver test has a new case where every attempt fails and checks the counter stays at 0, and the test cases now use field names. Since #722 touches the same NewMap lines, whichever of the two merges second will need a small rebase, and I'll take care of it.

thijmv · 2026-10-26T08:36:42Z

+		{desc: "succeeds after one retry", failures: 1, succeeds: true, want: 1},
+		{desc: "succeeds after two retries", failures: 2, succeeds: true, want: 1},
+		// MaxRetries is 3, so every one of the 4 attempts fails.
+		{desc: "never succeeds", failures: 4, succeeds: false, want: 0},


nit: We could add a testMaxRetries constant (also used by newTestDockerResolver) to avoid a comment and magic number here.

Good idea, done in e1dd338: testMaxRetries now sets MaxRetries in newTestDockerResolver, and the never-succeeds case uses testMaxRetries + 1, so the comment is gone.

Copilot

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

cm-howard · 2026-10-27T13:50:39Z

Heads up: this PR is being closed and immediately re-opened to re-trigger the CLA Assistant. The license/cla status was never reported on the latest commit, so this is to help remedy the missing CLA attribution. No action is needed from you; the CLA check should update shortly.

thijmv · 2026-10-27T14:38:23Z

Hi @Thanatos9404, can you rebase your branch with the latest changes from master?

Since uber#502 the docker tag type retries manifest downloads that fail with a 404 or a network error, but nothing records how often a retry is what made the download succeed, so there is no way to see what the retries buy. Pass the build-index tally scope into tagtype.NewMap, tag it with module tagtype as other components do, and increment a manifest_download_retry_successes counter when a manifest download succeeds on an attempt after the first. Downloads that succeed on the first attempt and downloads that fail after every retry are not counted. Fixes uber#507.

…metric test Add a case where every attempt fails and check that the counter is not incremented, and name the fields of the test cases.

Add a testMaxRetries constant for the MaxRetries value in newTestDockerResolver and use it in the never-succeeds case of the retry metric test, instead of a hardcoded attempt count and a comment.

Copilot

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

Thanatos9404 · 2026-10-27T14:51:47Z

Done, rebased onto master. The only conflict was in NewMap: the docker resolver now gets its backoff from the config added in #722 plus the stats scope, and the new backoff config test passes tally.NoopScope. The tagtype tests pass locally.

thijmv

LGTM

Copilot AI balanced review requested due to automatic review settings October 16, 2026 06:11

Copilot AI reviewed Oct 16, 2026

gkeesh7 requested a review from thijmv October 22, 2026 11:15

thijmv reviewed Oct 25, 2026

View reviewed changes

Thanatos9404 force-pushed the build-index-retry-success-metric branch from 4308831 to 4090d39 Compare October 25, 2026 13:57

Copilot AI balanced review requested due to automatic review settings October 25, 2026 13:57

Copilot AI reviewed Oct 25, 2026

thijmv approved these changes Oct 26, 2026

View reviewed changes

Copilot AI balanced review requested due to automatic review settings October 26, 2026 08:54

Copilot AI reviewed Oct 26, 2026

cm-howard closed this Oct 27, 2026

cm-howard reopened this Oct 27, 2026

Thanatos9404 added 3 commits October 27, 2026 20:19

test(build-index): cover a download that never succeeds in the retry …

0ba4586

…metric test Add a case where every attempt fails and check that the counter is not incremented, and name the fields of the test cases.

test(build-index): name the test resolver's retry limit

c120889

Add a testMaxRetries constant for the MaxRetries value in newTestDockerResolver and use it in the never-succeeds case of the retry metric test, instead of a hardcoded attempt count and a comment.

Thanatos9404 force-pushed the build-index-retry-success-metric branch from e1dd338 to c120889 Compare October 27, 2026 14:51

Copilot AI balanced review requested due to automatic review settings October 27, 2026 14:51

Copilot AI reviewed Oct 27, 2026

thijmv approved these changes Oct 28, 2026

View reviewed changes

thijmv merged commit 7a99f18 into uber:master Oct 28, 2026
12 checks passed

Navigation Menu

Repository navigation

feat(build-index): count manifest downloads that succeed after a retry - #723

feat(build-index): count manifest downloads that succeed after a retry#723
thijmv merged 3 commits into
uber:masterfrom
Thanatos9404:build-index-retry-success-metric

Thanatos9404 commented Oct 16, 2026 •

edited

Loading

Copilot AI left a comment

CLAassistant commented Oct 16, 2026 •

edited

Loading

sambhav-jain-16 commented Oct 19, 2026

Thanatos9404 commented Oct 19, 2026

thijmv left a comment

thijmv Oct 25, 2026

thijmv Oct 25, 2026

Copilot AI left a comment

Thanatos9404 commented Oct 25, 2026

thijmv Oct 26, 2026

Thanatos9404 Oct 26, 2026

Copilot AI left a comment

cm-howard commented Oct 27, 2026

thijmv commented Oct 27, 2026

Copilot AI left a comment

Thanatos9404 commented Oct 27, 2026

thijmv left a comment

Uh oh!

Uh oh!

Labels

6 participants

Conversation

Thanatos9404 commented Oct 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Tests

Copilot AI left a comment

Choose a reason for hiding this comment

CLAassistant commented Oct 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

sambhav-jain-16 commented Oct 19, 2026

Thanatos9404 commented Oct 19, 2026

thijmv left a comment

Choose a reason for hiding this comment

thijmv Oct 25, 2026

Choose a reason for hiding this comment

thijmv Oct 25, 2026

Choose a reason for hiding this comment

Copilot AI left a comment

Choose a reason for hiding this comment

Thanatos9404 commented Oct 25, 2026

thijmv Oct 26, 2026

Choose a reason for hiding this comment

Thanatos9404 Oct 26, 2026

Choose a reason for hiding this comment

Copilot AI left a comment

Choose a reason for hiding this comment

cm-howard commented Oct 27, 2026

thijmv commented Oct 27, 2026

Copilot AI left a comment

Choose a reason for hiding this comment

Thanatos9404 commented Oct 27, 2026

thijmv left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Labels

6 participants

Thanatos9404 commented Oct 16, 2026 •

edited

Loading

CLAassistant commented Oct 16, 2026 •

edited

Loading