-
Notifications
You must be signed in to change notification settings - Fork 493
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
metrics: use uint64 for Counter and Gauge types #4911
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4911 +/- ##
==========================================
- Coverage 53.64% 53.62% -0.02%
==========================================
Files 432 432
Lines 54058 54052 -6
==========================================
- Hits 28997 28987 -10
- Misses 22813 22817 +4
Partials 2248 2248
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
99% love it. about time. but we should go ahead and use sync/atomic
in this change
util/metrics/gauge.go
Outdated
@@ -60,14 +60,14 @@ func (gauge *Gauge) Deregister(reg *Registry) { | |||
} | |||
|
|||
// Add increases gauge by x | |||
func (gauge *Gauge) Add(x float64) { | |||
func (gauge *Gauge) Add(x uint64) { | |||
gauge.Lock() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
go ahead and use sync/atomic
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great idea, done!
util/metrics/gauge.go
Outdated
@@ -28,7 +28,7 @@ type Gauge struct { | |||
deadlock.Mutex | |||
name string | |||
description string | |||
value float64 | |||
value uint64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move to first member of struct for better sync/atomic
safety?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also done, thanks for the tip
// For adding an integer, see AddUint64(x) | ||
func (counter *Counter) Add(x float64, labels map[string]string) { | ||
// addLabels increases counter by x | ||
func (counter *Counter) addLabels(x uint64, labels map[string]string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that this was changed and nothing was lost because everything was already using AddUint64
Summary
Our counters and gauges support float64, but we never use anything but uint64, so this switches them to be uint64 metrics.
Test Plan
Updated tests in the metrics package.