Add notify support for Chinese User wechat by whybeyoung · Pull Request #1059 · prometheus/alertmanager

whybeyoung · 2017-10-26T05:40:10Z

In China ,every people use wechat. Add support for wechat will make the alermanager more helpful and popular.

whybeyoung · 2017-10-26T06:47:46Z

why failed ci ? i make test self and not problem. but ci give the reason that confused me

josedonizetti · 2017-10-27T19:21:21Z

Alertmanager team is not actively adding new notifiers, it's recommended that you implement this custom notification via the webhook receiver. See prometheus/docs#843

whybeyoung · 2017-10-28T01:15:29Z

Wechat In Chinese have a huge number users. Not Every users know webhook or implement the webhook notification. I strongly recommend to merge this contribute. All the notifier's in alertmanger now is never used in chinese .BTW i think alertmanger will get more users in chinese, this is good things ,right?

stuartnelson3 · 2017-10-28T09:22:55Z

I will take a look at this when I have a chance. Please be patient, as I've been relatively busy with non-OSS things lately.

josedonizetti · 2017-10-28T15:43:22Z

Wechat might be big, but it's a communication app just like WhatsApp, Line, and Telegram. Each of those apps is big in different places, but they are not directed to devops like most of the notifiers currently supported.

whybeyoung · 2017-10-29T23:05:04Z

sorry， my words maybe excited.i forgot to point out taht wechat has two version，one is for communication ,and the other for enterprise
. so mine is support for qywechat which means enterprise wechat.And most people use qywechat to be alert component. Thanks @josedonizetti
Your help will be very much appreciated by each one of every qywechat users in china.

josedonizetti · 2017-10-31T01:38:01Z

@berlinsaint Thanks for clarifying. Indeed Wechat Work appears to be just like Slack and Hipchat. :)
https://work.weixin.qq.com/

whybeyoung · 2017-11-01T02:51:02Z

@josedonizetti yes. In chinese we hardly use slack or hipchat, and you know the reason.
i think the promethues project can have the more pluralism . and i will conintue to contribute or give more my opinion about the prometheus and alertmanger.Thanks for your reply.

stuartnelson3 · 2017-11-11T14:47:26Z

Sorry for the long delay, thanks @josedonizetti for following up on this!

stuartnelson3 · 2017-11-11T14:49:28Z

My main concern was that I have no experience with or potentially way to interact with wechat, so this notifier will have to be maintained by you or someone with the ability to confirm that a request leaving alertmanager is correctly accepted by wechat.

I'm happy to add this and review code, but the burden of verifying this works will be on the alertmanager users who use wechat.

stuartnelson3 · 2017-11-11T14:49:56Z

config/config.go

+					return fmt.Errorf("no global Wechat ApiSecret set")
+				}
+
+


Can you remove this extra whitespace?

ok, i will take a look, thanks

stuartnelson3 · 2017-11-11T14:52:06Z

config/config.go

 	OpsGenieAPIHost  string `yaml:"opsgenie_api_host,omitempty" json:"opsgenie_api_host,omitempty"`
+	WeChatAPIHost    string `yaml:"wechat_host,omitempty" json:"wechat_host,omitempty"`
+	WeChatAPISecret  string `yaml:"wechat_secret,omitempty" json:"wechat_secret,omitempty"`
+	WeChatAPICorpId  string `yaml:"wechat_corpid,omitempty" json:"wechat_corpid,omitempty"`


the yaml and json values should match the struct member names:

`yaml:"wechat_api_host,omitempty" json:"wechat_api_host,omitempty"`

for example

Also rename to wechat_api_url instead of wechat_api_host

stuartnelson3 · 2017-11-11T15:47:16Z

notify/impl.go

+	var wechatToken WechatToken
+	if resp.StatusCode/100 == 2 {
+		body, _ := ioutil.ReadAll(resp.Body)
+		if err := json.Unmarshal(body, &wechatToken); err != nil {


This could be changed to

if err := json.NewDecoder(resp.Body).Decode(&wechatToken); err != nil { // etc... }

you generally don't want to ignore errors, like what is being done right now with ioutil.ReadAll.

stuartnelson3 · 2017-11-11T15:48:12Z

notify/impl.go

+		}
+
+		access_token := wechatToken.AccessToken
+		postmessage_url := n.conf.ApiHost + "message/send?access_token=" + access_token


This can just be

postmessage_url := n.conf.ApiHost + "message/send?access_token=" + wechatToken.AccessToken

Golang style uses camel case, so postMessageURL instead of postmessage_url

stuartnelson3 · 2017-11-11T15:49:54Z

notify/impl.go

+				Type:    "text",
+				Safe:    "0"}
+		default:
+			apiURL = postmessage_url


You're setting apiURL = postmessage_url in both of these branches, so it can just be dropped and then use postmessage_url in resp, err = ctxhttp.Post(ctx, http.DefaultClient, apiURL, contentTypeJSON, &buf) below.

stuartnelson3 · 2017-11-11T15:50:52Z

notify/impl.go

+	}
+
+	resp, err = ctxhttp.Post(ctx, http.DefaultClient, apiURL, contentTypeJSON, &buf)
+	body, _ := ioutil.ReadAll(resp.Body)


The error needs to be checked before you read the response body.

stuartnelson3 · 2017-11-11T15:53:25Z

notify/impl.go

+	}
+	defer resp.Body.Close()
+
+	return false, nil


You should implement a retry method based on the wechat api documentation for when a request should be retried or not. See this example: https://github.com/prometheus/alertmanager/blob/master/notify/impl.go#L874-L884

stuartnelson3 · 2017-11-11T15:53:56Z

config/config.go

 				ogc.APIHost += "/"
 			}
 		}
+		//ADD WECHAT BY YYB


This can be removed

stuartnelson3 · 2017-11-11T15:54:48Z

notify/impl.go

 	return false, nil
 }

+//Wechat implements a Notfier for wechat notifications


There's a space between // and the first word, so it should be // Wechat ...

stuartnelson3 · 2017-11-11T15:55:05Z

notify/impl.go

+		}
+		alerts = types.Alerts(as...)
+	)
+	//get token


This can be removed

Thanks , i will correct it these days;

whybeyoung · 2017-12-01T14:30:12Z

@stuartnelson3 sorry for long delay, i have corrected the code. can it be merged or what other things that i should do now?

josedonizetti · 2017-12-02T15:54:34Z

config/config.go

 		}
+		for _, wcc := range rcv.WechatConfigs {
+			wcc.ApiURL = c.Global.WeChatAPIURL
+			if wcc.ApiURL == "" {


can u rename ApiURL to APIURL? Keep the same pattern found on other provider configs.

josedonizetti · 2017-12-02T15:56:17Z

config/config.go

 	OpsGenieAPIURL   string `yaml:"opsgenie_api_url,omitempty" json:"opsgenie_api_url,omitempty"`
+	WeChatAPIURL     string `yaml:"wechat_api_url,omitempty" json:"wechat_api_url,omitempty"`
+	WeChatAPISecret  string `yaml:"wechat_api_secret,omitempty" json:"wechat_api_secret,omitempty"`
+	WeChatAPICorpID  string `yaml:"wechat_api_corpid,omitempty" json:"wechat_api_corpid,omitempty"`


If you use camel case on the naming CorpID, you got to separate it with an underscore corp_id

josedonizetti · 2017-12-02T15:57:29Z

config/notifiers.go

+	CorpID    string `yaml:"corp_id,omitempty" json:"corp_id,omitempty"`
+	Message   string `yaml:"message,omitempty" json:"message,omitempty"`
+	ApiURL    string `yaml:"api_url,omitempty" json:"api_url,omitempty"`
+	ToUser    string `yaml:"touser,omitempty" json:"touser,omitempty"`


naming, ToUser config must be to_user

josedonizetti · 2017-12-02T15:57:49Z

config/notifiers.go

+	Message   string `yaml:"message,omitempty" json:"message,omitempty"`
+	ApiURL    string `yaml:"api_url,omitempty" json:"api_url,omitempty"`
+	ToUser    string `yaml:"touser,omitempty" json:"touser,omitempty"`
+	ToParty   string `yaml:"toparty,omitempty" json:"toparty,omitempty"`


naming, ToParty config must be to_party

josedonizetti · 2017-12-02T15:59:38Z

config/config.go

+					return fmt.Errorf("no global Wechat URL set")
+				}				
+			}
+			wcc.ApiSecret = c.Global.WeChatAPISecret


