Remove unused verification metrics (#457)

* Remove unused verification metrics

* Fix formatting

* Remove unused field

* Fix test

* Update lib/dockerregistry/manifests.go

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

---------

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Author: hweawer

lib/dockerregistry/manifests.go

@@ -20,27 +20,14 @@ import (
 
 	"github.com/uber/kraken/utils/closers"
 
-	"github.com/uber-go/tally"
-	"github.com/uber/kraken/lib/store"
-
 	storagedriver "github.com/docker/distribution/registry/storage/driver"
+
 	"github.com/uber/kraken/core"
 	"github.com/uber/kraken/lib/dockerregistry/transfer"
-	"github.com/uber/kraken/utils/cache"
+	"github.com/uber/kraken/lib/store"
 	"github.com/uber/kraken/utils/log"
 )
 
-const (
-	signatureVerificationSuccessCounter = "signature_verification_success"
-	signatureVerificationFailureCounter = "signature_verification_failure"
-	signatureVerificationErrorCounter   = "signature_verification_error"
-	signatureVerificationDuration       = "signature_verification_duration"
-
-	// Verification cache configuration
-	defaultVerificationCacheSize = 300             // Maximum number of entries
-	defaultVerificationCacheTTL  = 5 * time.Minute // TTL for verification cache entries
-)
-
 type SignatureVerificationDecision int
 
 const (
@@ -52,22 +39,15 @@ const (
 type manifests struct {
 	transferer   transfer.ImageTransferer
 	verification func(repo string, digest core.Digest, blob store.FileReader) (SignatureVerificationDecision, error)
-	metrics      tally.Scope
-
-	// Cache to track verified (repo, digest) combinations to avoid duplicate metric emission.
-	verifiedCache *cache.LRUCache
 }
 
 func newManifests(
 	transferer transfer.ImageTransferer,
 	verification func(repo string, digest core.Digest, blob store.FileReader) (SignatureVerificationDecision, error),
-	metrics tally.Scope,
 ) *manifests {
 	return &manifests{
-		transferer:    transferer,
-		verification:  verification,
-		metrics:       metrics,
-		verifiedCache: cache.NewLRUCache(defaultVerificationCacheSize, defaultVerificationCacheTTL),
+		transferer:   transferer,
+		verification: verification,
 	}
 }
 
@@ -80,7 +60,7 @@ func newManifests(
 //     if subtype is revisions, parses the digest directly from the path.
 //  3. Downloads the manifest blob via the transferer using (repo, digest).
 //  4. Opportunistically invokes verify to run signature/image checks and
-//     record metrics/logs. Verification result is not enforced here.
+//     record logs. Verification result is not enforced here.
 //  5. Returns the digest in ASCII string form as a byte slice.
 //
 // Notes
@@ -121,36 +101,22 @@ func (t *manifests) getDigest(path string, subtype PathSubType) ([]byte, error)
 	}
 	defer closers.Close(blob)
 
-	// Only verify if we haven't already verified this (repo, digest) combination
-	cacheKey := fmt.Sprintf("%s:%s", repo, digest.String())
-	shouldEmitMetrics := !t.verifiedCache.Has(cacheKey)
-
 	// Signature verification is currently not enforced: errors from t.verify are ignored.
 	// This is intentional because verification enforcement is planned for a future release.
 	// Risks: manifests may be accepted without verification, which could allow untrusted content.
 	// TODO: Remove error ignoring and enforce verification once the feature is activated.
-	_, _ = t.verify(path, repo, digest, blob, shouldEmitMetrics) //nolint:errcheck
-
-	if shouldEmitMetrics {
-		t.verifiedCache.Add(cacheKey)
-	}
+	_, _ = t.verify(path, repo, digest, blob) //nolint:errcheck
 	return []byte(digest.String()), nil
 }
 
 // verify runs signature/image verification for a downloaded manifest blob and
-// records metrics/logs around the decision and duration.
+// logs around the decision.
 //
 // Returns
 //   - (true, nil)  when verification is allowed or intentionally skipped.
 //   - (false, nil) when verification explicitly denies.
 //   - (false, err) on verification errors or unknown decisions.
 //
-// Metrics
-//   - signature_verification_duration (timer): end-to-end verification latency.
-//   - signature_verification_success  (counter): allow decisions.
-//   - signature_verification_failure  (counter): deny decisions.
-//   - signature_verification_error    (counter): errors/unknown decisions.
-//
 // Logging
 //   - Error on verification error (includes repo/digest).
 //   - Warn  on deny (includes original path).
@@ -160,43 +126,23 @@ func (t *manifests) verify(
 	repo string,
 	digest core.Digest,
 	blob store.FileReader,
-	emitMetrics bool,
 ) (bool, error) {
-	// Always measure duration, but only emit if not cached
-	var stopwatch tally.Stopwatch
-	if emitMetrics {
-		stopwatch = t.metrics.Timer(signatureVerificationDuration).Start()
-		defer stopwatch.Stop()
-	}
-
 	decision, err := t.verification(repo, digest, blob)
 	if err != nil {
-		if emitMetrics {
-			t.metrics.Counter(signatureVerificationErrorCounter).Inc(1)
-		}
 		log.With("repo", repo, "digest", digest).Errorf("Error while performing image validation %s", err)
 		return false, err
 	}
 
 	switch decision {
 	case DecisionAllow:
-		if emitMetrics {
-			t.metrics.Counter(signatureVerificationSuccessCounter).Inc(1)
-		}
 		return true, nil
 	case DecisionDeny:
-		if emitMetrics {
-			t.metrics.Counter(signatureVerificationFailureCounter).Inc(1)
-		}
 		log.With("repo", repo, "digest", digest).Warnf("Verification failed %s", path)
 		return false, nil
 	case DecisionSkip:
 		log.With("repo", repo, "digest", digest).Debugf("Verification skipped for %s", path)
 		return true, nil
 	default:
-		if emitMetrics {
-			t.metrics.Counter(signatureVerificationErrorCounter).Inc(1)
-		}
 		return false, fmt.Errorf("unknown verification decision: %d", decision)
 	}
 }

lib/dockerregistry/manifests_verification_test.go

@@ -18,7 +18,6 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/require"
-	"github.com/uber-go/tally"
 	"github.com/uber/kraken/core"
 	"github.com/uber/kraken/lib/store"
 	"github.com/uber/kraken/utils/dockerutil"
@@ -59,10 +58,7 @@ func buildDriverWithVerification(t *testing.T, decision SignatureVerificationDec
 		require.Equal(t, manifestDigest, vDigest)
 		return decision, retErr
 	}
-
-	// Use a test scope so we can assert metrics
-	stats := tally.NewTestScope("", nil)
-	sd := NewReadWriteStorageDriver(Config{}, td.cas, td.transferer, verif, stats)
+	sd := NewReadWriteStorageDriver(Config{}, td.cas, td.transferer, verif)
 
 	// Path that triggers manifests.getDigest → verify
 	path := genManifestTagCurrentLinkPath(repo, tag, manifestDigest.Hex())

lib/dockerregistry/storage_driver.go

@@ -28,7 +28,6 @@ import (
 
 	"github.com/docker/distribution/registry/storage/driver"
 	"github.com/docker/distribution/registry/storage/driver/factory"
-	"github.com/uber-go/tally"
 )
 
 // The path layout in the storage backend is roughly as follows:
@@ -101,22 +100,19 @@ func getParam(params map[string]interface{}, name string) interface{} {
 	return p
 }
 
-func (factory *krakenStorageDriverFactory) Create(
-	params map[string]interface{}) (driver.StorageDriver, error) {
-
+func (factory *krakenStorageDriverFactory) Create(params map[string]interface{}) (driver.StorageDriver, error) {
 	// Common parameters.
 	constructor := getParam(params, "constructor").(string)
 	config := getParam(params, "config").(Config)
 	transferer := getParam(params, "transferer").(transfer.ImageTransferer)
-	metrics := getParam(params, "metrics").(tally.Scope)
 
 	switch constructor {
 	case _rw:
 		castore := getParam(params, "castore").(*store.CAStore)
-		return NewReadWriteStorageDriver(config, castore, transferer, factory.verification, metrics), nil
+		return NewReadWriteStorageDriver(config, castore, transferer, factory.verification), nil
 	case _ro:
 		blobstore := getParam(params, "blobstore").(BlobStore)
-		return NewReadOnlyStorageDriver(config, blobstore, transferer, factory.verification, metrics), nil
+		return NewReadOnlyStorageDriver(config, blobstore, transferer, factory.verification), nil
 	default:
 		return nil, fmt.Errorf("unknown constructor %s", constructor)
 	}
@@ -129,24 +125,20 @@ type KrakenStorageDriver struct {
 	blobs      *blobs
 	uploads    uploads
 	manifests  *manifests
-	metrics    tally.Scope
 }
 
 // NewReadWriteStorageDriver creates a KrakenStorageDriver which can push / pull blobs.
 func NewReadWriteStorageDriver(
 	config Config,
 	cas *store.CAStore,
 	transferer transfer.ImageTransferer,
-	verification func(repo string, digest core.Digest, blob store.FileReader) (SignatureVerificationDecision, error),
-	metrics tally.Scope) *KrakenStorageDriver {
-
+	verification func(repo string, digest core.Digest, blob store.FileReader) (SignatureVerificationDecision, error)) *KrakenStorageDriver {
 	return &KrakenStorageDriver{
 		config:     config,
 		transferer: transferer,
 		blobs:      newBlobs(cas, transferer),
 		uploads:    newCASUploads(cas, transferer),
-		manifests:  newManifests(transferer, verification, metrics),
-		metrics:    metrics,
+		manifests:  newManifests(transferer, verification),
 	}
 }
 
@@ -155,16 +147,13 @@ func NewReadOnlyStorageDriver(
 	config Config,
 	bs BlobStore,
 	transferer transfer.ImageTransferer,
-	verification func(repo string, digest core.Digest, blob store.FileReader) (SignatureVerificationDecision, error),
-	metrics tally.Scope) *KrakenStorageDriver {
-
+	verification func(repo string, digest core.Digest, blob store.FileReader) (SignatureVerificationDecision, error)) *KrakenStorageDriver {
 	return &KrakenStorageDriver{
 		config:     config,
 		transferer: transferer,
 		blobs:      newBlobs(bs, transferer),
 		uploads:    disabledUploads{},
-		manifests:  newManifests(transferer, verification, metrics),
-		metrics:    metrics,
+		manifests:  newManifests(transferer, verification),
 	}
 }
 

lib/dockerregistry/testutils_test.go

@@ -24,7 +24,6 @@ import (
 	"github.com/uber/kraken/utils/dockerutil"
 
 	"github.com/docker/distribution/uuid"
-	"github.com/uber-go/tally"
 )
 
 const (
@@ -56,7 +55,7 @@ func newTestDriver() (*testDriver, func()) {
 }
 
 func (d *testDriver) setup() (*KrakenStorageDriver, testImageUploadBundle) {
-	sd := NewReadWriteStorageDriver(Config{}, d.cas, d.transferer, DefaultVerificationFunc, tally.NoopScope)
+	sd := NewReadWriteStorageDriver(Config{}, d.cas, d.transferer, DefaultVerificationFunc)
 
 	// Create upload
 	uploadUUID := uuid.Generate().String()