Skip to content

Conversation

@cce
Copy link

@cce cce commented Feb 18, 2025

Steps to reproduce:

go mod edit -go=1.23
go get golang.org/x/sys
go get golang.org/x/crypto
go get golang.org/x/net
go get github.com/prometheus/[email protected]
go mod tidy

also changed calls to renamed functions in updated github.com/prometheus/procfs package and related dependencies.

Copy link

@algorandskiy algorandskiy left a comment

Choose a reason for hiding this comment

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

I guess ubuntu 20.04 build image also needs to be upgraded

@cce cce requested a review from algorandskiy February 18, 2025 20:47
@gmalouf
Copy link

gmalouf commented Feb 19, 2025

@onetechnical I think we should run this locally or in one of our build environments before updating packages in algod.

c.NFSTransportIdleTimeSeconds,
prometheus.GaugeValue,
s.Transport.IdleTime.Seconds(),
float64(s.Transport.IdleTimeSeconds%float64Mantissa),

Choose a reason for hiding this comment

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

What the heck is this doing?

Copy link
Author

@cce cce Feb 24, 2025

Choose a reason for hiding this comment

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

This was copied from prometheus#1364 which updated the upstream dependency, while fixing a rollover bug in the handling of some values collected. The function .Seconds() was removed from the upstream library, I think to force downstream consumers to do their own unit conversions (?). Because counters in Prometheus are meant to "roll over" to 0, usually when they hit some kind of max like MaxInt64, or otherwise, he is manually implementing a custom max here (Grafana and other systems handle metrics rollovers pretty well when deriving values like rates, etc). Since this is a float64 counter (I think for legacy reasons, this was a float before, so he is preserving its type?), I believe he wanted to pick a max value that would safely roll over. On his commit he left the comment Rollover counter automatically to avoid float64 accuracy issues. so I think he is trying to avoid some precision cliff?

Choose a reason for hiding this comment

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

I think I get it. That value is probably the largest that can be represented precisely in a float64.

@cce cce requested a review from jannotti February 24, 2025 19:55
@cce cce merged commit def45ac into master Feb 25, 2025
4 checks passed
@cce cce deleted the update-deps branch February 25, 2025 13:46
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.

5 participants