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

benchtool: avoid registering DNS metrics twice #188

Merged
merged 3 commits into from
Jun 5, 2021

Conversation

edma2
Copy link
Contributor

@edma2 edma2 commented Jun 2, 2021

Enabling both query and write capabilities (bench.query.enabled, bench.write.enabled) will result in this error on startup:

panic: duplicate metrics collector registration attempted

Use a different metric prefix for query and write runners.

@edma2 edma2 requested a review from a team as a code owner June 2, 2021 19:16
@gotjosh
Copy link
Collaborator

gotjosh commented Jun 3, 2021

Thanks @edma2 for your contribution! The changes LGTM. I think we could add a quick test to bench.go to avoid any further regressions and could you please add a changelog entry?

@edma2
Copy link
Contributor Author

edma2 commented Jun 3, 2021

@gotjosh I'm happy to add a test - but I'm unsure where to start. One idea is to call NewBenchRunner and check it doesn't fail, but looking at the code it might resolve DNS addresses as a side-effect which doesn't seem ideal for a unit test. Can you provide a pointer? Thanks!

@gotjosh
Copy link
Collaborator

gotjosh commented Jun 3, 2021

Ah, you're right. Adding this test doesn't look as simple as I originally thought.

In that case, I'm happy to proceed as is - can you please add a changelog entry? Something like this within cmd/bechtool should do the trick.

e.g.

Changelog
Order should be CHANGE, FEATURE, ENHANCEMENT, and BUGFIX

unreleased/master
[BUGFIX] Lorem ipsum #188

@edma2
Copy link
Contributor Author

edma2 commented Jun 3, 2021

Sure thing, added to CHANGELOG.

CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: gotjosh <josue.abreu@gmail.com>
@gotjosh
Copy link
Collaborator

gotjosh commented Jun 4, 2021

Thanks!

@gotjosh gotjosh merged commit 4c91805 into grafana:main Jun 5, 2021
simonswine pushed a commit to grafana/mimir that referenced this pull request Jan 12, 2022
friedrichg pushed a commit to cortexproject/cortex-tools that referenced this pull request Aug 1, 2023
Signed-off-by: Tom Hayward <thayward@infoblox.com>

improve CI rules

Signed-off-by: ShuzZzle <niclas.schad@gmail.com>
friedrichg pushed a commit to cortexproject/cortex-tools that referenced this pull request Aug 1, 2023
This reverts commit 6620fde.

Signed-off-by: Tom Hayward <thayward@infoblox.com>
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.

None yet

2 participants