Improve Github alerter for Mako (#748)

* update mako alerter

* revert a small change

* address comments

* revert all %w to %v

test/mako/alerter/alerter.go

@@ -17,6 +17,7 @@ limitations under the License.
 package alerter
 
 import (
+	"fmt"
 	"log"
 
 	qpb "github.com/google/mako/proto/quickstore/quickstore_go_proto"
@@ -55,7 +56,7 @@ func (alerter *Alerter) HandleBenchmarkResult(testName string, output qpb.Quicks
 	if err != nil {
 		if output.GetStatus() == qpb.QuickstoreOutput_ANALYSIS_FAIL {
 			var errs []error
-			summary := output.GetSummaryOutput()
+			summary := fmt.Sprintf("%s\n\nSee run chart at: %s", output.GetSummaryOutput(), output.GetRunChartLink())
 			if alerter.githubIssueHandler != nil {
 				if err := alerter.githubIssueHandler.CreateIssueForTest(testName, summary); err != nil {
 					errs = append(errs, err)

test/mako/alerter/github/issue.go

@@ -38,10 +38,11 @@ const (
 	// issueBodyTemplate is a template for issue body
 	issueBodyTemplate = `
 ### Auto-generated issue tracking performance regression
-* **Test name**: %s`
+* **Test name**: %s
+* **Repository name**: %s`
 
-	// newIssueCommentTemplate is a template for the comment of an issue that has been quiet for a long time
-	newIssueCommentTemplate = `
+	// issueSummaryCommentTemplate is a template for the summary of an issue
+	issueSummaryCommentTemplate = `
 A new regression for this test has been detected:
 %s`
 
@@ -50,9 +51,9 @@ A new regression for this test has been detected:
 New regression has been detected, reopening this issue:
 %s`
 
-	// closeIssueComment is the comment of an issue when we close it
+	// closeIssueComment is the comment of an issue when it is closed
 	closeIssueComment = `
-The performance regression goes way for this test, closing this issue.`
+The performance regression goes away for this test, closing this issue.`
 )
 
 // IssueHandler handles methods for github issues
@@ -87,65 +88,75 @@ func Setup(org, repo, githubTokenPath string, dryrun bool) (*IssueHandler, error
 // CreateIssueForTest will try to add an issue with the given testName and description.
 // If there is already an issue related to the test, it will try to update that issue.
 func (gih *IssueHandler) CreateIssueForTest(testName, desc string) error {
-	org := gih.config.org
-	repo := gih.config.repo
-	dryrun := gih.config.dryrun
 	title := fmt.Sprintf(issueTitleTemplate, testName)
-	issue := gih.findIssue(org, repo, title, dryrun)
+	issue, err := gih.findIssue(title)
+	if err != nil {
+		return fmt.Errorf("failed to find issues for test %q: %v, skipped creating new issue", testName, err)
+	}
 	// If the issue hasn't been created, create one
 	if issue == nil {
-		body := fmt.Sprintf(issueBodyTemplate, testName)
-		issue, err := gih.createNewIssue(org, repo, title, body, dryrun)
+		commentBody := fmt.Sprintf(issueBodyTemplate, testName, gih.config.repo)
+		issue, err := gih.createNewIssue(title, commentBody)
 		if err != nil {
-			return err
+			return fmt.Errorf("failed to create a new issue for test %q: %v", testName, err)
 		}
-		comment := fmt.Sprintf(newIssueCommentTemplate, desc)
-		if err := gih.addComment(org, repo, *issue.Number, comment, dryrun); err != nil {
-			return err
+		commentBody = fmt.Sprintf(issueSummaryCommentTemplate, desc)
+		if err := gih.addComment(*issue.Number, commentBody); err != nil {
+			return fmt.Errorf("failed to add comment for new issue %d: %v", *issue.Number, err)
 		}
-		// If one issue with the same title has been closed, reopen it and add new comment
-	} else if *issue.State == string(ghutil.IssueCloseState) {
-		if err := gih.reopenIssue(org, repo, *issue.Number, dryrun); err != nil {
-			return err
+		return nil
 	}
-		comment := fmt.Sprintf(reopenIssueCommentTemplate, desc)
-		if err := gih.addComment(org, repo, *issue.Number, comment, dryrun); err != nil {
-			return err
+
+	// If the issue has been created, edit it
+	issueNumber := *issue.Number
+
+	// If the issue has been closed, reopen it
+	if *issue.State == string(ghutil.IssueCloseState) {
+		if err := gih.reopenIssue(issueNumber); err != nil {
+			return fmt.Errorf("failed to reopen issue %d: %v", issueNumber, err)
 		}
-	} else {
-		// If the issue hasn't been updated for a long time, add a new comment
-		if time.Now().Sub(*issue.UpdatedAt) > daysConsideredOld*24*time.Hour {
-			comment := fmt.Sprintf(newIssueCommentTemplate, desc)
-			// TODO(Fredy-Z): edit the old comment instead of adding a new one, like flaky-test-reporter
-			if err := gih.addComment(org, repo, *issue.Number, comment, dryrun); err != nil {
-				return err
+		commentBody := fmt.Sprintf(reopenIssueCommentTemplate, desc)
+		if err := gih.addComment(issueNumber, commentBody); err != nil {
+			return fmt.Errorf("failed to add comment for reopened issue %d: %v", issueNumber, err)
 		}
 	}
+
+	// Edit the old comment
+	comments, err := gih.getComments(issueNumber)
+	if err != nil {
+		return fmt.Errorf("failed to get comments from issue %d: %v", issueNumber, err)
+	}
+	if len(comments) < 2 {
+		return fmt.Errorf("existing issue %d is malformed, cannot update", issueNumber)
+	}
+	commentBody := fmt.Sprintf(issueSummaryCommentTemplate, desc)
+	if err := gih.editComment(issueNumber, *comments[1].ID, commentBody); err != nil {
+		return fmt.Errorf("failed to edit the comment for issue %d: %v", issueNumber, err)
 	}
 
 	return nil
 }
 
 // createNewIssue will create a new issue, and add perfLabel for it.
-func (gih *IssueHandler) createNewIssue(org, repo, title, body string, dryrun bool) (*github.Issue, error) {
+func (gih *IssueHandler) createNewIssue(title, body string) (*github.Issue, error) {
 	var newIssue *github.Issue
 	if err := helpers.Run(
-		"creating issue",
+		fmt.Sprintf("creating issue %q in %q", title, gih.config.repo),
 		func() error {
 			var err error
-			newIssue, err = gih.client.CreateIssue(org, repo, title, body)
+			newIssue, err = gih.client.CreateIssue(gih.config.org, gih.config.repo, title, body)
 			return err
 		},
-		dryrun,
+		gih.config.dryrun,
 	); nil != err {
 		return nil, err
 	}
 	if err := helpers.Run(
-		"adding perf label",
+		fmt.Sprintf("adding perf label for issue %q in %q", title, gih.config.repo),
 		func() error {
-			return gih.client.AddLabelsToIssue(org, repo, *newIssue.Number, []string{perfLabel})
+			return gih.client.AddLabelsToIssue(gih.config.org, gih.config.repo, *newIssue.Number, []string{perfLabel})
 		},
-		dryrun,
+		gih.config.dryrun,
 	); nil != err {
 		return nil, err
 	}
@@ -155,74 +166,112 @@ func (gih *IssueHandler) createNewIssue(org, repo, title, body string, dryrun bo
 // CloseIssueForTest will try to close the issue for the given testName.
 // If there is no issue related to the test or the issue is already closed, the function will do nothing.
 func (gih *IssueHandler) CloseIssueForTest(testName string) error {
-	org := gih.config.org
-	repo := gih.config.repo
-	dryrun := gih.config.dryrun
 	title := fmt.Sprintf(issueTitleTemplate, testName)
-	issue := gih.findIssue(org, repo, title, dryrun)
-	if issue == nil || *issue.State == string(ghutil.IssueCloseState) {
+	issue, err := gih.findIssue(title)
+	// If no issue has been found, or the issue has already been closed, do nothing.
+	if issue == nil || err != nil || *issue.State == string(ghutil.IssueCloseState) {
 		return nil
 	}
 
 	issueNumber := *issue.Number
-	if err := helpers.Run(
-		"add comment for the issue to close",
-		func() error {
-			_, cErr := gih.client.CreateComment(org, repo, issueNumber, closeIssueComment)
-			return cErr
-		},
-		dryrun,
-	); err != nil {
-		return err
-	}
-	return helpers.Run(
-		"closing issue",
-		func() error {
-			return gih.client.CloseIssue(org, repo, issueNumber)
-		},
-		dryrun,
-	)
-}
-
-// reopenIssue will reopen the given issue.
-func (gih *IssueHandler) reopenIssue(org, repo string, issueNumber int, dryrun bool) error {
-	return helpers.Run(
-		"reopen the issue",
-		func() error {
-			return gih.client.ReopenIssue(org, repo, issueNumber)
-		},
-		dryrun,
-	)
-}
-
-// findIssue will return the issue in the given repo if it exists.
-func (gih *IssueHandler) findIssue(org, repo, title string, dryrun bool) *github.Issue {
-	var issues []*github.Issue
-	helpers.Run(
-		"list issues in the repo",
-		func() error {
-			var err error
-			issues, err = gih.client.ListIssuesByRepo(org, repo, []string{perfLabel})
-			return err
-		},
-		dryrun,
-	)
-	for _, issue := range issues {
-		if *issue.Title == title {
-			return issue
+	if err := gih.addComment(issueNumber, closeIssueComment); err != nil {
+		return fmt.Errorf("failed to add comment for the issue %d to close: %v", issueNumber, err)
 	}
+	if err := gih.closeIssue(issueNumber); err != nil {
+		return fmt.Errorf("failed to close the issue %d: %v", issueNumber, err)
 	}
 	return nil
 }
 
-// addComment will add comment for the given issue.
-func (gih *IssueHandler) addComment(org, repo string, issueNumber int, commentBody string, dryrun bool) error {
+// reopenIssue will reopen the given issue.
+func (gih *IssueHandler) reopenIssue(issueNumber int) error {
 	return helpers.Run(
-		"add comment for issue",
+		fmt.Sprintf("reopening issue %d in %q", issueNumber, gih.config.repo),
 		func() error {
-			_, err := gih.client.CreateComment(org, repo, issueNumber, commentBody)
-			return err
+			return gih.client.ReopenIssue(gih.config.org, gih.config.repo, issueNumber)
 		},
-		dryrun,
+		gih.config.dryrun,
+	)
+}
+
+// closeIssue will close the given issue.
+func (gih *IssueHandler) closeIssue(issueNumber int) error {
+	return helpers.Run(
+		fmt.Sprintf("closing issue %d in %q", issueNumber, gih.config.repo),
+		func() error {
+			return gih.client.CloseIssue(gih.config.org, gih.config.repo, issueNumber)
+		},
+		gih.config.dryrun,
+	)
+}
+
+// findIssue will return the issue in the given repo if it exists.
+func (gih *IssueHandler) findIssue(title string) (*github.Issue, error) {
+	var issues []*github.Issue
+	if err := helpers.Run(
+		fmt.Sprintf("listing issues in %q", gih.config.repo),
+		func() error {
+			var err error
+			issues, err = gih.client.ListIssuesByRepo(gih.config.org, gih.config.repo, []string{perfLabel})
+			return err
+		},
+		gih.config.dryrun,
+	); err != nil {
+		return nil, err
+	}
+	var existingIssue *github.Issue
+	for _, issue := range issues {
+		if *issue.Title == title {
+			// If the issue has been closed a long time ago, ignore this issue.
+			if issue.GetState() == string(ghutil.IssueCloseState) &&
+				time.Now().Sub(*issue.UpdatedAt) > daysConsideredOld*24*time.Hour {
+				continue
+			}
+
+			existingIssue = issue
+			break
+		}
+	}
+
+	return existingIssue, nil
+}
+
+// getComments will get comments for the given issue.
+func (gih *IssueHandler) getComments(issueNumber int) ([]*github.IssueComment, error) {
+	var comments []*github.IssueComment
+	if err := helpers.Run(
+		fmt.Sprintf("getting comments for issue %d in %q", issueNumber, gih.config.repo),
+		func() error {
+			var err error
+			comments, err = gih.client.ListComments(gih.config.org, gih.config.repo, issueNumber)
+			return err
+		},
+		gih.config.dryrun,
+	); err != nil {
+		return comments, err
+	}
+	return comments, nil
+}
+
+// addComment will add comment for the given issue.
+func (gih *IssueHandler) addComment(issueNumber int, commentBody string) error {
+	return helpers.Run(
+		fmt.Sprintf("adding comment %q for issue %d in %q", commentBody, issueNumber, gih.config.repo),
+		func() error {
+			_, err := gih.client.CreateComment(gih.config.org, gih.config.repo, issueNumber, commentBody)
+			return err
+		},
+		gih.config.dryrun,
+	)
+}
+
+// editComment will edit the comment to the new body.
+func (gih *IssueHandler) editComment(issueNumber int, commentID int64, commentBody string) error {
+	return helpers.Run(
+		fmt.Sprintf("editting comment to %q for issue %d in %q", commentBody, issueNumber, gih.config.repo),
+		func() error {
+			return gih.client.EditComment(gih.config.org, gih.config.repo, commentID, commentBody)
+		},
+		gih.config.dryrun,
 	)
 }

test/mako/alerter/github/issue_test.go

@@ -42,8 +42,8 @@ func TestNewIssueWillBeAdded(t *testing.T) {
 		t.Fatalf("expected to create a new issue %v, but failed", testName)
 	}
 	issueTitle := fmt.Sprintf(issueTitleTemplate, testName)
-	issueFound := gih.findIssue(gih.config.org, gih.config.repo, issueTitle, false)
-	if issueFound == nil {
+	issueFound, err := gih.findIssue(issueTitle)
+	if issueFound == nil || err != nil {
 		t.Fatalf("expected to find the new created issue %v, but failed to", testName)
 	}
 }
@@ -60,18 +60,15 @@ func TestClosedIssueWillBeReopened(t *testing.T) {
 	if err := gih.CreateIssueForTest(testName, testDesc); err != nil {
 		t.Fatalf("expected to update the existed issue %v, but failed", testName)
 	}
-	updatedIssue := gih.findIssue(org, repo, issueTitle, gih.config.dryrun)
-	if updatedIssue == nil || *updatedIssue.State != string(ghutil.IssueOpenState) {
+	updatedIssue, err := gih.findIssue(issueTitle)
+	if updatedIssue == nil || err != nil || *updatedIssue.State != string(ghutil.IssueOpenState) {
 		t.Fatalf("expected to reopen the closed issue %v, but failed", testName)
 	}
 }
 
 func TestIssueCanBeClosed(t *testing.T) {
-	org := gih.config.org
-	repo := gih.config.repo
 	testName := "test closing existed issue"
 	testDesc := "test closing existed issue desc"
-	issueTitle := fmt.Sprintf(issueTitleTemplate, testName)
 	if err := gih.CreateIssueForTest(testName, testDesc); err != nil {
 		t.Fatalf("expected to create a new issue %v, but failed", testName)
 	}
@@ -79,8 +76,4 @@ func TestIssueCanBeClosed(t *testing.T) {
 	if err := gih.CloseIssueForTest(testName); err != nil {
 		t.Fatalf("tried to close the existed issue %v, but got an error %v", testName, err)
 	}
-	updatedIssue := gih.findIssue(org, repo, issueTitle, gih.config.dryrun)
-	if updatedIssue == nil || *updatedIssue.State != string(ghutil.IssueCloseState) {
-		t.Fatalf("expected to close the issue %v, but failed", testName)
-	}
 }