Add ::path-cache to registry collectors map metadata (#59)

Author: kgann

dev/bench.clj

@@ -0,0 +1,81 @@
+(ns bench
+  (:require [clojure.test.check.generators :as gen]
+            [iapetos.core :as prometheus]
+            [iapetos.registry :as r]
+            [iapetos.registry.collectors :as c]
+            [iapetos.test.generators :as g]
+            [jmh.core :as jmh])
+  (:import [iapetos.registry IapetosRegistry]))
+
+(defn metrics
+  [metric-count]
+  (gen/sample g/metric metric-count))
+
+(def dirty-string-metric
+  (gen/let [first-char    gen/char-alpha
+            invalid-chars (gen/return (apply str (map char (range 33 45))))
+            last-char     gen/char-alphanumeric
+            rest-chars    gen/string-alphanumeric]
+    (gen/return
+     (str
+      (apply str first-char invalid-chars rest-chars)
+      last-char))))
+
+(defn dirty-metrics
+  [metric-count]
+  (gen/sample dirty-string-metric metric-count))
+
+;; JMH fns
+
+(defn collectors
+  [^IapetosRegistry registry]
+  (.-collectors registry))
+
+(defn register-collectors
+  [metrics]
+  (reduce (fn [reg metric]
+            (r/register reg metric (prometheus/counter metric)))
+          (r/create)
+          metrics))
+
+(defn lookup
+  [collectors metric]
+  (c/lookup collectors metric {}))
+
+(def bench-env
+  {:benchmarks [{:name :registry-lookup
+                 :fn   `lookup
+                 :args [:state/collectors :state/metric]}
+
+                {:name :dirty-registry-lookup
+                 :fn   `lookup
+                 :args [:state/dirty-collectors :state/dirty-metric]}]
+
+   :states {:dirty-metrics    {:fn `dirty-metrics :args [:param/metric-count]}
+            :dirty-metric     {:fn `rand-nth :args [:state/dirty-metrics]}
+            :dirty-registry   {:fn `register-collectors :args [:state/dirty-metrics]}
+            :dirty-collectors {:fn `collectors :args [:state/dirty-registry]}
+
+            :metrics    {:fn `metrics :args [:param/metric-count]}
+            :metric     {:fn `rand-nth :args [:state/metrics]}
+            :registry   {:fn `register-collectors :args [:state/metrics]}
+            :collectors {:fn `collectors :args [:state/registry]}}
+
+   :params {:metric-count 500}
+
+   :options {:registry-lookup       {:measurement {:iterations 1000}}
+             :dirty-registry-lookup {:measurement {:iterations 1000}}
+             :jmh/default           {:mode             :average
+                                     :output-time-unit :us
+                                     :measurement      {:iterations 1000
+                                                        :count      1}}}})
+
+(def bench-opts
+  {:type   :quick
+   :params {:metric-count 500}})
+
+(comment
+
+  (map #(select-keys % [:fn :mode :name :score]) (jmh/run bench-env bench-opts))
+
+  )

project.clj

@@ -17,7 +17,9 @@
                  [io.prometheus/simpleclient_hotspot "0.12.0" :scope "provided"]]
   :profiles {:dev
              {:dependencies [[org.clojure/test.check "1.1.0"]
-                             [aleph "0.4.6"]]
+                             [aleph "0.4.6"]
+                             [jmh-clojure "0.4.1"]]
+              :source-paths ["dev"]
               :global-vars {*warn-on-reflection* true}}
              :codox
              {:plugins [[lein-codox "0.10.0"]]

src/iapetos/collector.clj

@@ -15,6 +15,8 @@
      registries.")
   (metric [this]
     "Return a `iapetos.metric/Metric` for this collector.")
+  (metric-id [this]
+    "Return user supplied (unsanitized) identifier for this collector.")
   (label-instance [this instance values]
     "Add labels to the given collector instance produced by `instantiate`."))
 
@@ -55,6 +57,7 @@
 (defrecord SimpleCollectorImpl [type
                                 namespace
                                 name
+                                metric-id
                                 description
                                 subsystem
                                 labels
@@ -74,6 +77,8 @@
   (metric [_]
     {:name      name
      :namespace namespace})
+  (metric-id [_]
+    metric-id)
   (label-instance [_ instance values]
     (set-labels instance labels values)))
 
@@ -85,14 +90,16 @@
            ^String subsystem
            ^String description
            labels
-           lazy?]}
+           lazy?]
+    ::metric/keys [id]}
    collector-type
    builder-constructor]
   {:pre [type name namespace description]}
   (map->SimpleCollectorImpl
     {:type                collector-type
      :namespace           namespace
      :name                name
+     :metric-id           id
      :description         description
      :subsystem           subsystem
      :labels              (label-names labels)
@@ -119,6 +126,8 @@
     this)
   (metric [this]
     (raw-metric this))
+  (metric-id [this]
+    (hash this))
   (label-instance [_ instance values]
     (if-not (empty? values)
       (throw (UnsupportedOperationException.))
@@ -133,5 +142,7 @@
       (instantiate collector options))
     (metric [_]
       metric)
+    (metric-id [_]
+      metric)
     (label-instance [_ instance values]
       (label-instance collector instance values))))

src/iapetos/collector/exceptions.clj

@@ -43,6 +43,8 @@
     (collector/instantiate counter registry-options))
   (metric [this]
     (collector/metric counter))
+  (metric-id [this]
+    (collector/metric-id counter))
   (label-instance [_ instance values]
     (->ExceptionCounterChild counter instance values)))
 

src/iapetos/metric.clj

@@ -62,6 +62,7 @@
 
 (defn as-map
   [metric additional-keys]
-  (merge
-    (metric-name metric)
-    additional-keys))
+  (merge (metric-name metric)
+         additional-keys
+         ;; user supplied (unsanitized) identifier e.g., :app/runs-total
+         {::id metric}))

src/iapetos/registry.clj

@@ -56,7 +56,7 @@
       registry-name
       registry
       (update options :subsystem utils/join-subsystem subsystem-name)
-      {}))
+      (collectors/initialize)))
   (get [_ metric labels]
     (collectors/by collectors metric labels options))
   (raw [_]

src/iapetos/registry/collectors.clj

@@ -1,13 +1,18 @@
 (ns iapetos.registry.collectors
-  (:require [iapetos.registry.utils :as utils]
+  (:require [iapetos.metric :as metric]
+            [iapetos.registry.utils :as utils]
             [iapetos.collector :as collector])
   (:import [io.prometheus.client Collector CollectorRegistry]))
 
 ;; ## Init
 
 (defn initialize
+  "Initialize map for collectors.
+
+   - ::path-cache meta used to prevent additional metric path computation
+  requiring metric name sanitization."
   []
-  {})
+  ^{::path-cache {}} {})
 
 ;; ## Management
 
@@ -39,22 +44,26 @@
         instance (collector/instantiate collector options)]
     (-> {:collector  collector
          :metric     metric
+         :cache-key  [(collector/metric-id collector) options]
          :path       path
          :raw        instance
          :register   (register-collector-delay registry instance)
          :unregister (unregister-collector-delay registry instance)}
         (warn-lazy-deprecation!))))
 
 (defn insert
-  [collectors {:keys [path] :as collector}]
-  (assoc-in collectors path collector))
+  [collectors {:keys [path cache-key] :as collector}]
+  (-> collectors
+      (vary-meta update ::path-cache assoc cache-key path)
+      (assoc-in path collector)))
 
 (defn delete
-  [collectors {:keys [path] :as collector}]
-  (update-in collectors
-             (butlast path)
-             dissoc
-             (last path)))
+  [collectors {:keys [path cache-key] :as _collector}]
+  (-> collectors
+      (vary-meta update ::path-cache dissoc cache-key)
+      (update-in (butlast path)
+                 dissoc
+                 (last path))))
 
 (defn unregister
   [collectors {:keys [register unregister] :as collector}]
@@ -69,9 +78,9 @@
 
 (defn clear
   [collectors]
-  (->> (for [[namespace vs] collectors
-             [subsystem vs] vs
-             [_ collector] vs]
+  (->> (for [[_namespace vs] collectors
+             [_subsystem vs] vs
+             [_name collector] vs]
          collector)
        (reduce unregister collectors)))
 
@@ -83,7 +92,9 @@
 
 (defn lookup
   [collectors metric options]
-  (some->> (utils/metric->path metric options)
+  (some->> (or (get-in (meta collectors)
+                       [::path-cache [metric options]])
+               (utils/metric->path metric options))
            (get-in collectors)))
 
 (defn by

test/iapetos/metric_test.clj

@@ -44,4 +44,5 @@
     (let [{:keys [name namespace] :as r} (metric/as-map metric additional-keys)]
       (and (re-matches #"[a-zA-Z0-9_]+" name)
            (re-matches #"[a-zA-Z0-9_]+" namespace)
-           (= additional-keys (dissoc r :name :namespace))))))
+           (= additional-keys (dissoc r :name :namespace ::metric/id))
+           (= metric (::metric/id r))))))

test/iapetos/registry/collectors_test.clj

@@ -0,0 +1,45 @@
+(ns iapetos.registry.collectors-test
+  (:require [clojure.test.check
+             [generators :as gen]
+             [properties :as prop]
+             [clojure-test :refer [defspec]]]
+            [clojure.test :refer :all]
+            [iapetos.test.generators :as g]
+            [iapetos.core :as prometheus]
+            [iapetos.collector :as collector]
+            [iapetos.registry.collectors :as c])
+  (:import [iapetos.registry IapetosRegistry]))
+
+(defn- all-collectors
+  [^IapetosRegistry registry]
+  (for [[_namespace vs] (.-collectors registry)
+        [_subsystem vs] vs
+        [_name collector] vs]
+    collector))
+
+(defn- path-cache
+  [^IapetosRegistry registry]
+  (::c/path-cache (meta (.-collectors registry))))
+
+(defspec t-registry-should-cache-the-path-of-registered-collectors 50
+  (prop/for-all
+   [registry              (gen/return (prometheus/collector-registry))
+    collector-constructor g/collector-constructor
+    collector-name        g/metric]
+   (let [collector  (collector-constructor collector-name)
+         registry   (prometheus/register registry collector)
+         cache      (path-cache registry)]
+     (is (every? (fn [{:keys [path cache-key] :as _collector}]
+                   (= path (get cache cache-key)))
+                 (all-collectors registry))))))
+
+(defspec t-registry-should-evict-path-from-cache-for-unregistered-collectors 50
+  (prop/for-all
+   [registry              (gen/return (prometheus/collector-registry))
+    collector-constructor g/collector-constructor
+    collector-name        g/metric]
+   (let [collector  (collector-constructor collector-name)
+         registry   (-> registry
+                        (prometheus/register collector)
+                        (prometheus/unregister collector))]
+     (is (empty? (path-cache registry))))))