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

Implements a set() method for Counters #303

Closed
wants to merge 1 commit into from
Closed

Implements a set() method for Counters #303

wants to merge 1 commit into from

Conversation

jedisct1
Copy link
Contributor

Being able to set a counter is useful when a counter is set at scrape time, for example to the sum of other counters.

Prometheus doesn't have any issues with Counters getting reset.

@breezewish
Copy link
Member

Thanks! I'm mostly fine with the PR. However I'm curious that looks like this scenario should use a Gauge instead?

@jedisct1
Copy link
Contributor Author

jedisct1 commented Jan 6, 2020

Hi,

One reason is consistency.
For a concrete example, on a DNS server, the sum of queries received over UDP (counter) and queries received over TCP (counter) is intuitively expected to also be a counter.

Having it as a gauge seems also seems to be an issue for the munin monitoring tool: DNSCrypt/encrypted-dns-server#23

@breezewish
Copy link
Member

@jedisct1 How about increasing two counters simultaneously in this case? e.g. increase client_queries when increasing client_queries_udp, and increase client_queries when increasing client_queries_tcp. Notice that the official prometheus client library does not support setting the value for a counter as well and I'm very worried about encouraging abuse after the change.

@siddontang
Copy link
Contributor

setting the counter breaks the rule of Prometheus, see https://prometheus.io/docs/concepts/metric_types/#counter

So I think we still need to not allow this.

@jedisct1
Copy link
Contributor Author

jedisct1 commented Jan 6, 2020

Incrementing both counters is something to think about every time, and is bug-prone.

The proposed code ensures that the set operation can only increase a counter (the current debug_assert() can be turned into assert() if required).

@breezewish
Copy link
Member

@jedisct1 @jedisct1 @siddontang How about put these unrecommended methods behind a feature gate (like extra_helpers) which is not enabled by default, so that user have absolute awareness that these methods are not supported by standard Prometheus interfaces and user should totally know what he/she is doing when enabling such feature?

@phyber
Copy link

phyber commented Jan 19, 2020

This use case and the reset PR that added a reset (#261) method to counters, should be covered by ConstMetrics (#94, https://www.robustperception.io/setting-a-prometheus-counter). If this PR is to be disallowed, the reset feature should also be backed out. From the same Counter rules linked above: A counter is a cumulative metric that represents a single monotonically increasing counter whose value can only increase or be reset to zero on restart.

How about put these unrecommended methods behind a feature gate

That's probably a good workaround until ConstMetrics appear.

@mxinden
Copy link
Contributor

mxinden commented Jun 22, 2020

I am as well not in favor of adding a set method to Counter as I find it to be encouraging a wrong API usage.

Instead of introducing a feature gate I think people trying to reexpose counters from an underlying system (e.g. the OS) should implement custom collectors.

@jedisct1 I am closing this as I feel the discussion has stalled. Feel free to either reopen this pull request or post another if you would like to discuss this further.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants