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

Add user label to ruler query and write metrics #5312

Merged
merged 1 commit into from
May 4, 2023

Conversation

rajagopalanand
Copy link
Contributor

@rajagopalanand rajagopalanand commented May 1, 2023

What this PR does:

Adding user label to cortex_ruler_queries_failed_total, cortex_ruler_queries_total, cortex_ruler_write_requests_failed_total, cortex_ruler_write_requests_total metrics. This would allow for better tracking of failures at a user level

Checklist

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

Copy link
Contributor

@alvinlin123 alvinlin123 left a comment

Choose a reason for hiding this comment

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

overall looks good, but I think some test should be updated/added for the user label.

@alanprot
Copy link
Member

alanprot commented May 1, 2023

Humm.. is this a breaking change?

@alanprot @yeya24 WDYT?

@alvinlin123 alvinlin123 requested review from friedrichg and yeya24 May 1, 2023 20:39
@yeya24
Copy link
Contributor

yeya24 commented May 1, 2023

The change looks okay to me. Please make sure we mention the metrics we are going to change in the changelog.
And those metrics are counters so I feel the cardinality change for these metrics is okay.

Copy link
Member

@friedrichg friedrichg left a comment

Choose a reason for hiding this comment

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

Looks good, like Alvin says: just add 1 test using testutil.GatherAndCompare

Copy link
Contributor

@alvinlin123 alvinlin123 left a comment

Choose a reason for hiding this comment

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

Nice enhancement, but I think something is wrong with the PR; it includes changes that are already in the cortexporject:master branch. Can you fix this before I merge this PR?

@@ -157,6 +157,7 @@ jobs:
docker pull quay.io/cortexproject/cortex:v1.14.0
docker pull quay.io/cortexproject/cortex:v1.14.1
docker pull quay.io/cortexproject/cortex:v1.15.0
docker pull quay.io/cortexproject/cortex:v1.15.1
Copy link
Contributor

Choose a reason for hiding this comment

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

this looks like a change that is already in cortexproject:master

.gitignore Outdated
cmd/cortex/cortex
cmd/query-tee/query-tee
cmd/thanosconvert/thanosconvert
cmd/test-exporter/test-exporter-*
Copy link
Contributor

Choose a reason for hiding this comment

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

this looks like a change that is already in cortexproject:master

@rajagopalanand rajagopalanand force-pushed the master branch 3 times, most recently from 85b384a to f443580 Compare May 4, 2023 18:40
…ser level

Signed-off-by: Anand Rajagopal <anrajag@amazon.com>
Copy link
Contributor

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

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

Thanks

@yeya24 yeya24 merged commit 36d88c0 into cortexproject:master May 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants