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

Test data in api/prometheus/v1/api_test.go overflows on 32-bit archs #1132

Closed
dswarbrick opened this issue Sep 12, 2022 · 7 comments
Closed
Labels

Comments

@dswarbrick
Copy link
Contributor

The millisecond-resolution Unix timestamps (i.e., JavaScript-style) in the new TSDBHeadStats struct introduced by #925 will overflow on 32-bit archs

// TSDBHeadStats contains TSDB stats
type TSDBHeadStats struct {
	NumSeries     int `json:"numSeries"`
	NumLabelPairs int `json:"numLabelPairs"`
	ChunkCount    int `json:"chunkCount"`
	MinTime       int `json:"minTime"`
	MaxTime       int `json:"maxTime"`
}

A typical / recent Unix timestamp in millisecond resolution is approximately 760 times larger than a signed 32-bit int can hold.

The struct would need to be modified to use explicit int64 types, or if it is intended to be consumed by JavaScript clients, then it should really match the data type that timestamps are in the JavaScript world - i.e., float64 (JavaScript "number").

Test failure:

$ GOARCH=386 go test ./...
ok      github.com/prometheus/client_golang/api (cached)
# github.com/prometheus/client_golang/api/prometheus/v1 [github.com/prometheus/client_golang/api/prometheus/v1.test]
api/prometheus/v1/api_test.go:1155:23: cannot use 1634644800304 (untyped int constant) as int value in map literal (overflows)
api/prometheus/v1/api_test.go:1156:23: cannot use 1634650590304 (untyped int constant) as int value in map literal (overflows)
api/prometheus/v1/api_test.go:1188:21: cannot use 1634644800304 (untyped int constant) as int value in struct literal (overflows)
api/prometheus/v1/api_test.go:1189:21: cannot use 1634650590304 (untyped int constant) as int value in struct literal (overflows)
FAIL    github.com/prometheus/client_golang/api/prometheus/v1 [build failed]

It seems that unit tests are only being performed on 64-bit archs, allowing assumptions like this to slip past.

@dswarbrick
Copy link
Contributor Author

dswarbrick commented Sep 18, 2022

Does Circle CI support testing on 32-bit arch? If so, and it is enabled in the config, that would catch such oversights in future.

Alternatively, add a test on amd64 which runs with GOARCH=386.

@Dean-Coakley
Copy link
Contributor

@kakkoyun Would you be open to accepting a change to change all fields in the TSDBHeadStats struct to be int64? Or do you have something else in mind?

@stale
Copy link

stale bot commented Apr 2, 2023

Hello 👋 Looks like there was no activity on this issue for the last 3 months. Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity in the next 4 weeks, this issue will be closed (we can always reopen an issue if we need!).

@stale stale bot added the stale label Apr 2, 2023
@dswarbrick
Copy link
Contributor Author

It is still reproducible, as the underlying cause has not been addressed.

@stale
Copy link

stale bot commented Sep 17, 2023

Closing for now as promised, let us know if you need this to be reopened! 🤗

@stale stale bot closed this as completed Sep 17, 2023
@dswarbrick
Copy link
Contributor Author

Bad bot. Don't close this. It is not resolved yet.

@dswarbrick
Copy link
Contributor Author

dswarbrick commented Mar 7, 2024

Is there any plan to address this? We still have to patch it in Debian so that tests will pass on 32-bit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants