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

promtool: Speed up checking for duplicate rules #9262

Closed
wants to merge 3 commits into from

Conversation

zecke
Copy link
Contributor

@zecke zecke commented Aug 27, 2021

Running promtool check config on large rule files is rather slow. Improve this by adding a testcase, benchmark and changing the algorithm used to find duplicates.

Before:
goos: darwin
goarch: amd64
pkg: github.com/prometheus/prometheus/cmd/promtool
cpu: VirtualApple @ 2.50GHz
BenchmarkCheckDuplicates-8   	       1	9543373875 ns/op

After:
goos: darwin
goarch: amd64
pkg: github.com/prometheus/prometheus/cmd/promtool
cpu: VirtualApple @ 2.50GHz
BenchmarkCheckDuplicates-8   	     187	   6313867 ns/op

@zecke zecke force-pushed the zecke/fasta branch 3 times, most recently from e9b1e5d to 6e446a2 Compare August 27, 2021 16:44
@roidelapluie roidelapluie requested a review from dgl August 28, 2021 16:37
@dgl
Copy link
Member

dgl commented Aug 31, 2021

@zecke thanks for this, looks good, but there's something strange with CircleCI build, could you merge main into this?

@zecke zecke force-pushed the zecke/fasta branch 4 times, most recently from 4cdf5d9 to 8f28f28 Compare August 31, 2021 09:56
@zecke
Copy link
Contributor Author

zecke commented Aug 31, 2021

@zecke thanks for this, looks good, but there's something strange with CircleCI build, could you merge main into this?

I lowered the parallelism in circle-ci but now tests fail in code not touched and in go mod sanity checking. I am not sure whether this is related to my changes at all. Needless to say that building locally works.

@roidelapluie
Copy link
Member

please revert your circleci changes. it is because you probably also have a circleci account.

@roidelapluie
Copy link
Member

the last test failure was done with a run with SSH, so it might have been tampered somehow.

@zecke
Copy link
Contributor Author

zecke commented Sep 6, 2021

please revert your circleci changes. it is because you probably also have a circleci account.

Reverted. How can we proceed? How can I not associate this with my account? Okay. I was able to unfollow the project which makes the build work/pass. PTAL now.

Introduce a basic test for checking for duplicate rules.

Signed-off-by: Holger Hans Peter Freyther <holger@moiji-mobile.com>
Add a simple benchmark with a large number of rules.

Signed-off-by: Holger Hans Peter Freyther <holger@moiji-mobile.com>
Trade space for speed. Convert all rules into our temporary struct, sort
and then iterate. This is a significant when having many rules.

Signed-off-by: Holger Hans Peter Freyther <holger@moiji-mobile.com>
@dgl
Copy link
Member

dgl commented Sep 7, 2021

Going to merge from my account as CircleCI seems unhappy with @zecke's account.

(Per previous comments the tests did succeed, but it didn't post the status to github, so the required check wouldn't let me merge.)

@dgl
Copy link
Member

dgl commented Sep 7, 2021

Thank you @zecke! & sorry for the trouble here, I've asked CircleCI if there's anything we can do better for future contributions.

I've merged your exact branch under #9306.

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.

3 participants