Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue #11/clearing default registry #12

Conversation

codification
Copy link

I added a simple test-case illustrating the issue as well as a workaround. Also added a caveat to the README.
Use if you like.

Ville Svärd added 2 commits October 10, 2017 23:13
- seems to work _although_
  - underlying registry will be wiped
  - any references to old "default" registries lying around will be
    invald
@codification
Copy link
Author

Regarding #11

@marco-m
Copy link

marco-m commented Jul 9, 2018

@xsc any news about this PR ? Should it be accepted, modified or declined ?

@delitescere
Copy link

delitescere commented Jul 19, 2018

The problem is that things can register collectors with the default registry outside of the Iapetos lifecycle (e.g. log4j2).

The latest changes to Iapetos allow easy registering/de-registering (and clearing) of collectors Iapetos has handled. In no circumstances should Iapetos de-register or clear anything it hasn't registered.

If you're using something like Sierra's components life is now very good:

(ns component.prometheus
  (:require [com.stuartsierra.component :refer [Lifecycle start stop]]
            [iapetos.core :as metrics]))

(defrecord Prometheus []
  Lifecycle

  (start [component]
    (-> component
      (assoc :registry ((apply comp (:metrics-collectors component)) metrics/default-registry))))

  (stop [component]
    (try
      (metrics/clear (:registry component))
      (catch Exception _))
    (-> component
      (assoc :metrics-collectors nil)
      (assoc :registry nil))))

Note the metrics/default-registry in the start, and the metrics/clear in the stop. 💯

@marco-m
Copy link

marco-m commented Jul 20, 2018

thanks for the explanation!

@MalloZup
Copy link
Member

@delitescere @marco-m @codification . I didn't dive to much on this PR but afaik what stated here: #12 (comment)
make this PR not desirable to merge.

Please correct me if I am wrong thx.

@MalloZup
Copy link
Member

I'm losing this since is resolved thx!

@MalloZup MalloZup closed this Jul 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants