-
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
Add msteams #3324
Add msteams #3324
Conversation
1d5b341
to
1136511
Compare
docs/configuration.md
Outdated
|
||
```yaml | ||
# Whether to notify about resolved alerts. | ||
[ send_resolved: <boolean> | default = true ] |
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.
The default is false
[ send_resolved: <boolean> | default = true ] | |
[ send_resolved: <boolean> | default = false ] |
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.
Thanks for your review. I think we need to keep the default value to true
.
I have updated the default value in code.
Hey @zhan9san, |
1136511
to
eb1f936
Compare
Could you help review this PR? |
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!
Thank you so much the contribution 🙏 our users will massively appreciate that. I have left a few nits and some comments please let me know what you think.
cmd/alertmanager/main.go
Outdated
@@ -56,6 +56,7 @@ import ( | |||
"github.com/prometheus/alertmanager/notify/pushover" | |||
"github.com/prometheus/alertmanager/notify/slack" | |||
"github.com/prometheus/alertmanager/notify/sns" | |||
"github.com/prometheus/alertmanager/notify/teams" |
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.
nit: Teams is a very common "name" for these type of integrations e.g. Webex also calls their platform Webex Teams.
How do we feel about renaming the package and structs from "teams" to "msteams", I think this is clearer and leaves no room for interpretation.
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.
agreed
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.
The package is renamed to msteams in new commit
docs/configuration.md
Outdated
@@ -1044,6 +1046,27 @@ attributes: | |||
[ role_arn: <string> ] | |||
``` | |||
|
|||
### `<teams_config>` | |||
|
|||
Teams notifications are sent via the [Incoming Webhooks](https://learn.microsoft.com/en-us/microsoftteams/platform/webhooks-and-connectors/what-are-webhooks-and-connectors). |
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.
Teams notifications are sent via the [Incoming Webhooks](https://learn.microsoft.com/en-us/microsoftteams/platform/webhooks-and-connectors/what-are-webhooks-and-connectors). | |
Microsoft Teams notifications are sent via the [Incoming Webhooks](https://learn.microsoft.com/en-us/microsoftteams/platform/webhooks-and-connectors/what-are-webhooks-and-connectors) API endpoint. |
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.
It is fixed in new commit
notify/teams/teams.go
Outdated
webhookURL *config.SecretURL | ||
} | ||
|
||
// Message card https://learn.microsoft.com/en-us/outlook/actionable-messages/message-card-reference |
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.
// Message card https://learn.microsoft.com/en-us/outlook/actionable-messages/message-card-reference | |
// Message card reference can be found at https://learn.microsoft.com/en-us/outlook/actionable-messages/message-card-reference. |
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.
It is fixed in new commit
notify/teams/teams.go
Outdated
resp, err := notify.PostJSON(ctx, n.client, n.webhookURL.String(), &payload) | ||
if err != nil { | ||
return true, notify.RedactURL(err) | ||
} |
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.
} | |
} | |
defer notify.Drain(resp) |
Let's make sure we close the connection.
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.
It is fixed in new commit
notify/teams/teams.go
Outdated
|
||
shouldRetry, err := n.retrier.Check(resp.StatusCode, resp.Body) | ||
if err != nil { | ||
return shouldRetry, err |
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.
return shouldRetry, err | |
return shouldRetry, notify.NewErrorWithReason(notify.GetFailureReasonFromStatusCode(resp.StatusCode), err) |
Let's make sure we add a reason to it.
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.
The 429 status code is not set in a standard way. Instead, if it hits rate limit, 200 OK is always send with 'Microsoft Teams endpoint returned HTTP error 429' in response content.
So GetFailureReasonFromStatusCode
is renamed to GetFailureReason
and add a new parameter responseContent string
to address this issue
notify/teams/teams_test.go
Outdated
// This is a test URL that has been modified to not be valid. | ||
var testWebhookURL, _ = url.Parse("https://example.webhook.office.com/webhookb2/xxxxxx/IncomingWebhook/xxx/xxx") | ||
|
||
func TestTeamsRetry(t *testing.T) { |
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 we please add 1 more test that is similar to what we have in https://github.com/prometheus/alertmanager/pull/3252/files
I think it's important that we test:
- The expected format of the JSON on the webhook api
- the reason codes
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.
The test is added in new commit
notify/teams/teams.go
Outdated
ThemeColor string `json:"themeColor"` | ||
} | ||
|
||
func New(c *config.TeamsConfig, t *template.Template, l log.Logger, httpOpts ...commoncfg.HTTPClientOption) (*Notifier, 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.
func New(c *config.TeamsConfig, t *template.Template, l log.Logger, httpOpts ...commoncfg.HTTPClientOption) (*Notifier, error) { | |
// New returns a new notifier that uses the Microsoft Teams Webhook API. | |
func New(c *config.TeamsConfig, t *template.Template, l log.Logger, httpOpts ...commoncfg.HTTPClientOption) (*Notifier, 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.
It is fixed in new commit
cmd/alertmanager/main.go
Outdated
@@ -56,6 +56,7 @@ import ( | |||
"github.com/prometheus/alertmanager/notify/pushover" | |||
"github.com/prometheus/alertmanager/notify/slack" | |||
"github.com/prometheus/alertmanager/notify/sns" | |||
"github.com/prometheus/alertmanager/notify/teams" |
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.
agreed
notify/teams/teams.go
Outdated
@@ -0,0 +1,130 @@ | |||
// Copyright 2019 Prometheus Team |
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.
(nit)
// Copyright 2019 Prometheus Team | |
// Copyright 2023 Prometheus Team |
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.
It is fixed in new commit
notify/teams/teams.go
Outdated
if alerts.Status() == model.AlertFiring { | ||
color = colorRed | ||
} | ||
if alerts.Status() == model.AlertResolved { | ||
color = colorGreen | ||
} |
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.
(nit)
if alerts.Status() == model.AlertFiring { | |
color = colorRed | |
} | |
if alerts.Status() == model.AlertResolved { | |
color = colorGreen | |
} | |
switch alerts.Status() { | |
case model.AlertFiring: | |
color = colorRed | |
case alerts.Status(): | |
color = colorGreen | |
} |
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.
It is addressed in new commit
notify/teams/teams.go
Outdated
|
||
level.Debug(n.logger).Log("incident", key) | ||
|
||
alerts := types.Alerts(as...) |
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.
(nit) can you move it right before L99?
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.
It is addressed in new commit
Thanks for your careful review. I just get back from a five-day vacation and I'll handle it later. |
@zhan9san thanks a lot four your great contribution. Since the MessageCards are marked "legacy" I was wondering if it might be beneficial to explore switching to AdaptiveCards. Of cause only if that's not a lot of effort or causes more delays in this feature being available. |
Thanks for pointing it out. I thought about the legacy issue at first. But to make the PR simple, I just make a lift-shift migration to make MSTeams work. Besides, this can be considered as an improvement in next PR. |
bf3b5ef
to
787079b
Compare
Would you kindly review this PR? |
Thank you again for taking the time to review the changes. Please let me know if there's anything I can do to make the process easier for you. |
Signed-off-by: Jack Zhang <jack4zhang@gmail.com>
Signed-off-by: Jack Zhang <jack4zhang@gmail.com>
787079b
to
13bda64
Compare
Signed-off-by: Jack Zhang <jack4zhang@gmail.com>
13bda64
to
9e017b6
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.
Sorry for the delay! Can you add msteams
to the list of integrations here?
Lines 283 to 294 in 83304da
for _, integration := range []string{ | |
"email", | |
"pagerduty", | |
"wechat", | |
"pushover", | |
"slack", | |
"opsgenie", | |
"webhook", | |
"victorops", | |
"sns", | |
"telegram", | |
} { |
Otherwise this is good for me.
Co-authored-by: Simon Pasquier <spasquie@redhat.com> Signed-off-by: Jack <jack4zhang@gmail.com>
Signed-off-by: Jack Zhang <jack4zhang@gmail.com>
Thanks for your review.
It's done. |
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.
Thanks! I'll wait a few days before merging in case @gotjosh has more comments :)
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
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
Thank you very much for your patience and your contributions ❤️ |
Now all we need is a new release to be build 😁 |
@zhan9san Amazing thanks a lot for this, we had to use Telegram until MS Teams is added, now we can use 1 messenger only :) @rgarcia89 I hope will be soon, finger crossed |
when can we have a new release? last release was like ~6 months ago.. |
@gotjosh @simonpasquier any idea when we can count with a new release? It looks like there is quite some demand for this PR to become available 🤔 |
Anyone tried to build an image including the merge by itself? Is there anything I should give a special look? I think this is the only reasonable way to get an updated image in time |
You can use the :main image. |
* Add msteams Signed-off-by: Jack Zhang <jack4zhang@gmail.com> --------- Signed-off-by: Jack Zhang <jack4zhang@gmail.com> Signed-off-by: Jack <jack4zhang@gmail.com>
Dear all - this as now been released as part of 0.26 |
@@ -896,6 +907,7 @@ type Receiver struct { | |||
SNSConfigs []*SNSConfig `yaml:"sns_configs,omitempty" json:"sns_configs,omitempty"` | |||
TelegramConfigs []*TelegramConfig `yaml:"telegram_configs,omitempty" json:"telegram_configs,omitempty"` | |||
WebexConfigs []*WebexConfig `yaml:"webex_configs,omitempty" json:"webex_configs,omitempty"` | |||
MSTeamsConfigs []*MSTeamsConfig `yaml:"msteams_configs,omitempty" json:"teams_configs,omitempty"` |
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.
Should we align the configuration name? I am wondering why we call msteams
for yaml but teams
for json.
@zhan9san @simonpasquier
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 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.
Nice catch!
* Add msteams Signed-off-by: Jack Zhang <jack4zhang@gmail.com> --------- Signed-off-by: Jack Zhang <jack4zhang@gmail.com> Signed-off-by: Jack <jack4zhang@gmail.com>
* Add msteams Signed-off-by: Jack Zhang <jack4zhang@gmail.com> --------- Signed-off-by: Jack Zhang <jack4zhang@gmail.com> Signed-off-by: Jack <jack4zhang@gmail.com>
Related to #3076
As the
new line
is important to Markdown format, anested template definition
__text_alert_list_markdown
is added.For simplicity, only
text
field is used to send message in Message card.The message format is similar to Discord's.
Card design can be checked in Card Playground with following json text.