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

metrics: use uint64 for Counter and Gauge types #4911

Merged
merged 4 commits into from
Jan 9, 2023

Conversation

cce
Copy link
Contributor

@cce cce commented Dec 14, 2022

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.

@codecov
Copy link

codecov bot commented Dec 14, 2022

Codecov Report

Merging #4911 (5f9a4bc) into master (787d386) will decrease coverage by 0.01%.
The diff coverage is 82.14%.

@@            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              
Impacted Files Coverage Δ
data/txHandler.go 72.07% <0.00%> (ø)
logging/usage.go 0.00% <0.00%> (ø)
node/node.go 4.28% <0.00%> (ø)
util/metrics/gauge.go 80.64% <66.66%> (-0.44%) ⬇️
ledger/metrics.go 100.00% <100.00%> (ø)
network/wsNetwork.go 64.92% <100.00%> (+0.17%) ⬆️
util/metrics/counter.go 90.32% <100.00%> (+2.15%) ⬆️
ledger/blockqueue.go 85.63% <0.00%> (-2.88%) ⬇️
ledger/testing/randomAccounts.go 55.65% <0.00%> (-0.62%) ⬇️
ledger/acctupdates.go 69.24% <0.00%> (-0.49%) ⬇️
... and 3 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@cce cce requested review from brianolson and winder December 14, 2022 23:07
Copy link
Contributor

@brianolson brianolson left a 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

@@ -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()
Copy link
Contributor

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?

Copy link
Contributor Author

@cce cce Jan 6, 2023

Choose a reason for hiding this comment

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

great idea, done!

@@ -28,7 +28,7 @@ type Gauge struct {
deadlock.Mutex
name string
description string
value float64
value uint64
Copy link
Contributor

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?

Copy link
Contributor Author

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) {
Copy link
Contributor

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

@algorandskiy algorandskiy merged commit 410d327 into algorand:master Jan 9, 2023
@cce cce deleted the uint64-metrics branch January 9, 2023 18:28
winder pushed a commit to winder/go-algorand that referenced this pull request Jan 18, 2023
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.

3 participants