13198: Remove old counters from status.json.

Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tclegg@veritasgenetics.com>

diff --git a/services/keep-web/cache.go b/services/keep-web/cache.go
index b2bab78216eaf921a4710a5037d9af2231df31c5..8336b78f9ea9614af2796211d9ed89d58da741e8 100644 (file)
--- a/services/keep-web/cache.go
+++ b/services/keep-web/cache.go
@@ -6,7 +6,6 @@ package main
 
 import (
        "sync"
-       "sync/atomic"
        "time"
 
        "git.curoverse.com/arvados.git/sdk/go/arvados"
@@ -26,7 +25,6 @@ type cache struct {
        MaxUUIDEntries       int
 
        registry    *prometheus.Registry
-       stats       cacheStats
        metrics     cacheMetrics
        pdhs        *lru.TwoQueueCache
        collections *lru.TwoQueueCache
@@ -34,17 +32,6 @@ type cache struct {
        setupOnce   sync.Once
 }
 
-// cacheStats is EOL - add new metrics to cacheMetrics instead
-type cacheStats struct {
-       Requests          uint64 `json:"Cache.Requests"`
-       CollectionBytes   uint64 `json:"Cache.CollectionBytes"`
-       CollectionEntries int    `json:"Cache.CollectionEntries"`
-       CollectionHits    uint64 `json:"Cache.CollectionHits"`
-       PDHHits           uint64 `json:"Cache.UUIDHits"`
-       PermissionHits    uint64 `json:"Cache.PermissionHits"`
-       APICalls          uint64 `json:"Cache.APICalls"`
-}
-
 type cacheMetrics struct {
        requests          prometheus.Counter
        collectionBytes   prometheus.Gauge
@@ -157,19 +144,6 @@ var selectPDH = map[string]interface{}{
        "select": []string{"portable_data_hash"},
 }
 
-func (c *cache) Stats() cacheStats {
-       c.setupOnce.Do(c.setup)
-       return cacheStats{
-               Requests:          atomic.LoadUint64(&c.stats.Requests),
-               CollectionBytes:   c.collectionBytes(),
-               CollectionEntries: c.collections.Len(),
-               CollectionHits:    atomic.LoadUint64(&c.stats.CollectionHits),
-               PDHHits:           atomic.LoadUint64(&c.stats.PDHHits),
-               PermissionHits:    atomic.LoadUint64(&c.stats.PermissionHits),
-               APICalls:          atomic.LoadUint64(&c.stats.APICalls),
-       }
-}
-
 // Update saves a modified version (fs) to an existing collection
 // (coll) and, if successful, updates the relevant cache entries so
 // subsequent calls to Get() reflect the modifications.
@@ -195,8 +169,6 @@ func (c *cache) Update(client *arvados.Client, coll arvados.Collection, fs arvad
 
 func (c *cache) Get(arv *arvadosclient.ArvadosClient, targetID string, forceReload bool) (*arvados.Collection, error) {
        c.setupOnce.Do(c.setup)
-
-       atomic.AddUint64(&c.stats.Requests, 1)
        c.metrics.requests.Inc()
 
        permOK := false
@@ -208,7 +180,6 @@ func (c *cache) Get(arv *arvadosclient.ArvadosClient, targetID string, forceRelo
                        c.permissions.Remove(permKey)
                } else {
                        permOK = true
-                       atomic.AddUint64(&c.stats.PermissionHits, 1)
                        c.metrics.permissionHits.Inc()
                }
        }
@@ -222,7 +193,6 @@ func (c *cache) Get(arv *arvadosclient.ArvadosClient, targetID string, forceRelo
                        c.pdhs.Remove(targetID)
                } else {
                        pdh = ent.pdh
-                       atomic.AddUint64(&c.stats.PDHHits, 1)
                        c.metrics.pdhHits.Inc()
                }
        }
@@ -239,7 +209,6 @@ func (c *cache) Get(arv *arvadosclient.ArvadosClient, targetID string, forceRelo
                // likely, the cached PDH is still correct; if so,
                // _and_ the current token has permission, we can
                // use our cached manifest.
-               atomic.AddUint64(&c.stats.APICalls, 1)
                c.metrics.apiCalls.Inc()
                var current arvados.Collection
                err := arv.Get("collections", targetID, selectPDH, &current)
@@ -268,7 +237,6 @@ func (c *cache) Get(arv *arvadosclient.ArvadosClient, targetID string, forceRelo
        }
 
        // Collection manifest is not cached.
-       atomic.AddUint64(&c.stats.APICalls, 1)
        c.metrics.apiCalls.Inc()
        err := arv.Get("collections", targetID, nil, &collection)
        if err != nil {
@@ -359,7 +327,6 @@ func (c *cache) lookupCollection(key string) *arvados.Collection {
                c.collections.Remove(key)
                return nil
        }
-       atomic.AddUint64(&c.stats.CollectionHits, 1)
        c.metrics.collectionHits.Inc()
        return ent.collection
 }

diff --git a/services/keep-web/cache_test.go b/services/keep-web/cache_test.go
index cddeaf489763500b9e7230a75c2b19a4c25f40cf..d147573eec72d402faec43c21da86a010f13dc94 100644 (file)
--- a/services/keep-web/cache_test.go
+++ b/services/keep-web/cache_test.go
@@ -5,17 +5,36 @@
 package main
 
 import (
+       "bytes"
+
        "git.curoverse.com/arvados.git/sdk/go/arvados"
        "git.curoverse.com/arvados.git/sdk/go/arvadosclient"
        "git.curoverse.com/arvados.git/sdk/go/arvadostest"
+       "github.com/prometheus/client_golang/prometheus"
+       "github.com/prometheus/common/expfmt"
        "gopkg.in/check.v1"
 )
 
+func (s *UnitSuite) checkCacheMetrics(c *check.C, reg *prometheus.Registry, regs ...string) {
+       mfs, err := reg.Gather()
+       c.Check(err, check.IsNil)
+       buf := &bytes.Buffer{}
+       enc := expfmt.NewEncoder(buf, expfmt.FmtText)
+       for _, mf := range mfs {
+               c.Check(enc.Encode(mf), check.IsNil)
+       }
+       mm := buf.String()
+       for _, reg := range regs {
+               c.Check(mm, check.Matches, `(?ms).*collectioncache_`+reg+`\n.*`)
+       }
+}
+
 func (s *UnitSuite) TestCache(c *check.C) {
        arv, err := arvadosclient.MakeArvadosClient()
        c.Assert(err, check.Equals, nil)
 
        cache := DefaultConfig().Cache
+       cache.registry = prometheus.NewRegistry()
 
        // Hit the same collection 5 times using the same token. Only
        // the first req should cause an API call; the next 4 should
@@ -29,11 +48,12 @@ func (s *UnitSuite) TestCache(c *check.C) {
                c.Check(coll.PortableDataHash, check.Equals, arvadostest.FooPdh)
                c.Check(coll.ManifestText[:2], check.Equals, ". ")
        }
-       c.Check(cache.Stats().Requests, check.Equals, uint64(5))
-       c.Check(cache.Stats().CollectionHits, check.Equals, uint64(4))
-       c.Check(cache.Stats().PermissionHits, check.Equals, uint64(4))
-       c.Check(cache.Stats().PDHHits, check.Equals, uint64(4))
-       c.Check(cache.Stats().APICalls, check.Equals, uint64(1))
+       s.checkCacheMetrics(c, cache.registry,
+               "requests 5",
+               "hits 4",
+               "permission_hits 4",
+               "pdh_hits 4",
+               "api_calls 1")
 
        // Hit the same collection 2 more times, this time requesting
        // it by PDH and using a different token. The first req should
@@ -49,11 +69,12 @@ func (s *UnitSuite) TestCache(c *check.C) {
        c.Check(coll2.ManifestText[:2], check.Equals, ". ")
        c.Check(coll2.ManifestText, check.Not(check.Equals), coll.ManifestText)
 
-       c.Check(cache.Stats().Requests, check.Equals, uint64(5+1))
-       c.Check(cache.Stats().CollectionHits, check.Equals, uint64(4+0))
-       c.Check(cache.Stats().PermissionHits, check.Equals, uint64(4+0))
-       c.Check(cache.Stats().PDHHits, check.Equals, uint64(4+0))
-       c.Check(cache.Stats().APICalls, check.Equals, uint64(1+1))
+       s.checkCacheMetrics(c, cache.registry,
+               "requests 6",
+               "hits 4",
+               "permission_hits 4",
+               "pdh_hits 4",
+               "api_calls 2")
 
        coll2, err = cache.Get(arv, arvadostest.FooPdh, false)
        c.Check(err, check.Equals, nil)
@@ -61,11 +82,12 @@ func (s *UnitSuite) TestCache(c *check.C) {
        c.Check(coll2.PortableDataHash, check.Equals, arvadostest.FooPdh)
        c.Check(coll2.ManifestText[:2], check.Equals, ". ")
 
-       c.Check(cache.Stats().Requests, check.Equals, uint64(5+2))
-       c.Check(cache.Stats().CollectionHits, check.Equals, uint64(4+1))
-       c.Check(cache.Stats().PermissionHits, check.Equals, uint64(4+1))
-       c.Check(cache.Stats().PDHHits, check.Equals, uint64(4+0))
-       c.Check(cache.Stats().APICalls, check.Equals, uint64(1+1))
+       s.checkCacheMetrics(c, cache.registry,
+               "requests 7",
+               "hits 5",
+               "permission_hits 5",
+               "pdh_hits 4",
+               "api_calls 2")
 
        // Alternating between two collections N times should produce
        // only 2 more API calls.
@@ -80,11 +102,12 @@ func (s *UnitSuite) TestCache(c *check.C) {
                _, err := cache.Get(arv, target, false)
                c.Check(err, check.Equals, nil)
        }
-       c.Check(cache.Stats().Requests, check.Equals, uint64(5+2+20))
-       c.Check(cache.Stats().CollectionHits, check.Equals, uint64(4+1+18))
-       c.Check(cache.Stats().PermissionHits, check.Equals, uint64(4+1+18))
-       c.Check(cache.Stats().PDHHits, check.Equals, uint64(4+0+18))
-       c.Check(cache.Stats().APICalls, check.Equals, uint64(1+1+2))
+       s.checkCacheMetrics(c, cache.registry,
+               "requests 27",
+               "hits 23",
+               "permission_hits 23",
+               "pdh_hits 22",
+               "api_calls 4")
 }
 
 func (s *UnitSuite) TestCacheForceReloadByPDH(c *check.C) {
@@ -92,17 +115,19 @@ func (s *UnitSuite) TestCacheForceReloadByPDH(c *check.C) {
        c.Assert(err, check.Equals, nil)
 
        cache := DefaultConfig().Cache
+       cache.registry = prometheus.NewRegistry()
 
        for _, forceReload := range []bool{false, true, false, true} {
                _, err := cache.Get(arv, arvadostest.FooPdh, forceReload)
                c.Check(err, check.Equals, nil)
        }
 
-       c.Check(cache.Stats().Requests, check.Equals, uint64(4))
-       c.Check(cache.Stats().CollectionHits, check.Equals, uint64(3))
-       c.Check(cache.Stats().PermissionHits, check.Equals, uint64(1))
-       c.Check(cache.Stats().PDHHits, check.Equals, uint64(0))
-       c.Check(cache.Stats().APICalls, check.Equals, uint64(3))
+       s.checkCacheMetrics(c, cache.registry,
+               "requests 4",
+               "hits 3",
+               "permission_hits 1",
+               "pdh_hits 0",
+               "api_calls 3")
 }
 
 func (s *UnitSuite) TestCacheForceReloadByUUID(c *check.C) {
@@ -110,15 +135,17 @@ func (s *UnitSuite) TestCacheForceReloadByUUID(c *check.C) {
        c.Assert(err, check.Equals, nil)
 
        cache := DefaultConfig().Cache
+       cache.registry = prometheus.NewRegistry()
 
        for _, forceReload := range []bool{false, true, false, true} {
                _, err := cache.Get(arv, arvadostest.FooCollection, forceReload)
                c.Check(err, check.Equals, nil)
        }
 
-       c.Check(cache.Stats().Requests, check.Equals, uint64(4))
-       c.Check(cache.Stats().CollectionHits, check.Equals, uint64(3))
-       c.Check(cache.Stats().PermissionHits, check.Equals, uint64(1))
-       c.Check(cache.Stats().PDHHits, check.Equals, uint64(3))
-       c.Check(cache.Stats().APICalls, check.Equals, uint64(3))
+       s.checkCacheMetrics(c, cache.registry,
+               "requests 4",
+               "hits 3",
+               "permission_hits 1",
+               "pdh_hits 3",
+               "api_calls 3")
 }

diff --git a/services/keep-web/handler.go b/services/keep-web/handler.go
index d0ba431aa6312d64f44e518d5ca19d8826ad1c5c..bb77e5859449f5e7e4783d76d02120c359d51085 100644 (file)
--- a/services/keep-web/handler.go
+++ b/services/keep-web/handler.go
@@ -91,14 +91,7 @@ func (h *handler) setup() {
 }
 
 func (h *handler) serveStatus(w http.ResponseWriter, r *http.Request) {
-       status := struct {
-               cacheStats
-               Version string
-       }{
-               cacheStats: h.Config.Cache.Stats(),
-               Version:    version,
-       }
-       json.NewEncoder(w).Encode(status)
+       json.NewEncoder(w).Encode(struct{ Version string }{version})
 }
 
 // updateOnSuccess wraps httpserver.ResponseWriter. If the handler

diff --git a/services/keep-web/status_test.go b/services/keep-web/status_test.go
index 0a2b9eb988dce96c4791f7cbbaf0c602d5b16980..62db198dd9b9ef27618b9bfd04262b32ac2736f0 100644 (file)
--- a/services/keep-web/status_test.go
+++ b/services/keep-web/status_test.go
@@ -30,7 +30,6 @@ func (s *UnitSuite) TestStatus(c *check.C) {
        var status map[string]interface{}
        err := json.NewDecoder(resp.Body).Decode(&status)
        c.Check(err, check.IsNil)
-       c.Check(status["Cache.Requests"], check.Equals, float64(0))
        c.Check(status["Version"], check.Not(check.Equals), "")
 }