From 6db4c94527903f403e77ba4fce2d1d5fe4e29b03 Mon Sep 17 00:00:00 2001 From: Tom Clegg Date: Tue, 7 Aug 2018 18:09:25 -0400 Subject: [PATCH] 13198: Remove old counters from status.json. Arvados-DCO-1.1-Signed-off-by: Tom Clegg --- services/keep-web/cache.go | 33 ------------ services/keep-web/cache_test.go | 87 +++++++++++++++++++++----------- services/keep-web/handler.go | 9 +--- services/keep-web/status_test.go | 1 - 4 files changed, 58 insertions(+), 72 deletions(-) diff --git a/services/keep-web/cache.go b/services/keep-web/cache.go index b2bab78216..8336b78f9e 100644 --- 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, ¤t) @@ -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 cddeaf4897..d147573eec 100644 --- 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 d0ba431aa6..bb77e58594 100644 --- 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 0a2b9eb988..62db198dd9 100644 --- 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), "") } -- 2.39.5