Update metrics

5 files changed, 102 insertions, 98 deletions

diff --git a/acceptance_test.go b/acceptance_test.go
index 1df6abfb..a3fedc24 100644
--- a/acceptance_test.go
+++ b/acceptance_test.go
@@ -497,9 +497,7 @@ func TestPrometheusMetricsCanBeScraped(t *testing.T) {
 	// zip archives
 	require.Contains(t, string(body), "gitlab_pages_zip_opened")
 	require.Contains(t, string(body), "gitlab_pages_zip_cache_requests")
-	require.Contains(t, string(body), "gitlab_pages_zip_data_offset_cache_requests")
-	require.Contains(t, string(body), "gitlab_pages_zip_readlink_cache_requests")
-	require.Contains(t, string(body), "gitlab_pages_zip_cached_archives")
+	require.Contains(t, string(body), "gitlab_pages_zip_cached_entries")
 	require.Contains(t, string(body), "gitlab_pages_zip_archive_entries_cached")
 	require.Contains(t, string(body), "gitlab_pages_zip_opened_entries_count")
 }
diff --git a/internal/vfs/zip/archive.go b/internal/vfs/zip/archive.go
index 74a73121..630d5fd5 100644
--- a/internal/vfs/zip/archive.go
+++ b/internal/vfs/zip/archive.go
@@ -47,7 +47,7 @@ type zipArchive struct {
 	done        chan struct{}
 	openTimeout time.Duration
 
-	cacheKey string
+	namespace string
 
 	resource *httprange.Resource
 	reader   *httprange.RangedReader
@@ -64,7 +64,7 @@ func newArchive(fs *zipVFS, path string, openTimeout time.Duration) *zipArchive
 		done:        make(chan struct{}),
 		files:       make(map[string]*zip.File),
 		openTimeout: openTimeout,
-		cacheKey:    strconv.FormatInt(atomic.AddInt64(&fs.archiveCount, 1), 10) + ":",
+		namespace:   strconv.FormatInt(atomic.AddInt64(&fs.archiveCount, 1), 10) + ":",
 	}
 }
 
@@ -167,13 +167,15 @@ func (a *zipArchive) Open(ctx context.Context, name string) (vfs.File, error) {
 		return nil, os.ErrNotExist
 	}
 
-	dataOffset, err := a.getDataOffset(name, file)
+	dataOffset, err := a.fs.dataOffsetCache.findOrFetch(a.namespace, name, func() (interface{}, error) {
+		return file.DataOffset()
+	})
 	if err != nil {
 		return nil, err
 	}
 
 	// only read from dataOffset up to the size of the compressed file
-	reader := a.reader.SectionReader(ctx, dataOffset, int64(file.CompressedSize64))
+	reader := a.reader.SectionReader(ctx, dataOffset.(int64), int64(file.CompressedSize64))
 
 	switch file.Method {
 	case zip.Deflate:
@@ -185,29 +187,6 @@ func (a *zipArchive) Open(ctx context.Context, name string) (vfs.File, error) {
 	}
 }
 
