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

Adding the alert group info API #3364

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

qinxx108
Copy link
Contributor

Implement the proposal here #3361

@qinxx108 qinxx108 changed the title Adding the alert group info API spec Adding the alert group info API May 16, 2023
@qinxx108 qinxx108 force-pushed the alert-group-api branch 6 times, most recently from 31caabd to cf65ed4 Compare May 23, 2023 15:48
@qinxx108 qinxx108 force-pushed the alert-group-api branch 2 times, most recently from e8d049c to ff168b1 Compare June 2, 2023 00:39
Copy link
Contributor

@emanlodovice emanlodovice left a 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) {
Copy link

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?

Copy link
Contributor Author

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)
Copy link

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?

Copy link
Contributor Author

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 {
Copy link

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

Copy link
Contributor Author

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?

Copy link

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 {
Copy link

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?

Copy link
Contributor Author

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()
Copy link

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

Copy link
Contributor Author

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

func (r *Route) ID() string {
b := strings.Builder{}

var position *int
Copy link

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@gotjosh
Copy link
Member

gotjosh commented Jun 8, 2023

The overall direction looks good, great job @qinxx108 ! I have comments but let's finish with #3372 first before we continue here.

@qinxx108 qinxx108 force-pushed the alert-group-api branch 2 times, most recently from 80fad23 to e16034e Compare July 6, 2023 21:20
grobinson-grafana and others added 13 commits July 7, 2023 02:50

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
* 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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants