client_python icon indicating copy to clipboard operation
client_python copied to clipboard

Calling clear() on Counter without labels fails

Open andrew-cybsafe opened this issue 3 years ago • 12 comments

To reproduce:

from prometheus_client import Counter

c = Counter("c", "c")
c.inc()
c.clear()

The error returned is:

AttributeError: 'Counter' object has no attribute '_lock'

Is using .clear() in this way supported?

andrew-cybsafe avatar Oct 13 '21 13:10 andrew-cybsafe

Clear is for clearing a labelset, so it should bug out here but probably it needs a better error message.

If your objective is to set a counter back to some starting value then you are using the wrong metric, you need a Gauge. From the information on counter:

Do not use a counter to expose a value that can decrease. For example, do not use a counter for the number of currently running processes; instead use a gauge.

sdfordham avatar Oct 13 '21 15:10 sdfordham

Exactly. We would benefit for a better error message.

roidelapluie avatar Oct 13 '21 15:10 roidelapluie

I also thought clear was for resetting things (Counters, Histograms). I tried to call it in my test suite so I could always start with a known, simple state (0).

exarkun avatar Dec 10 '21 21:12 exarkun

(And I thought it was a bug in prometheus_client that it raised an exception - until I read this ticket)

exarkun avatar Dec 10 '21 21:12 exarkun

I thought maybe I should just make a new Histogram to replace the Histogram I had wanted to clear ... but nope, that's a collision in the registry.

So ... it would be nice if there were a way to reset everything to 0 for testing purposes. Right now I'm calling the private _metric_init which seems to work for Histogram but I dunno about the other types.

exarkun avatar Dec 10 '21 21:12 exarkun

Clear is for clearing a labelset, so it should bug out here but probably it needs a better error message.

If you treat having no labels as a degenerate case, then it would make sense for it to not error out here.

I understand the point about not setting Counter back to 0 in particular, but if you have a fixed-ish set of label values (perhaps 'successandfailure), you probably have exactly the same issue with .clear()` currently.

tomprince avatar Dec 10 '21 21:12 tomprince

So ... it would be nice if there were a way to reset everything to 0 for testing purposes.

Yes I think this is sensible, probably should be private or it will confuse people because of the mantra about "counters only go up".

If you treat having no labels as a degenerate case, then it would make sense for it to not error out here.

I think I agree since clear is not private.

sdfordham avatar Dec 10 '21 22:12 sdfordham

Yes I think this is sensible, probably should be private or it will confuse people because of the mantra about "counters only go up".

I would recommend adding a testing namespace somewhere (eg prometheus_client.testing) and placing such things there, with otherwise public names (eg prometheus_client.testing.reset_counter). This avoids the confusion of the typical private convention (If it is private, am I intended to use it outside of prometheus_client? Usually the answer would be no.) but still makes it quite clear that if you use this API outside of testing then you're doing something wrong (and this message can be augmented with good docstrings on the testing module/package and the individual APIs themselves).

exarkun avatar Dec 13 '21 13:12 exarkun

I do agree that this should not error in the without labels case. It might just not do anything which is fine as the counter would already be cleared.

I do actually think it could make sense to set the value to 0 though. This would be effectively forcing a counter reset which Prometheus would handle similar to if the process was restarted. It would also be the same as if you cleared all the labels then re-added one as that would start at 0 again.

csmarchbanks avatar Dec 14 '21 18:12 csmarchbanks

Use case:

I have a metric deployment{name="my-deployment"} that is either 1 or 0, depending on the state.

If the deployment goes away the deployment metric will be stale. I need to clear them out so no stale metrics are left.

This works just fine from the REPL.

>>> import prometheus_client
>>> gauge=prometheus_client.Gauge("test","Test Gauage",["name"])
>>> gauge.clear()
>>>

hholst80 avatar Jan 29 '23 15:01 hholst80

Redesigned the solution to be a pure wsgi application instead of some hybrid.

  from prometheus_client import Gauge, make_wsgi_app, CollectorRegistry, genera
  import kwrapi

  app = make_wsgi_app()

  @app.route("/metrics")
  def metrics():
      registry = CollectorRegistry()
      gauges = {
              "availableReplicas": Gauge("replicas_available", "Replicas availa
              "readyReplicas": Gauge("replicas_ready", "Replicas ready", ["name
              "updatedReplicas": Gauge("replicas_updated", "Updated replicas",
              "replicas": Gauge("replicas", "Replicas", ["name"], registry=regi
              }

      deployments = []
      with kwrapi.session() as session:
          request = kwrapi.get_deployments()
          response = session.send(request)
          deployments.extend(response.json().get("items", []))

      for deployment in deployments:
          metadata = deployment["metadata"]
          status = deployment.get("status", {})
          for key, gauge in gauges.items():
              _ = status.get(key)
              if _ is not None:
                  gauge.labels(name=metadata["name"]).set(_)

      response = generate_latest(registry).decode()
      return response
~

hholst80 avatar Jan 29 '23 16:01 hholst80

Addendum. I had to use the flask Application instead because the WSGI implementation required a predefined registry as input. I wanted to generate a new one for each call.

hholst80 avatar Jan 29 '23 21:01 hholst80