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

add ability to update in comments #161

Closed

Conversation

twotired
Copy link
Contributor

@twotired twotired commented May 8, 2023

As an alternative, or as an addition to updated the Description, additional Alert updates should go to comments.

The default behavior is to update the JIRA Description when the Alert details change. This makes it difficult if the viewers of the JIRA are researching each failure because it is not obvious which alerts are new or existing. Depending on the Alert expression, old firing details may also be removed.

Additionally, adding too many firing details to the description increases the likelihood of exceeding the 32KB char limit for the JIRA Description.

By optionally sending each update to a Comment, viewers can better keep track of which activity is new, and it will be possible to continue adding firing details without exceeding the Description char limit.

resolves #160

@twotired twotired changed the title add ability to update in comments (#160) add ability to update in comments May 8, 2023
@twotired
Copy link
Contributor Author

twotired commented Jun 4, 2023

@bwplotka Could you help review? If there are any deal-breakers, I'd like to get them fixed ASAP.
TIA

@MytkoEnko
Copy link

@twotired Wouldn't it be better to add these features in config instead of global flag? That way each of the features might be managed per project saving the configurations flexibility, otherwise for two projects with flag on/off we'd need two instances of jiralert.

@cropalato
Copy link

Hey @twotired,
I need to agree with @MytkoEnko. Could you consider my PR for your branch?
The solution was yours, as I said I agree with the comment, but I want to see that feature included, so I sent you my suggestion to put that in place.
Please let me know if there is something else I can help with that PR

@bwplotka
Copy link
Member

Tests are still failing (also DCO) - do you mind to fix that?

cropalato and others added 4 commits July 25, 2023 15:38
- Changed comment;
- Replaced space indentation by tab indentation.

Signed-off-by: Jason Wells <[email protected]>
…KB) (prometheus-community#165)

* truncate descriptions that exceed -max-description-length (default 32,768)

Signed-off-by: Jason Wells <[email protected]>

* Update main.go

size was off by 1

Signed-off-by: Jason Wells <[email protected]>

---------

Signed-off-by: Jason Wells <[email protected]>
@twotired
Copy link
Contributor Author

Tests are still failing (also DCO) - do you mind to fix that?

@bwplotka tests and DCO fixed, please review.

@holger-waschke
Copy link
Contributor

holger-waschke commented Oct 17, 2023

We want to use this too so I tested it. There were some tweaks necessary for us to work.

I think its better to only add a comment in a issue if the description contains something new. Otherwise a new comment will be created as if the repeat_interval from Alertmanager is configured.

		if updateDescription {
			if issue.Fields.Description != issueDesc {
				if r.conf.UpdateInComment {
					retry, err := r.addComment(issue.Key, issueDesc)
					if err != nil {
						return retry, err
					}
				}
				retry, err := r.updateDescription(issue.Key, issueDesc)
				if err != nil {
					return retry, err
				}
			}
		}

For this to work I had to add description to the options field. Now only a comment is added if theres a actual update and not just a polling

func (r *Receiver) search(projects []string, issueLabel string) (*jira.Issue, bool, error) {
	// search multiple projects in case issue was moved and further alert firings are desired in existing JIRA
	projectList := "'" + strings.Join(projects, "', '") + "'"
	query := fmt.Sprintf("project in(%s) and labels=%q order by resolutiondate desc", projectList, issueLabel)
	options := &jira.SearchOptions{
		Fields:     append([]string{"summary", "status", "resolution", "resolutiondate", "description"},
		MaxResults: 2,
	}

I will push a commit this week if I find time

@twotired
Copy link
Contributor Author

@holger-waschke I was planning to close this PR and open a new one after I realized some cleanup is needed.

To work around duplicate comments, I set a repeat_interval in the Alertmanager config that is longer than our time between releases:

            group_wait: 10m     # wait 10m to group related alerts before creating/updating JIRA
            repeat_interval: 7d # do not re-send alert for which there's already a JIRA
            group_interval: 1h  # wait 1h before adding comment for additional failures

This works for us because we replace all containers (and reset counters) every 7d.

Also, we wouldn't be able to compare to the Description because we disable updating the Description, because sometimes users want to make changes to the Description and those would otherwise be lost.

@holger-waschke
Copy link
Contributor

holger-waschke commented Oct 18, 2023

@holger-waschke I was planning to close this PR and open a new one after I realized some cleanup is needed.

To work around duplicate comments, I set a repeat_interval in the Alertmanager config that is longer than our time between releases:

            group_wait: 10m     # wait 10m to group related alerts before creating/updating JIRA
            repeat_interval: 7d # do not re-send alert for which there's already a JIRA
            group_interval: 1h  # wait 1h before adding comment for additional failures

This works for us because we replace all containers (and reset counters) every 7d.

Also, we wouldn't be able to compare to the Description because we disable updating the Description, because sometimes users want to make changes to the Description and those would otherwise be lost.

I see then it has to be outside of the
if updateDescription {} block, the only important thing is to compare the current description with the issue description
if issue.Fields.Description != issueDesc {} to avoid comment flodding.

Most users probably have a much lower repeat interval. Before jiralert wasnt comparing the description at all and always updated the description if updateDescription was set to true only jira itself prevents updating the description is the content is identical so its cleaner now anyway to add the description to the fields

Fields: append([]string{"summary", "status", "resolution", "resolutiondate", "description"},

@@ -107,6 +113,13 @@ func (r *Receiver) Notify(data *alertmanager.Data, hashJiraLabel bool, updateSum
}
}

if r.conf.UpdateInComment {
Copy link
Contributor

Choose a reason for hiding this comment

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

this works better for me to avoid flodding of comments if repeat interval is set low

		if r.conf.UpdateInComment {
			if issue.Fields.Description != issueDesc {
				retry, err := r.addComment(issue.Key, issueDesc)
				if err != nil {
					return retry, err
				}
			}
		}

Copy link
Contributor

Choose a reason for hiding this comment

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

And add descriptionhere

func (r *Receiver) search(project, issueLabel string) (*jira.Issue, bool, error) {
	query := fmt.Sprintf("project=\"%s\" and labels=%q order by resolutiondate desc", project, issueLabel)
	options := &jira.SearchOptions{
		Fields:     []string{"summary", "status", "resolution", "resolutiondate", "description"},
		MaxResults: 2,
	}

@bwplotka
Copy link
Member

Thanks for helping @holger-waschke 👍🏽

Happy to review the final result of your collaboration (:

@twotired
Copy link
Contributor Author

closing this now that I have a new PR with more tests and better config options: #180

@twotired twotired closed this Nov 28, 2023
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.

Send additional Alert updates to Comments
5 participants