Skip to content

fix(metrics): Increment SynchronousReadRegistryFetchCount only on registry fetch#2015

Open
ayush-panta wants to merge 1 commit into
mainfrom
fix-sync-fetch-metric
Open

fix(metrics): Increment SynchronousReadRegistryFetchCount only on registry fetch#2015
ayush-panta wants to merge 1 commit into
mainfrom
fix-sync-fetch-metric

Conversation

@ayush-panta

@ayush-panta ayush-panta commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

Issue #, if available: #1016

Description of changes: Move SynchronousReadRegistryFetchCount metric increment from reader.go (fired on every ReadAt, including cache hits) to span_manager.go's getSpanContent (where it fires only when a span is in unrequested state and an actual network fetch to the registry is issued). To preserve the layer digest dimension on the metric, the layer digest is now passed into SpanManager at construction time.

Testing performed: Added TestSynchronousFetchMetricOnlyFiresOnNetworkFetch unit test that verifies:

  • Metric increments when getSpanContent hits an unrequested span (network fetch)
  • Metric does NOT increment on a subsequent read of the same span (cache hit)
  • Metric does NOT increment when the background fetcher has already fetched the span

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@ayush-panta ayush-panta force-pushed the fix-sync-fetch-metric branch from 3ba57fc to 854e863 Compare June 13, 2026 00:05
@ayush-panta ayush-panta marked this pull request as ready for review June 13, 2026 00:07
@ayush-panta ayush-panta requested a review from a team as a code owner June 13, 2026 00:07

@sondavidb sondavidb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally LGTM some small things

Comment thread fs/metrics/common/metrics.go
Comment thread fs/span-manager/span_manager_test.go Outdated
…istry fetch

Move the metric from reader.go (fired on every ReadAt, including cache
hits) to span_manager.go's getSpanContent (fired only when a span is in
unrequested state and an actual network fetch is issued). Pass the layer
digest into SpanManager so the metric retains its layer dimension.

Signed-off-by: ayush-panta <ayushkp@amazon.com>
@ayush-panta ayush-panta force-pushed the fix-sync-fetch-metric branch from 854e863 to 2d05489 Compare June 17, 2026 22:13
@github-actions github-actions Bot added go Pull requests that update Go code testing Unit and/or integration tests labels Jun 17, 2026

@sondavidb sondavidb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the work on this, LGTM. Definitely need to make sure that we call this out in our release notes since it could be confusing for folks who have relied on this metric.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

go Pull requests that update Go code testing Unit and/or integration tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants