-
Notifications
You must be signed in to change notification settings - Fork 812
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
Refactor Rules Protos #2226
Conversation
pkg/ruler/rules/rules.proto
Outdated
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
5, 7, 8?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
pkg/ruler/rules/rules.proto
Outdated
} | ||
|
||
// RuleDesc is a proto representation of a Prometheus Rule | ||
message RuleDesc { | ||
reserved 7 to 10; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to 12?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
pkg/ruler/ruler.go
Outdated
} | ||
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) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this 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.
Signed-off-by: Jacob Lisi <[email protected]>
Signed-off-by: Jacob Lisi <[email protected]>
Signed-off-by: Jacob Lisi <[email protected]>
Signed-off-by: Jacob Lisi <[email protected]>
fffb87d
to
6177192
Compare
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 separateGroupStateDesc
message in theruler/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
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]
Signed-off-by: Jacob Lisi [email protected]