Got be consistent, using APISecret on the global config but ApiSecret on the local one, better be APISecret both places

josedonizetti · 2017-12-02T16:00:56Z

config/notifiers.go

+		return err
+	}
+	if c.ApiSecret == "" {
+		return fmt.Errorf("missing Wechat ApiSecret in Wechat config")


This can be tested. see https://github.com/prometheus/alertmanager/blob/master/config/notifiers_test.go#L147

josedonizetti · 2017-12-02T16:01:01Z

config/notifiers.go

+		return fmt.Errorf("missing Wechat ApiSecret in Wechat config")
+	}
+	if c.CorpID == "" {
+		return fmt.Errorf("missing Wechat CorpID in Wechat config")


josedonizetti · 2017-12-02T16:02:53Z

notify/impl.go

+	defer resp.Body.Close()
+	return n.retry(resp.StatusCode)
+}
+func (n *Wechat) retry(statusCode int) (bool, error) {


add test. see https://github.com/prometheus/alertmanager/blob/master/notify/impl_test.go#L47

josedonizetti · 2017-12-02T16:05:17Z

notify/impl.go

+		}
+		alerts = types.Alerts(as...)
+	)
+	var params = "corpid=" + n.conf.CorpID + "&corpsecret=" + n.conf.ApiSecret


can you use url.Values here instead of concacting? see https://github.com/prometheus/alertmanager/blob/master/notify/impl.go#L1026

thanks your review~

whybeyoung · 2017-12-03T13:23:33Z

i change the bindata.go， but i dont know how make it to pass the ci check. @josedonizetti

stuartnelson3

All the code in this pr needs to have gofmt run on it. You can do this with make format.

Try regenerating the assets with:

rm template/internal/deftmpl/bindata.go && make assets

stuartnelson3 · 2017-12-03T13:47:50Z

notify/impl.go

 	"strings"
 	"time"
-
+    "io/ioutil"


there needs to be a linebreak between the stdlib packages and the third-party github packages. So, just add a newline below this.

Signed-off-by: yb_home <berlinsaint@126.com>

whybeyoung · 2017-12-03T15:49:27Z

i used the windows to compile and have regenerated the bindata.go, one ci is ok, but it still not passed by the travis ci. it errors on step git-diff,(i really don't now what this step do) can you help me to sovle this? @josedonizetti

whybeyoung · 2017-12-05T02:09:16Z

@josedonizetti @stuartnelson3
is there any other things i need to do ,need i write the doc? and the above error i can't solve it thougn i execute the make assets.

this is my first pr on github, and i'm very glad and excited to see your review, you two are very nice and because of you i made a greate progress.Thanks , i will keep patience to notice the prometheus project, and i think will make more pr for you.

stuartnelson3 · 2017-12-06T16:08:19Z

@berlinsaint hey sorry for the delay, I'll will try tomorrow and see if I can get this to build successfully

stuartnelson3 · 2017-12-07T12:01:32Z

I was able to get your branch to successfully build against ci:

https://github.com/prometheus/alertmanager/compare/stn/test-ci-wechat

I executed the following command, and then committed the changes to bindata.go:

rm template/internal/deftmpl/bindata.go && make assets

whybeyoung · 2017-12-08T05:34:07Z

i execute the command and ,i m using windows ,so i don't have make , i use the makefile's command bin-data tool, and make the bindata.go .but it looks like a little different（length is the same but some bindata not the same ） with your bindata.go.

Thank you for you review, now is it ok to be merged? @stuartnelson3

i checkout the bindata.go in test-wechat-ci branch.

whybeyoung · 2017-12-08T06:06:22Z

i don't know why there is only one check , there were two check travis ci ,circle ci in the old(and always travis ci failed)
now is it become a ok status ?

stuartnelson3 · 2017-12-09T15:19:57Z

Not sure why it isn't showing up, but I checked any the changes you have build fine. Thanks for the effort!

whybeyoung · 2017-12-11T07:40:42Z

@stuartnelson3 And shall i need to write some doc about this notifier?？

jenningsloy318 · 2017-12-27T06:35:31Z

@berlinsaint can you share the config to use wechat for notification?

stuartnelson3 · 2018-01-08T09:59:03Z

@berlinsaint sorry for the delay, I didn't see this message until now. Adding documentation to https://github.com/prometheus/docs/blob/b20ed77d9307f06e0e2ff169abb51bc8d3460ebc/content/docs/alerting/configuration.md would be very much appreciated!

tj13 · 2018-03-05T04:48:39Z

@berlinsaint could you share some configuration docs about wechat integration?

opera443399 · 2018-03-28T08:13:02Z

no wechat related docs found...

simonpasquier · 2018-03-28T08:16:05Z

There's a pending PR (prometheus/docs#977) for updating the docs.

* Add node_exporter script for init.d Signed-off-by: gentlejo <josungil@gmail.com>

stuartnelson3 suggested changes Nov 11, 2017

View reviewed changes

stuartnelson3 reviewed Nov 11, 2017

View reviewed changes

config/config.go Outdated

ogc.APIHost += "/"

}

}

