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

Change Integer Counter type from AtomicI64 to AtomicU64 #365

Merged
merged 3 commits into from
Nov 13, 2020

Conversation

Folyd
Copy link
Contributor

@Folyd Folyd commented Nov 6, 2020

According to the definition of Prometheus's Counter type, the AtomicU64 is preferred to AtomicI64 for IntCounter* types:

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.

Fix #364 .

Copy link
Contributor

@mxinden mxinden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am in favor of this change. Thanks.

I find the name IntCounter a bit confusing, as my intuition would include negative numbers as well, which (rightly so) doesn't make any sense for a Prometheus counter. I am failing to come up with a better name.

While already enforced through the type system, I suggest to make the following changes to the type signature comments to emphasize the fact that one can only to pass natural numbers.

@tim-seoss
Copy link

It might be useful to also document (not sure where's best, maybe along with the "better performance" notes?), that these integer counters will wrap with inc and inc_by (and that this is acceptable, so long as it doesn't wrap more than once between samples).

Not sure if that's in scope for this PR tho'.

@mxinden
Copy link
Contributor

mxinden commented Nov 12, 2020

It might be useful to also document (not sure where's best, maybe along with the "better performance" notes?), that these integer counters will wrap with inc and inc_by (and that this is acceptable, so long as it doesn't wrap more than once between samples).

I am in favor of this, but as you said, I think it is out of scope for this pull request. @tim-seoss would you mind opening up a new pull request doing the suggested changes?

@mxinden
Copy link
Contributor

mxinden commented Nov 12, 2020

Unless there are any objections I will merge this pull request later today.

Copy link
Member

@breezewish breezewish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

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.

Integer Counter type uses signed type (AtomicI64), when unsigned might be preferrable?
4 participants