refactor: replace uber atomic with stdlib atomic types (Issue #14866)#17358
refactor: replace uber atomic with stdlib atomic types (Issue #14866)#17358Gokula-6sg wants to merge 3 commits intoprometheus:mainfrom
Conversation
Gokula-6sg
requested review from
bwplotka,
cstyan,
dgl,
jesusvazquez,
juliusv and
tomwilkie
as code owners
.golangci.yml
Outdated
| @@ -84,7 +84,7 @@ linters: | |||
| main: | |||
| deny: | |||
| - pkg: "sync/atomic" | |||
There was a problem hiding this comment.
You should also replace this with "go.uber.org/atomic"
There was a problem hiding this comment.
Thanks for catching that! I've updated the .golangci.yml to deny go.uber.org/atomic instead of sync/atomic.
There was a problem hiding this comment.
Thanks for the clarification! I’ve updated .golangci.yml as suggested. I’ll run make lint locally to verify everything passes before pushing the changes.
| // Wait until the mock server receives at least two requests from Prometheus. | ||
| require.Eventually(t, func() bool { | ||
| return requestCount.Load() >= 2 | ||
| return atomic.LoadInt32(&requestCount) >= 2 |
There was a problem hiding this comment.
Was there a reason why you used atomic.LoadInt32() instead of leaving requestCount as atomic.Int32 and using the Load() function here? According to the documentation, Consider using the more ergonomic and less error-prone Int32.Load instead., it's preferable to use the Load() function.
There was a problem hiding this comment.
Agreed, this is the wrong way to go.
| func (r *AlertingRule) SetLastError(err error) { | ||
| r.lastError.Store(err) | ||
| if err != nil { | ||
| r.lastError.Store(err) |
There was a problem hiding this comment.
I'm not sure it's a good idea to ignore nil values. To avoid panic during Store(), you can use a wrapper structure that can contains a nil value. You can write a utility structure in utils/atomicutils using generics, which will eliminate two problems at once: storing nil values and safely casting types during Load(). You can take a look at the implementation in my branch. It might not be perfect either, so feel free to modify it.
|
Hi @Gokula-6sg , |
bboreham
left a comment
bboreham
left a comment
There was a problem hiding this comment.
Thanks for doing this. Seems there are a few corners to explore still.
| toolkit_web "github.com/prometheus/exporter-toolkit/web" | ||
| "go.uber.org/atomic" | ||
| "sync/atomic" | ||
|
|
| // Wait until the mock server receives at least two requests from Prometheus. | ||
| require.Eventually(t, func() bool { | ||
| return requestCount.Load() >= 2 | ||
| return atomic.LoadInt32(&requestCount) >= 2 |
There was a problem hiding this comment.
Agreed, this is the wrong way to go.
| addSamplesTotal := func(delta float64) { | ||
| for { | ||
| oldBits := samplesTotal.Load() | ||
| oldVal := math.Float64frombits(oldBits) | ||
| newVal := oldVal + delta | ||
| newBits := math.Float64bits(newVal) | ||
| if samplesTotal.CompareAndSwap(oldBits, newBits) { | ||
| break | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This seems over-complicated. Just add the int which comes from len. samplesTotal should be fine as a Uint64.
| (series.lastHistogramValue != nil && value.IsStaleNaN(series.lastHistogramValue.Sum)) || | ||
| (series.lastFloatHistogramValue != nil && value.IsStaleNaN(series.lastFloatHistogramValue.Sum)) { | ||
| numStaleSeries.Dec() | ||
| numStaleSeries.Add(^uint64(0)) // Equivalent to -1 for unsigned overflow |
There was a problem hiding this comment.
Make it an atomic.Int64 then you can Add(-1) and save explaining.
|
Can you rebase this on main please @Gokula-6sg? |
|
Hello from the bug-scrub! @Gokula-6sg do you think you will come back to this? |
5 participants
Which issue(s) does the PR fix:
go.uber.org/atomicwith stdlibsync/atomic.Does this PR introduce a user-facing change?