*: configure http client from config #930
Conversation
| @@ -176,6 +178,8 @@ func (c *EmailConfig) UnmarshalYAML(unmarshal func(interface{}) error) error { | |||
| type PagerdutyConfig struct { | |||
| NotifierConfig `yaml:",inline" json:",inline"` | |||
|
|
|||
| HTTPConfig *commoncfg.HTTPClientConfig `yaml:"http_config,omitempty" json:"http_config,omitempty"` | |||
There was a problem hiding this comment.
Should this go in NotifierConfig considering we use it everywhere?
There was a problem hiding this comment.
We have it everywhere except email configs, it would be odd to be able to have it there, no?
notify/impl.go
Outdated
| @@ -141,13 +142,13 @@ var userAgentHeader = fmt.Sprintf("Alertmanager/%s", version.Version) | |||
| // Webhook implements a Notifier for generic webhooks. | |||
| type Webhook struct { | |||
| // The URL to which notifications are sent. | |||
There was a problem hiding this comment.
Comment needs an update
There was a problem hiding this comment.
Not sure why the URL was exported before, I moved the Config -> conf to be consistent with all the other implementations.
|
If there are no general concerns anymore I'll start working on docs. |
|
No concerns from me, looks cool |
|
I just talked to @fabxc, he has some comments on the general topic of the common package. I'll hold off on merging util he has a look. |
|
I've plenty of concerns on this – but that's a longer discussion I hope to find time for soonish. Where else is this struct being used outside of AM? |
|
Right now the blackbox exporter. We likely also need it in the consul exporter. |
|
@fabxc @brian-brazil how should we continue on this, as it should be possible for people to configure this somehow. |
|
I've been proposing code in common for this for 18 months now, which is what the blackbox exporter uses. @fabxc is against us having common packages for this, while I strongly believe code like this (particularly security related) must come from a common library. |
I guess this would be the start of having this discussion, so we can come to an agreement of how to proceed. I personally don't object to having this particular piece in common, but am open to hearing counter arguments. |
|
To set the discussion, here is a recap of the concrete use cases that call for more flexibility on the HTTP client configuration:
|
|
We're all good with the use cases, it's exactly how we share the common code that there is disagreement on. |
|
Moving my question to this more recent thread. I still don't see code that supports http proxy for receivers and it looks like all the threads on this closed. Is my understanding correct? |
|
@brancz @brian-brazil @fabxc Now that Prometheus itself uses http client from the common library (see prometheus/prometheus#3674), can we reconsider this PR? |
|
I'm happy with going forward with this. |
|
@brancz, cool! This is your PR rebased on master: master...simonpasquier:httpcfg |
|
@simonpasquier I don't know when I would get to it, so if you can, go for it! |
Signed-off-by: Steve Kotsopoulos <sk@fywss.com>
Changes since 0.16.0-rc.3 * [CHANGE] align Darwin disk stat names with Linux prometheus#930 Signed-off-by: Ben Kochie <superq@gmail.com>
Changes since 0.16.0-rc.3 * [CHANGE] align Darwin disk stat names with Linux prometheus#930 Signed-off-by: Ben Kochie <superq@gmail.com>
It seems that many people are interested in configuring the HTTP client used to perform requests to the notification providers. As @Conorbro recently added support for this in the common/config package, I made use of it here.
Closes #853
@stuartnelson3 @fabxc @mxinden @gouthamve @brian-brazil