-func (a *zipArchive) getDataOffset(name string, file *zip.File) (int64, error) {
-	var dataOffset int64
-
-	item := a.fs.dataOffsetCache.Get(a.cacheKey + name)
-	if item == nil || item.Expired() {
-		var err error
-
-		dataOffset, err = file.DataOffset()
-		if err != nil {
-			return 0, err
-		}
-
-		a.fs.dataOffsetCache.Set(a.cacheKey+name, dataOffset, DataOffsetCacheInterval)
-
-		metrics.ZipServingArchiveDataOffsetCache.WithLabelValues("miss").Inc()
-	} else {
-		dataOffset = item.Value().(int64)
-		metrics.ZipServingArchiveDataOffsetCache.WithLabelValues("hit").Inc()
-	}
-
-	return dataOffset, nil
-}
-
 // Lstat finds the file by name inside the zipArchive and returns its FileInfo
 func (a *zipArchive) Lstat(ctx context.Context, name string) (os.FileInfo, error) {
 	file := a.findFile(name)
@@ -228,27 +207,11 @@ func (a *zipArchive) Readlink(ctx context.Context, name string) (string, error)
 	if file.FileInfo().Mode()&os.ModeSymlink != os.ModeSymlink {
 		return "", errNotSymlink
 	}
-	symlink, err := a.getSymlink(name, file)
-	if err != nil {
-		return "", err
-	}
 
-	// return errSymlinkSize if the number of bytes read from the link is too big
-	if len(symlink) > maxSymlinkSize {
-		return "", errSymlinkSize
-	}
-
-	return symlink, nil
-}
-
-func (a *zipArchive) getSymlink(name string, file *zip.File) (string, error) {
-	var symlink string
-
-	item := a.fs.readlinkCache.Get(a.cacheKey + name)
-	if item == nil || item.Expired() {
+	symlinkValue, err := a.fs.readlinkCache.findOrFetch(a.namespace, name, func() (interface{}, error) {
 		rc, err := file.Open()
 		if err != nil {
-			return "", err
+			return nil, err
 		}
 		defer rc.Close()
 
@@ -258,17 +221,20 @@ func (a *zipArchive) getSymlink(name string, file *zip.File) (string, error) {
 		n, err := io.ReadFull(rc, link[:])
 		if err != nil && err != io.ErrUnexpectedEOF {
 			// if err == io.ErrUnexpectedEOF the link is smaller than len(symlink) so it's OK to not return it
-			return "", err
+			return nil, err
 		}
 
-		symlink = string(link[:n])
-		// cache symlink up to desired size
-		a.fs.readlinkCache.Set(a.cacheKey+name, symlink, ReadLinkCacheInterval)
+		return string(link[:n]), nil
+	})
+	if err != nil {
+		return "", err
+	}
+
+	symlink := symlinkValue.(string)
 
-		metrics.ZipServingArchiveReadlinkCache.WithLabelValues("miss").Inc()
-	} else {
-		symlink = item.Value().(string)
-		metrics.ZipServingArchiveReadlinkCache.WithLabelValues("hit").Inc()
+	// return errSymlinkSize if the number of bytes read from the link is too big
+	if len(symlink) > maxSymlinkSize {
+		return "", errSymlinkSize
 	}
 
 	return symlink, nil
diff --git a/internal/vfs/zip/lru_cache.go b/internal/vfs/zip/lru_cache.go
new file mode 100644
index 00000000..d33c81e2
--- /dev/null
+++ b/internal/vfs/zip/lru_cache.go
@@ -0,0 +1,52 @@
+package zip
+
+import (
+	"time"
+
+	"github.com/karlseguin/ccache/v2"
+	"gitlab.com/gitlab-org/gitlab-pages/metrics"
+)
+
+type lruCache struct {
+	op       string
+	duration time.Duration
+	cache    *ccache.Cache
+}
+
+func newLruCache(op string, maxEntries uint32, duration time.Duration) *lruCache {
+	configuration := ccache.Configure()
+	configuration.MaxSize(int64(maxEntries))
+	configuration.ItemsToPrune(maxEntries / 16)
+	configuration.GetsPerPromote(64) // if item gets requested frequently promote it
+	configuration.OnDelete(func(*ccache.Item) {
+		metrics.ZipCachedEntries.WithLabelValues(op).Dec()
+	})
+
+	c := &lruCache{
+		cache:    ccache.New(configuration),
+		duration: duration,
+	}
+
+	return c
+}
+
+func (c *lruCache) findOrFetch(namespace, key string, fetchFn func() (interface{}, error)) (interface{}, error) {
+	item := c.cache.Get(namespace + key)
+
+	if item != nil && !item.Expired() {
+		metrics.ZipCacheRequests.WithLabelValues(c.op, "hit").Inc()
+		return item.Value(), nil
+	}
+
+	value, err := fetchFn()
+	if err != nil {
+		metrics.ZipCacheRequests.WithLabelValues(c.op, "error").Inc()
+		return nil, err
+	}
+
+	metrics.ZipCacheRequests.WithLabelValues(c.op, "miss").Inc()
+	metrics.ZipCachedEntries.WithLabelValues(c.op).Inc()
+
+	c.cache.Set(namespace+key, value, c.duration)
+	return value, nil
+}
diff --git a/internal/vfs/zip/vfs.go b/internal/vfs/zip/vfs.go
index a39a1cfe..71899e80 100644
--- a/internal/vfs/zip/vfs.go
+++ b/internal/vfs/zip/vfs.go
@@ -6,7 +6,6 @@ import (
 	"net/url"
 	"time"
 
-	"github.com/karlseguin/ccache/v2"
 	"github.com/patrickmn/go-cache"
 
 	"gitlab.com/gitlab-org/gitlab-pages/internal/vfs"
@@ -18,6 +17,16 @@ const (
 	defaultCacheExpirationInterval = time.Minute
 	defaultCacheCleanupInterval    = time.Minute / 2
 	defaultCacheRefreshInterval    = time.Minute / 2
+
+	// we assume that each item costs around 100 bytes
+	// this gives around 5MB of raw memory needed without acceleration structures
+	defaultDataOffsetItems              = 50000
+	defaultDataOffsetExpirationInterval = time.Minute
+
+	// we assume that each item costs around 200 bytes
+	// this gives around 2MB of raw memory needed without acceleration structures
+	defaultReadlinkItems              = 10000
+	defaultReadlinkExpirationInterval = time.Minute
 )
 
 var (
@@ -26,9 +35,10 @@ var (
 
 // zipVFS is a simple cached implementation of the vfs.VFS interface
 type zipVFS struct {
-	cache           *cache.Cache
-	dataOffsetCache *ccache.Cache
-	readlinkCache   *ccache.Cache
+	cache *cache.Cache
+
+	dataOffsetCache *lruCache
+	readlinkCache   *lruCache
 
 	archiveCount int64
 }
@@ -36,14 +46,13 @@ type zipVFS struct {
 // New creates a zipVFS instance that can be used by a serving request
 func New() vfs.VFS {
 	zipVFS := &zipVFS{
-		// TODO: add cache operation callbacks https://gitlab.com/gitlab-org/gitlab-pages/-/issues/465
 		cache:           cache.New(defaultCacheExpirationInterval, defaultCacheCleanupInterval),
-		dataOffsetCache: ccache.New(ccache.Configure().MaxSize(10000).ItemsToPrune(2000)),
-		readlinkCache:   ccache.New(ccache.Configure().MaxSize(1000).ItemsToPrune(200)),
+		dataOffsetCache: newLruCache("data-offset", defaultDataOffsetItems, defaultDataOffsetExpirationInterval),
+		readlinkCache:   newLruCache("readlink", defaultReadlinkItems, defaultReadlinkExpirationInterval),
 	}
 
 	zipVFS.cache.OnEvicted(func(s string, i interface{}) {
-		metrics.ZipCachedArchives.Dec()
+		metrics.ZipCachedEntries.WithLabelValues("archive").Dec()
 
 		i.(*zipArchive).onEvicted()
 	})
@@ -85,7 +94,7 @@ func (fs *zipVFS) Name() string {
 func (fs *zipVFS) findOrOpenArchive(ctx context.Context, path string) (*zipArchive, error) {
 	archive, expiry, found := fs.cache.GetWithExpiration(path)
 	if found {
-		metrics.ZipServingArchiveCache.WithLabelValues("hit").Inc()
+		metrics.ZipCacheRequests.WithLabelValues("archive", "hit").Inc()
 
 		// TODO: do not refreshed errored archives https://gitlab.com/gitlab-org/gitlab-pages/-/merge_requests/351
 		if time.Until(expiry) < defaultCacheRefreshInterval {
@@ -100,8 +109,8 @@ func (fs *zipVFS) findOrOpenArchive(ctx context.Context, path string) (*zipArchi
 		if fs.cache.Add(path, archive, cache.DefaultExpiration) != nil {
 			return nil, errAlreadyCached
 		}
-		metrics.ZipServingArchiveCache.WithLabelValues("miss").Inc()
-		metrics.ZipCachedArchives.Inc()
+		metrics.ZipCacheRequests.WithLabelValues("archive", "miss").Inc()
+		metrics.ZipCachedEntries.WithLabelValues("archive").Inc()
 	}
 
 	zipArchive := archive.(*zipArchive)
diff --git a/metrics/metrics.go b/metrics/metrics.go
index d2243fae..db7cae9a 100644
--- a/metrics/metrics.go
+++ b/metrics/metrics.go
@@ -164,41 +164,22 @@ var (
 		[]string{"state"},
 	)
 
-	// ZipServingArchiveCache is the number of zip archive cache hits/misses
-	ZipServingArchiveCache = prometheus.NewCounterVec(
+	// ZipCacheRequests is the number of cache hits/misses
+	ZipCacheRequests = prometheus.NewCounterVec(
 		prometheus.CounterOpts{
 			Name: "gitlab_pages_zip_cache_requests",
 			Help: "The number of zip archives cache hits/misses",
 		},
-		[]string{"cache"},
+		[]string{"op", "cache"},
 	)
 
-	// ZipServingArchiveDataOffsetCache is the number of zip archive
-	// data offset cache hits/misses
-	ZipServingArchiveDataOffsetCache = prometheus.NewCounterVec(
-		prometheus.CounterOpts{
-			Name: "gitlab_pages_zip_data_offset_cache_requests",
-			Help: "The number of zip archive data offset cache hits/misses",
-		},
-		[]string{"cache"},
-	)
-
-	// ZipServingArchiveReadlinkCache is the number of zip archive Readlink
-	// cache hits/misses
-	ZipServingArchiveReadlinkCache = prometheus.NewCounterVec(
-		prometheus.CounterOpts{
-			Name: "gitlab_pages_zip_readlink_cache_requests",
-			Help: "The number of zip archives Readlink cache hits/misses",
-		},
-		[]string{"cache"},
-	)
-
-	// ZipCachedArchives is the number of zip archives currently in the cache
-	ZipCachedArchives = prometheus.NewGauge(
+	// ZipCachedArchives is the number of entries in the cache
+	ZipCachedEntries = prometheus.NewGaugeVec(
 		prometheus.GaugeOpts{
-			Name: "gitlab_pages_zip_cached_archives",
-			Help: "The number of zip archives currently in the cache",
+			Name: "gitlab_pages_zip_cached_entries",
+			Help: "The number of entries in the cache",
 		},
+		[]string{"op"},
 	)
 
 	// ZipArchiveEntriesCached is the number of files per zip archive currently
@@ -245,10 +226,8 @@ func MustRegister() {
 		HTTPRangeOpenRequests,
 		ZipOpened,
 		ZipOpenedEntriesCount,
-		ZipServingArchiveCache,
-		ZipServingArchiveDataOffsetCache,
-		ZipServingArchiveReadlinkCache,
+		ZipCacheRequests,
 		ZipArchiveEntriesCached,
-		ZipCachedArchives,
+		ZipCachedEntries,
 	)
 }