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

Refactor Rules Protos #2226

Merged

Conversation

jtlisi
Copy link
Contributor

@jtlisi jtlisi commented Mar 6, 2020

What this PR does:

The ruler/rules/rules.proto file defines a proto for storing Prometheus rule groups. In order to make this proto message more amenable to the storage of rules, information that communicates the state of a running rule was removed and placed into a separate GroupStateDesc message in the ruler/ruler.proto file.

This PR executes the above change and updates all the consumers that utilize this change.

When rolling out this change, the experimental ruler API will be unable to serve the status of running rules until the rollout is complete. This will only affect the /api/v1/rules and /api/v1/alerts endpoint. It will not affect the evaluation of running rule groups.

Checklist

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

Signed-off-by: Jacob Lisi [email protected]

import "github.com/cortexproject/cortex/pkg/ingester/client/cortex.proto";

option (gogoproto.marshaler_all) = true;
option (gogoproto.unmarshaler_all) = true;

// RuleGroupDesc is a proto representation of a cortex rule group
message RuleGroupDesc {
reserved 5;
Copy link
Contributor

Choose a reason for hiding this comment

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

5, 7, 8?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

}

// RuleDesc is a proto representation of a Prometheus Rule
message RuleDesc {
reserved 7 to 10;
Copy link
Contributor

Choose a reason for hiding this comment

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

to 12?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

}
groupDescs = append(groupDescs, groupDesc)
}
return groupDescs, nil
}

func (r *Ruler) getShardedRules(ctx context.Context, userID string) ([]*rules.RuleGroupDesc, error) {
func (r *Ruler) getShardedRules(ctx context.Context, userID string) ([]*GroupStateDesc, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

userID parameter is unused here

Copy link
Contributor Author

@jtlisi jtlisi Mar 10, 2020

Choose a reason for hiding this comment

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

Makes sense, it should be safe to remove. An early version of this function used it but it was removed at some point. The history of that was squashed when the PR for this was merged. I've removed the parameter and added some code to ensure the context has a user set.

Copy link
Contributor

@pstibrany pstibrany left a comment

Choose a reason for hiding this comment

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

There is one place where userID parameter to the function is unused, which may be a bug. If it's intentional, perhaps just remove it completely. Other than that, looks good to me overall, pretty straightforward change.

jtlisi added 4 commits March 10, 2020 12:07
Signed-off-by: Jacob Lisi <[email protected]>
Signed-off-by: Jacob Lisi <[email protected]>
Signed-off-by: Jacob Lisi <[email protected]>
@jtlisi jtlisi force-pushed the 20200306_separate_rulestate_proto branch from fffb87d to 6177192 Compare March 10, 2020 16:08
@jtlisi jtlisi merged commit 829109c into cortexproject:master Mar 10, 2020
@jtlisi jtlisi deleted the 20200306_separate_rulestate_proto branch March 10, 2020 16:21
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.

2 participants