//ADD WECHAT BY YYB

Copy link

Contributor

stuartnelson3 Nov 11, 2017

Choose a reason for hiding this comment

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

This can be removed

stuartnelson3 reviewed Nov 11, 2017

View reviewed changes

whybeyoung closed this Dec 1, 2017

WECHAT support by ybyang2/berlinsaint

a4dc1d5

whybeyoung reopened this Dec 1, 2017

correct the whitespace

a0e1f04

josedonizetti reviewed Dec 2, 2017

View reviewed changes

whybeyoung added 2 commits December 3, 2017 13:58

add some TestFile and modify some naming errors by ybyang2/berlinsaint

516dd80

modify wechat retry test expect

72fc66b

template error

563f858

stuartnelson3 suggested changes Dec 3, 2017

View reviewed changes

add newline

c5a6cfb

Signed-off-by: yb_home <berlinsaint@126.com>

fmt some pr code

6c543a0

ybyang2 added 2 commits December 8, 2017 13:16

use the @stuartnelson3 the test-ci-wechat bingdata.go

8934bdf

notify go add wechat

5d9bb11

Merge branch 'master' into master

04dc233

stuartnelson3 approved these changes Dec 9, 2017

View reviewed changes

stuartnelson3 merged commit 6bab629 into prometheus:master Dec 9, 2017

KeithTt mentioned this pull request Jan 15, 2018

Can alertmanager be upgraded to a newer version? prometheus-operator/prometheus-operator#891

Closed

xizhibei mentioned this pull request Mar 18, 2018

feat: support dingtalk notification #1276

Closed

juliusv mentioned this pull request Apr 20, 2018

Document WeChat notification support prometheus/docs#1005

Closed

hh pushed a commit to ii/alertmanager that referenced this pull request Oct 4, 2018

Add node_exporter script for init.d (prometheus#1059)

2269df2

* Add node_exporter script for init.d Signed-off-by: gentlejo <josungil@gmail.com>

Conversation

whybeyoung commented Oct 26, 2017

Uh oh!

whybeyoung commented Oct 26, 2017

Uh oh!

josedonizetti commented Oct 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

whybeyoung commented Oct 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stuartnelson3 commented Oct 28, 2017

Uh oh!

josedonizetti commented Oct 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

whybeyoung commented Oct 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

josedonizetti commented Oct 31, 2017

Uh oh!

whybeyoung commented Nov 1, 2017

Uh oh!

stuartnelson3 commented Nov 11, 2017

Uh oh!

stuartnelson3 commented Nov 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josedonizetti Nov 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

whybeyoung commented Dec 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josedonizetti Dec 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josedonizetti Dec 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

josedonizetti commented Oct 27, 2017 •

edited

Loading

whybeyoung commented Oct 28, 2017 •

edited

Loading

josedonizetti commented Oct 28, 2017 •

edited

Loading

whybeyoung commented Oct 29, 2017 •

edited

Loading

josedonizetti Nov 11, 2017 •

edited

Loading

josedonizetti Dec 2, 2017 •

edited

Loading

josedonizetti Dec 2, 2017 •

edited

Loading