Reload/refresh and using the default prometheus registry to instrument functions

[codification]

Hi,

I am having a little trouble using the default registry and instrumenting functions with it. At first I thought it was solely due to the fact that I use component and refresh/reload of all namespaces a lot so i defonce:ed the definition of the registry and made sure to do fn/initialize only once.

Like so:

(defonce registry (let [registry iapetos.registry/default]
                    (try
                      (-> registry
                          (fn/initialize))
                      (catch IllegalArgumentException e
                        registry))))

Still, this is what happens when I try to call an instrumented function

user> (require '[iapetos.core :as prometheus]
               '[iapetos.collector.fn :as fn])
nil
user> (defn my-fun-1 [] "EHLO")
#'user/my-fun-1
user> (fn/instrument! prometheus/default-registry #'my-fun-1)
#object[iapetos.registry.IapetosRegistry 0x78aa7886 "iapetos.registry.IapetosRegistry@78aa7886"]
user> (my-fun-1)
IllegalArgumentException No implementation of method: :increment* of protocol: #'iapetos.operations/IncrementableCollector found for class: nil  clojure.core/-cache-protocol-fn (core_deftype.clj:583)
user> 

I have checked that the fn_... metrics are registered in the export from the default registry, but I cannot seem to get instrumentation to work. Somehow it seems that I am caught between some protocol redefinition or cached reference in iapetos and what the Prometheus default registry thinks is already registered.

The above code works fine in a fresh project, void of any dependencies or code except for iapetos itself.

How would you suggest I go about using the default registry?
I would prefer not having to use it, but most other integrations (like, for example the logback appender) insist on using Prometheus CollectorRegistry/defaultRegistry.

[xsc]

Iapetos registries are "immutable" (not really, though, due to the underlying mutable Prometheus registry), so while fn/initialize (unfortunately) mutates the underlying registry, the iapetos wrapper is not updated. This means the default registry at that point knows nothing about the function instrumentation collectors and should not be used with fn/instrument!.

So, basically, the REPL session you pasted should always throw an exception, but if you replaced prometheus/default-registry with your own, once-defined registry above, it should work:

(defonce registry
  (-> prometheus/default-registry
      (fn/initialize)))

(fn/instrument! registry #'my-fun-1)

(my-fun-1)
;; => "EHLO"

Hope I didn't misunderstand your question and this helps!

[codification]

Ok, thanks. Still, I think (will verify) I get the same error using my "wrapped" registry - at least after a round of refresh/reload etc. The reason I posted is that it all seems to work fine in production/test where we only instantiate these things once but provides a terrible development experience when we constantly reload the namepaces and do bootstrapping all over again.
As far as I can understand, the fn/instrument! function is run upon every reload of the ns, but I guess that the defonce is not. Would that explain anything?

[codification]

Hi, I verified that the issue still stands. What I found is that (as I suspected) initially everything works as expected. After a refresh the instrumentation stops working.

Here is another repl-session:

user> (require '[my.metrics :refer [registry]])
nil
user> (require '[iapetos.core :as prometheus]
               '[iapetos.collector.fn :as fn])
nil
user> (-> registry
          (fn/initialize))
IllegalArgumentException Collector already registered that provides name: fn_duration_seconds_count  io.prometheus.client.CollectorRegistry.register (CollectorRegistry.java:54)

;; Verifies that the registry has already been setup the way I expected

user> 
user> (defn my-fun-1 [] "EHLO")
#'user/my-fun-1
user> (fn/instrument! registry #'my-fun-1)
#object[iapetos.registry.IapetosRegistry 0x387cd195 "iapetos.registry.IapetosRegistry@387cd195"]
user> 
user> (my-fun-1)
"EHLO"

;; Ok, that worked fine
;; now I do a full refresh of the running repl

;; refreshed, ns cleansed need to re-require
user> (require '[my.metrics :refer [registry]])
nil
user> (require '[iapetos.core :as prometheus]
               '[iapetos.collector.fn :as fn])
nil
user> (-> registry
          (fn/initialize))
IllegalArgumentException Collector already registered that provides name: fn_duration_seconds_count  io.prometheus.client.CollectorRegistry.register (CollectorRegistry.java:54)

;; Ok, same reaction
;; now, redefine function and instrument it anew

user> (defn my-fun-1 [] "EHLO")
#'user/my-fun-1
user> (fn/instrument! registry #'my-fun-1)
#object[iapetos.registry.IapetosRegistry 0x2b6f29b9 "iapetos.registry.IapetosRegistry@2b6f29b9"]
user> 
user> (my-fun-1)
IllegalArgumentException No implementation of method: :increment* of protocol: #'iapetos.operations/IncrementableCollector found for class: nil  clojure.core/-cache-protocol-fn (core_deftype.clj:583)
user>

Does that make anything clearer?

[xsc]

I tried it out, replicating your REPL session and what happens is that (refresh) re-created the registry binding, even though it was declared using defonce. This means, the following code gets executed:

(let [registry iapetos.registry/default]
  (try
    (-> registry
        (fn/initialize))
    (catch IllegalArgumentException e
      registry)))

Now, fn/initialize throws an exception since the collector is already attached to the defaultRegistry, you catch that exception and return iapetos.registry/default. And now we circle back to something I mentioned above: The collectors are attached to the underlying (mutable) registry but iapetos (with its immutable registries) knows nothing about them, resulting in your observed behaviour.

So, this is indeed a problem (and something I didn't think of when I introduced the default registry). One thing that could solve this is implementing iapetos.core/register as a combination of unregister and register to overwrite any existing collector on the underlying registry. (And this could be restricted to the default registry as that is the only point where we can lose control over the registry state.)

[codification]

Aha, thank you for investigating further! Sounds more or less like what I was expecting although expressed with insight into the actual implementation.

[codification]

I realized that perhaps I can get by by simply using the clear-method on the underlying collector registry every time I initialize (defonce) the iapetos registry.

[codification]

Something like this seems to work:

(ns iapetos.registry.default_registry_test
  (:require [iapetos.registry :as registry]
            [iapetos.collector.fn :as fn]
            [clojure.test :refer [deftest is testing]])
  (:import [io.prometheus.client CollectorRegistry]))

(defn init-default-registry []
  (do
    (-> registry/default
        ^CollectorRegistry (registry/raw)
        (.clear))
    (-> registry/default
        ;; ...other initializers or registrations
        (fn/initialize))))

(defn test-fn []
  "EHLO")

(deftest default-registry
  (is (some? (registry/raw registry/default)))

  (testing "default registry can be initialized a first time"
    (let [default-registry (init-default-registry)]
      (is (some? default-registry))
      (fn/instrument! default-registry #'test-fn)
      (is (string? (test-fn)))))

  (testing "default registry can be initialized a second time"
    (let [default-registry (init-default-registry)]
      (is (some? default-registry))
      (fn/instrument! default-registry #'test-fn)
      (is (string? (test-fn)))))
  )