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

Rule: Support ruleGroup limit #4868

Merged
merged 6 commits into from Nov 19, 2021
Merged

Rule: Support ruleGroup limit #4868

merged 6 commits into from Nov 19, 2021

Conversation

hanjm
Copy link
Member

@hanjm hanjm commented Nov 14, 2021 •
edited

#4837.
Signed-off-by: Jimmie Han hanjinming@outlook.com

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

  1. Upgrade promethes dependencies version.
  2. Add limit field to pb.
  3. Add limit field to pb.
  4. Add unit test about prase rule file and eval rule.

Verification

Unit test.

@hanjm hanjm force-pushed the feature/jimmiehan-ruler-support-limit branch 2 times, most recently from ba0d555 to 89b70aa Compare November 14, 2021 09:11
yeya24
yeya24 reviewed Nov 14, 2021
View reviewed changes
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.

IIUC this pr only adds limit to the protobuf. Would you like to also implement the actual limit logic in the API in this pr?

@hanjm
Copy link
Member Author

hanjm commented Nov 15, 2021

IIUC this pr only adds limit to the protobuf. Would you like to also implement the actual limit logic in the API in this pr?

@yeya24 I foud the actual limit logic done by prometheus. look at the unit test. https://github.com/thanos-io/thanos/pull/4868/files#diff-f197af2d6f28659de1d4f1166ddc965356d93971449ea2b6ad71f142c65f9768R403. it return a limit error when eval.

yeya24
yeya24 previously approved these changes Nov 15, 2021
View reviewed changes
Copy link
Contributor

@yeya24 yeya24 left a comment •
edited

Choose a reason for hiding this comment

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

I see. I thought this limit is done at the API side at the beginning. Then can you also add this to our E2E test to see if it works as expected?

GiedriusS
GiedriusS reviewed Nov 15, 2021
View reviewed changes
Copy link
Member

@GiedriusS GiedriusS left a comment

Choose a reason for hiding this comment

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

👍 e2e test would be great to ensure that this limit is passed down properly

@hanjm
Copy link
Member Author

hanjm commented Nov 15, 2021

OK. let me add some e2e test about rules api.

@hanjm
Copy link
Member Author

hanjm commented Nov 16, 2021

Seems some thing break the ruler e2e test parse yaml config in flag. i need a bit more time to debug.

@hanjm
Rule: Support ruleGroup limit (thanos-io#4837)
2738f1c 
Signed-off-by: Jimmie Han <hanjinming@outlook.com>
@hanjm
Rules: format rpc.proto
c0a3531 
Signed-off-by: Jimmie Han <hanjinming@outlook.com>
@hanjm
Rules: fix unit test
a42d4c4 
Signed-off-by: Jimmie Han <hanjinming@outlook.com>
@hanjm hanjm force-pushed the feature/jimmiehan-ruler-support-limit branch 2 times, most recently from 04f9db7 to bf86d8a Compare November 17, 2021 12:34
@hanjm
Rules: add e2e test
a79983d 
Signed-off-by: Jimmie Han <hanjinming@outlook.com>
@hanjm hanjm force-pushed the feature/jimmiehan-ruler-support-limit branch from bf86d8a to a79983d Compare November 17, 2021 12:42
@hanjm
Copy link
Member Author

hanjm commented Nov 17, 2021

I shoud create a execeed limit rule and assert the rule health field. let me implement it.

@hanjm
Rules: add e2e rule test case hit limit
7b1ee8d 
Signed-off-by: Jimmie Han <hanjinming@outlook.com>
@hanjm
Copy link
Member Author

hanjm commented Nov 18, 2021

I shoud create a execeed limit rule and assert the rule health field. let me implement it.

Done

@hanjm hanjm force-pushed the feature/jimmiehan-ruler-support-limit branch from d2b498c to 1bc041a Compare November 18, 2021 09:14
@hanjm
Copy link
Member Author

hanjm commented Nov 18, 2021

The e2e test error when do thanos-receive test. I am confusing . @GiedriusS Could you help me check it , Thank you.

GiedriusS
GiedriusS previously approved these changes Nov 18, 2021
View reviewed changes
Copy link
Member

@GiedriusS GiedriusS left a comment

Choose a reason for hiding this comment

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

Two nits but overall LGTM 💪

pkg/rules/manager_test.go Outdated Show resolved Hide resolved
pkg/rules/manager_test.go Outdated Show resolved Hide resolved
@hanjm
fix unit test ci, use runutil.Retry instead of time.Sleep.
2de2981 
Signed-off-by: Jimmie Han <hanjinming@outlook.com>
@hanjm hanjm force-pushed the feature/jimmiehan-ruler-support-limit branch from 1bc041a to 2de2981 Compare November 19, 2021 02:22
yeya24
yeya24 approved these changes Nov 19, 2021
View reviewed changes
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.

LGTM

@yeya24 yeya24 merged commit 243526d into thanos-io:main Nov 19, 2021
@yeya24 yeya24 mentioned this pull request Nov 19, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yeya24 yeya24 yeya24 approved these changes

@GiedriusS GiedriusS GiedriusS left review comments

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

Successfully merging this pull request may close these issues.

None yet
3 participants
@hanjm @GiedriusS @yeya24