-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Adding the alert group info API #3364
base: main
Are you sure you want to change the base?
Conversation
31caabd
to
cf65ed4
Compare
e8d049c
to
ff168b1
Compare
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.
lgtm
rf := func(receiverFilter *regexp.Regexp) func(r *dispatch.Route) bool { | ||
return func(r *dispatch.Route) bool { | ||
receiver := r.RouteOpts.Receiver | ||
if receiverFilter != nil && !receiverFilter.MatchString(receiver) { |
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.
Question, do we need to use regex here? That means the input params.Receiver
contains multiple receivers?
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.
This is more about support regex search for receivers, i am following convention of receiver filter parameter in the other apis
See https://github.com/prometheus/alertmanager/blob/main/api/v2/api.go#L250-L260
|
||
func validateNextToken(nextToken *string) error { | ||
if nextToken != nil && *nextToken != "" { | ||
match, _ := regexp.MatchString("^[a-fA-F0-9]{40}$", *nextToken) |
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.
Let's compile the regex once and match 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.
removed the nextToken != ""
api/v2/api.go
Outdated
for _, alertGroup := range ags { | ||
|
||
// Skip the aggregation group if the next token is set and hasn't arrived the nextToken item yet. | ||
if params.NextToken != nil && *params.NextToken != "" && *params.NextToken >= alertGroup.ID { |
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.
Can you simplify to if len(params.NextToken) > 0 && *params.NextToken >= alertGroup.ID
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.
removed *params.NextToken != "" check as it won't be empty string from validateNextToken. I won't remove the params.NextToken != nil as won't len(params.NextToken) result in NPE when params.NextToken is nil?
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.
Nvm. I thought params.NextToken
is a string type from the hints when doing code review on GH. If it is *string
I am good.
return nil | ||
} | ||
|
||
func validateNextToken(nextToken *string) 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.
I think we can use string
here as param?
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.
I want to escape the validation when nextToken is nil, kind of want to keep that if into this function rather in main function
@@ -414,6 +459,12 @@ func (ag *aggrGroup) fingerprint() model.Fingerprint { | |||
return ag.labels.Fingerprint() | |||
} | |||
|
|||
func (ag *aggrGroup) GroupID() string { | |||
h := sha1.New() |
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.
Any reason to use sha1? Will xxhash or other libraries works
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.
only reason of using sha1 is wanting to keep the output of the hash to be short as this is going to be nexttoken for user to use
dispatch/route.go
Outdated
func (r *Route) ID() string { | ||
b := strings.Builder{} | ||
|
||
var position *int |
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.
I prefer to not use pointer... We can use int and initialize it to an invalid value like -1
then we will have less pointer reference operations
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.
fixed
80fad23
to
e16034e
Compare
* Add tests to TestMatchers This commit adds a number of tests to TestMatchers that asserts some of the more nuanced behavior when parsing label matchers. Signed-off-by: George Robinson <[email protected]> --------- Signed-off-by: George Robinson <[email protected]> Signed-off-by: Yijie Qin <[email protected]>
Signed-off-by: Yijie Qin <[email protected]>
Signed-off-by: Yijie Qin <[email protected]>
Signed-off-by: Yijie Qin <[email protected]>
Signed-off-by: Yijie Qin <[email protected]>
Signed-off-by: Yijie Qin <[email protected]>
Signed-off-by: Yijie Qin <[email protected]>
Signed-off-by: Yijie Qin <[email protected]>
Signed-off-by: Yijie Qin <[email protected]>
Signed-off-by: Yijie Qin <[email protected]>
Signed-off-by: Yijie Qin <[email protected]>
Signed-off-by: Yijie Qin <[email protected]>
Signed-off-by: Yijie Qin <[email protected]>
Implement the proposal here #3361