From 98eb9b309fd124aad815a458e16e1beae7f01f41 Mon Sep 17 00:00:00 2001 From: Elizabeth Worstell Date: Mon, 9 Nov 2026 14:43:02 -0800 Subject: [PATCH] fix: exclude GitHub release metadata lookups from origin metrics For private orgs, the github-releases transform gets release metadata from the GitHub API before it downloads the asset. That request used the artifact request context, so cachew.origin.requests counted it as origin traffic. The README says that GitHub metadata lookups are not counted. The metadata request now uses a context without the artifact request. The 429 backoff scope stays on the context. Co-authored-by: Chain <492cad42391b4a5c84d831885f468e6f1699c0a8c39241f1c002235ece37ee69@buzz> Co-Authored-By: Claude Opus 5.5 --- README.md | 2 +- internal/metrics/artifact.go | 6 +++ internal/strategy/github_releases.go | 3 +- internal/strategy/github_releases_test.go | 45 +++++++++++++++++++++-- 4 files changed, 51 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index e41653aa..a723f2d0 100644 --- a/README.md +++ b/README.md @@ -368,7 +368,7 @@ All carry `strategy` and `registry`. Registry values come from configured host/A The HTTP server metrics of these handlers carry `cachew.cache.result` (`hit`, `miss`, `coalesced` or `error`). Hits also carry `cachew.cache.source`, the backend that served them (`disk`, `s3`, `memory`, `remote`, or `other`), after stale and conditional probes. -Origin metrics count HTTP exchanges that reach an origin through `http.DefaultTransport`, including redirect hops and transport failures. Requests rejected during an upstream 429 cooldown are not counted. `status_code=none` means no HTTP response; `error_type` is `none`, `transport`, `timeout`, or `canceled`. Duration ends at response headers or transport failure. Registry identifies the logical artifact provider, including its redirects, rather than each physical destination. These metrics do not count every low-level connection retry, Git transport, Go-module fetch, GitHub metadata lookup, OCI tag-list or referrer pass-through, or S3 operation. Internal Hermit forwarding to another Cachew route is excluded from origin/fill counts; the receiving handler measures its own work. +Origin metrics count HTTP exchanges that reach an origin through `http.DefaultTransport`, including redirect hops and transport failures. Requests rejected during an upstream 429 cooldown are not counted. `status_code=none` means no HTTP response; `error_type` is `none`, `transport`, `timeout`, or `canceled`. Duration ends at response headers or transport failure. Registry identifies the logical artifact provider, including its redirects, rather than each physical destination. These metrics do not count every low-level connection retry, Git transport, Go-module fetch, GitHub metadata lookup, OCI tag-list or referrer pass-through, or S3 operation. OCI registry ping and token exchanges are counted, because they go to the registry for the artifact. Internal Hermit forwarding to another Cachew route is excluded from origin/fill counts; the receiving handler measures its own work. Fills record a terminal outcome, rather than treating an origin 200 as successful publication. `purpose` separates `origin`, `backfill` and ranged `heal`. Results distinguish successful completion from creation, write, commit, transfer or read errors, cancellation, and superseded heals. Tier writes report backend outcomes separately: a failed S3 write can coexist with a successful disk write. Early close or canceled backfill is not a successful fill. Client responses continue normally when cache writes fail. diff --git a/internal/metrics/artifact.go b/internal/metrics/artifact.go index a816f47f..bbc8e620 100644 --- a/internal/metrics/artifact.go +++ b/internal/metrics/artifact.go @@ -61,6 +61,12 @@ func ContinueArtifactRequest(ctx, request context.Context) context.Context { return ctx } +// WithoutArtifactRequest detaches ctx from its artifact request, so auxiliary +// lookups such as release metadata are not counted as origin traffic. +func WithoutArtifactRequest(ctx context.Context) context.Context { + return context.WithValue(ctx, artifactKey{}, nil) +} + // WithCachePurpose distinguishes origin fills from background cache replication. func WithCachePurpose(ctx context.Context, purpose string) context.Context { return context.WithValue(ctx, purposeKey{}, purpose) diff --git a/internal/strategy/github_releases.go b/internal/strategy/github_releases.go index c5dcc494..ef64c131 100644 --- a/internal/strategy/github_releases.go +++ b/internal/strategy/github_releases.go @@ -15,6 +15,7 @@ import ( "github.com/block/cachew/internal/githubapp" "github.com/block/cachew/internal/httputil" "github.com/block/cachew/internal/logging" + "github.com/block/cachew/internal/metrics" "github.com/block/cachew/internal/spool" "github.com/block/cachew/internal/strategy/handler" ) @@ -172,7 +173,7 @@ func (g *GitHubReleases) downloadRelease(ctx context.Context, org, repo, release logger.DebugContext(ctx, "Using GitHub API for private release") apiURL := fmt.Sprintf("https://api.github.com/repos/%s/%s/releases/tags/%s", url.PathEscape(org), url.PathEscape(repo), escapePathSegments(release)) - req, err := g.newGitHubRequest(ctx, apiURL, "application/vnd.github+json", org, "get_release") + req, err := g.newGitHubRequest(metrics.WithoutArtifactRequest(ctx), apiURL, "application/vnd.github+json", org, "get_release") if err != nil { return nil, httputil.Errorf(http.StatusInternalServerError, "create API request") } diff --git a/internal/strategy/github_releases_test.go b/internal/strategy/github_releases_test.go index 3a526f06..bc9bd69d 100644 --- a/internal/strategy/github_releases_test.go +++ b/internal/strategy/github_releases_test.go @@ -12,11 +12,15 @@ import ( "time" "github.com/alecthomas/assert/v2" + "go.opentelemetry.io/otel" + sdkmetric "go.opentelemetry.io/otel/sdk/metric" + "go.opentelemetry.io/otel/sdk/metric/metricdata" "github.com/block/cachew/internal/cache" "github.com/block/cachew/internal/githubapp" "github.com/block/cachew/internal/httputil" "github.com/block/cachew/internal/logging" + "github.com/block/cachew/internal/metrics" "github.com/block/cachew/internal/strategy" ) @@ -122,11 +126,11 @@ func setupTest(t *testing.T, config strategy.GitHubReleasesConfig) (*mockGitHubS t.Cleanup(mock.close) originalTransport := http.DefaultTransport - t.Cleanup(func() { http.DefaultTransport = originalTransport }) //nolint:reassign // Required for testing - http.DefaultTransport = httputil.NewBackoffTransport(&testTransport{ //nolint:reassign // Required for testing + t.Cleanup(func() { http.DefaultTransport = originalTransport }) //nolint:reassign // Required for testing + http.DefaultTransport = httputil.NewBackoffTransport(metrics.NewOriginTransport(&testTransport{ //nolint:reassign // Required for testing backend: mock.server, originalTransport: originalTransport, - }, time.Minute) + }), time.Minute) _, ctx := logging.Configure(context.Background(), logging.Config{Level: slog.LevelError}) memCache, err := cache.NewMemory(ctx, cache.MemoryConfig{MaxTTL: time.Hour}) @@ -202,6 +206,41 @@ func TestGitHubReleasesPrivateRepo(t *testing.T) { assert.Equal(t, 1, mock.downloadCallCount, "second request should be served from cache") } +func TestGitHubReleasesMetadataIsNotOriginTraffic(t *testing.T) { + previous := otel.GetMeterProvider() + reader := sdkmetric.NewManualReader() + provider := sdkmetric.NewMeterProvider(sdkmetric.WithReader(reader)) + otel.SetMeterProvider(provider) + t.Cleanup(func() { otel.SetMeterProvider(previous); assert.NoError(t, provider.Shutdown(context.Background())) }) + mock, mux, ctx := setupTest(t, strategy.GitHubReleasesConfig{ + Token: "test-token", + PrivateOrgs: []string{"privateorg"}, + }) + + req := httptest.NewRequest(http.MethodGet, "/github.com/privateorg/repo/releases/download/v2.0.0/binary.tar.gz", nil) + w := httptest.NewRecorder() + mux.ServeHTTP(w, req.WithContext(ctx)) + assert.Equal(t, http.StatusOK, w.Code) + assert.Equal(t, 1, mock.apiCallCount) + + var data metricdata.ResourceMetrics + assert.NoError(t, reader.Collect(ctx, &data)) + var origin int64 + for _, scope := range data.ScopeMetrics { + for _, m := range scope.Metrics { + if m.Name != "cachew.origin.requests" { + continue + } + sum, ok := m.Data.(metricdata.Sum[int64]) + assert.True(t, ok) + for _, point := range sum.DataPoints { + origin += point.Value + } + } + } + assert.Equal(t, int64(1), origin) +} + func TestGitHubReleasesPrivateRepoAssetNotFound(t *testing.T) { mock, mux, ctx := setupTest(t, strategy.GitHubReleasesConfig{ Token: "test-token",