search

prometheus / prometheus

The Prometheus monitoring system and time series database.
https://prometheus.io/
Apache License 2.0
55.58k stars 9.14k forks source link

replace uber atomic with stdlib atomic types #14866

Open kruskall opened 1 month ago

kruskall commented 1 month ago

Proposal

Go 1.19 added new atomic types. Migrate usage of go.uber.org/atomic to sync/atomic and drop the dependency.

See https://go.dev/doc/go1.19#atomic_types

roidelapluie commented 1 month ago

I would like to keep using uber atomic:

  1. it helps up make sure that no unsafe method from the atomic package are called, by excluding sync/atomic completely
  2. it contains more types that we use (time, duration,...)
kruskall commented 1 month ago

Thank you for the comment! :bow:

it helps up make sure that no unsafe method from the atomic package are called, by excluding sync/atomic completely

Can you expand on this ? the new sync/atomic types should have the same syntax, if the concern is about the old methods you can exclude them to ensure you are only using the wrapped types.

roidelapluie commented 1 month ago

How can we exclude them?

kruskall commented 1 month ago

forbidigo is able to prevent specific functions from being used, it would need to be added to the linters in https://github.com/prometheus/prometheus/blob/main/.golangci.yml See https://golangci-lint.run/usage/linters/#forbidigo

beorn7 commented 1 month ago

I think the main selling point of go.uber.org/atomic at the time we introduce it was the prevention of 64bit alignment issues. The new atomic wrappers in the stdlib take care of that, too.

Assuming that there is no performance penalty, I would be all up for migrating "back" to the stdlib. (Lint rules to exclude the low level atomic operations would be good to have, but even without them, I prefer not to import a 3rd party dependency that is not really needed anymore.)