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.

Sign up for GitHub

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

Jump to bottom

refactor: mimirtool analyze prometheus: add concurrency and resiliency #3349

Merged
merged 1 commit into from
Nov 9, 2022
Merged

refactor: mimirtool analyze prometheus: add concurrency and resiliency #3349

merged 1 commit into from
Nov 9, 2022

Conversation

anilmisirlioglu
Copy link
Contributor

@anilmisirlioglu anilmisirlioglu commented Nov 1, 2022 •
edited by osg-grafana
Loading

What this PR does

This PR adds resiliency and concurrency in analyze Prometheus command. Removes duplicate codes and optimizes code. It reduces the working time considerably. Notably performance increment as you can see the following:

image__51_

Which issue(s) this PR fixes or relates to

Fixes #3062

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

cc: @Dentrax @aysenrsrt

@anilmisirlioglu anilmisirlioglu requested a review from a team as a code owner November 1, 2022 13:31
@CLAassistant
Copy link

CLAassistant commented Nov 1, 2022 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@anilmisirlioglu anilmisirlioglu force-pushed the refactor/analyse-prometheus branch 3 times, most recently from e4ffc72 to 3db7b9b Compare November 1, 2022 13:49
@osg-grafana
Copy link
Contributor

Unchecked documentation in description because it does not yet exist; does this change need documentation? Will a user know to what extent it could be used?

@Dentrax
Copy link
Contributor

Dentrax commented Nov 1, 2022

Since we've respected as-is design during refactor, this PR does not include any behavioural change from the end user perspective. (Only --concurrent flag, which is just introduced). So no documentation needed IMO.

Logiraptor
Logiraptor reviewed Nov 2, 2022
View reviewed changes
@Logiraptor
Copy link
Contributor

Thanks for working on this!

@anilmisirlioglu anilmisirlioglu force-pushed the refactor/analyse-prometheus branch 2 times, most recently from e05b094 to 979826b Compare November 2, 2022 07:14
@anilmisirlioglu
Copy link
Contributor Author

Thanks! Just resolved your reviews, can you please check again?

dimitarvdimitrov
dimitarvdimitrov reviewed Nov 2, 2022
View reviewed changes
Copy link
Contributor

@dimitarvdimitrov dimitarvdimitrov left a comment

Choose a reason for hiding this comment

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

Amazing, thanks for working on this! I have a few comments, most of which are minor

@anilmisirlioglu anilmisirlioglu force-pushed the refactor/analyse-prometheus branch from 979826b to 38d47bc Compare November 2, 2022 12:59
dimitarvdimitrov
dimitarvdimitrov approved these changes Nov 2, 2022
View reviewed changes
Copy link
Contributor

@dimitarvdimitrov dimitarvdimitrov left a comment

Choose a reason for hiding this comment

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

apart from that minor godoc/usage difference, LGTM!

@anilmisirlioglu anilmisirlioglu force-pushed the refactor/analyse-prometheus branch from 38d47bc to 5694fac Compare November 3, 2022 10:38
Logiraptor
Logiraptor approved these changes Nov 8, 2022
View reviewed changes
Copy link
Contributor

@Logiraptor Logiraptor left a comment

Choose a reason for hiding this comment

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

LGTM. We can merge as soon as the CI is passing. I believe you just need to run this command and commit the change:

goimports -w -local github.com/grafana/mimir pkg/mimirtool/commands/analyse_prometheus.go

@anilmisirlioglu
refactor: add resiliency & concurrency and optimize code
b4ce498 
Fixes grafana#3062

Signed-off-by: Anıl Mısırlıoğlu <[email protected]>
Co-authered-by: Furkan Türkal <[email protected]>
Co-authered-by: Ayşe Sert <[email protected]>
@anilmisirlioglu anilmisirlioglu force-pushed the refactor/analyse-prometheus branch from 5694fac to b4ce498 Compare November 8, 2022 18:03
@anilmisirlioglu
Copy link
Contributor Author

I run and commit it. I hope the linter is fixed.

@Logiraptor Logiraptor enabled auto-merge (squash) November 9, 2022 20:19
@Logiraptor Logiraptor disabled auto-merge November 9, 2022 20:20
@Logiraptor Logiraptor merged commit cbcc350 into grafana:main Nov 9, 2022
pracucci
pracucci reviewed Nov 17, 2022
View reviewed changes
CHANGELOG.md
@@ -63,6 +63,8 @@
### Mimirtool

* [ENHANCEMENT] Added `mimirtool rules delete-namespace` command to delete all of the rule groups in a namespace including the namespace itself. #3136
* [ENHANCEMENT] Refactor `mimirtool analyze prometheus`: add concurrency and resiliency #3062
Copy link
Collaborator

Choose a reason for hiding this comment

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

We're used to reference the PR number instead of the issue number (so #3349 instead of #3062). This will make our release tooling matching the PR when checking the CHANGELOG.

@anilmisirlioglu would you mind opening a follow up PR to change it, please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure

@anilmisirlioglu anilmisirlioglu mentioned this pull request Nov 17, 2022
Merged
3 tasks
masonmei pushed a commit to udmire/mimir that referenced this pull request Dec 16, 2022
@anilmisirlioglu
refactor: add resiliency & concurrency and optimize code (grafana#3349)
bcf746f 
Fixes grafana#3062

Signed-off-by: Anıl Mısırlıoğlu <[email protected]>
Co-authered-by: Furkan Türkal <[email protected]>
Co-authered-by: Ayşe Sert <[email protected]>

Signed-off-by: Anıl Mısırlıoğlu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pracucci pracucci pracucci left review comments

@dimitarvdimitrov dimitarvdimitrov dimitarvdimitrov approved these changes

@Logiraptor Logiraptor Logiraptor approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

mimir-tool: analyse prometheus: ux: refactor / resiliency / concurrency
7 participants
@anilmisirlioglu @CLAassistant @osg-grafana @Dentrax @Logiraptor @pracucci @dimitarvdimitrov