-
Notifications
You must be signed in to change notification settings - Fork 1.2k
websocket target support #1278
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
base: master
Are you sure you want to change the base?
websocket target support #1278
Conversation
19da630 to
9d8f9ec
Compare
|
Any updates here? |
If only.. althrough the year isn't passed yet :) |
|
Could you either approve this merge or let us know what needs to be revised? |
| FailIfNoneMatchesRegexp []string `yaml:"fail_if_none_matches_regexp,omitempty"` | ||
| } | ||
|
|
||
| type WebsocketProbe struct { |
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 you add tests for the probe config.
|
|
||
| ``` | ||
|
|
||
| ### `<tls_config>` |
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.
I am bit confused by this change in tls_config, why is this added?
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 you address this requested change? this looks like a duplicate of the existing <tls_config> section
config/config.go
Outdated
| QueryResponse []QueryResponse `yaml:"query_response,omitempty"` | ||
| } | ||
|
|
||
| type HTTPClientConfig struct { |
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.
any reasons to redefine this instead of using config.HTTPClientConfig from github.com/prometheus/common/config package?
we use it in HTTPProbe so it would be a good idea to use the same struct and keep things common and uniform.
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.
config.HTTPClientConfig is used to construct new http client using prometheus internal functions while I need to create a custom Dialer with a set of http headers to establish a connection. I still don't see any way to create raw headers from it, so it needs a third-party implementation to provide full support of the config options (including password files etc). That's why I made my own config structure.
| InsecureSkipVerify bool `yaml:"insecure_skip_verify,omitempty"` | ||
| } | ||
|
|
||
| type HTTPBasicAuth struct { |
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.
same comment as HTTPClientConfig, the BasicAuth in the github.com/prometheus/common/config has File support and is more rich, any reasons to NOT use it?
if we use github.com/prometheus/common/config's HTTPClientConfig, we don't need this type.
prober/websocket.go
Outdated
| registry.MustRegister(httpStatusCode) | ||
|
|
||
| dialer := websocket.Dialer{ | ||
| TLSClientConfig: &tls.Config{ |
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.
once we use the TLSConfig from github.com/prometheus/common/config, please configure more fields so end users can control those.
TLS MinVersion and TLS ServerName seems to be something I can think we need to expose but it also make sense to expose CA cert and client cert configs so folks can configure tls for websocket probes.
prober/websocket.go
Outdated
|
|
||
| if len(module.Websocket.QueryResponse) > 0 { | ||
| probeFailedDueToRegex := prometheus.NewGauge(prometheus.GaugeOpts{ | ||
| Name: "probe_failed_due_to_regex", |
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.
same here, please see metric names in other probers and follow their lead.
| registry.MustRegister(probeFailedDueToRegex) | ||
|
|
||
| queryMatched := true | ||
| for _, qr := range module.Websocket.QueryResponse { |
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: this block is deeply nested and hard to read. can you simplify it and make it easier to read please.
|
please rebase with master and resolve conflicts and update the PR desc to follow the new PR template: https://github.com/prometheus/blackbox_exporter/blob/master/.github/PULL_REQUEST_TEMPLATE.md 🙏🏼 |
|
@adnull Could you bring the pull request up to date so it can be merged into master? Do you need any help with that? |
|
@bilkoua Hi! Will do shortly. |
|
@adnull any updates? Do you need any help with that? |
Signed-off-by: ad <[email protected]>
Signed-off-by: adnull <[email protected]>
Signed-off-by: adnull <[email protected]>
Co-authored-by: Suraj Nath <[email protected]> Signed-off-by: Alex <[email protected]>
869a6a3 to
6017309
Compare
|
@bilkoua Just rebased on master, fixed suggested things, except using standart http and tls config structures. I can't use prometheus HTTPConfig as TLSConfig as they are only used in prometheus functions and probes whereas I need to make Dialer myself and I'm only able to pass headers into it, so it needs additional implementation to construct headers from prometheus config structures if we want to use it. |
|
@adnull Thank you very much. Could you also fix |
Signed-off-by: adnull <[email protected]>
6017309 to
0d8e05f
Compare
Signed-off-by: adnull <[email protected]>
|
@bilkoua Sure, fixed the things |
Signed-off-by: adnull <[email protected]>
|
@electron0zero Could you take a look at this PR after the changes? |
| X-API-Key: | ||
| files: | ||
| - /path/to/api-key.txt | ||
|
|
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.
trim this line and add the missing newline at the end of file
| ``` | ||
|
|
||
| **Example configurations:** | ||
|
|
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.
you should link to example.yaml for examples and add all the examples in one place (example.yml), only add the config options exposed by the websocket probe in this page.
|
|
||
| ``` | ||
|
|
||
| ### `<tls_config>` |
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 you address this requested change? this looks like a duplicate of the existing <tls_config> section
| ### `<tls_config>` | ||
|
|
||
| ```yml | ||
| [ http_config: <websocket_http_config>] |
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.
seems like a stray change? can you remove it?
| [ max_version: <string> ] | ||
| ``` | ||
|
|
||
| #### `<oauth2>` |
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 this be removed as well, looks like a copy of existing oauth config section?
| HTTPHeaders map[string]interface{} `yaml:"headers,omitempty"` | ||
| BasicAuth HTTPBasicAuth `yaml:"basic_auth,omitempty"` | ||
| BearerToken string `yaml:"bearer_token,omitempty"` | ||
| TLSConfig *tls.Config `yaml:"tls_config,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.
any reasons to not use config.TLSConfig in this case? we use that type across all probes so it's better to stay consistent with TLS config options we expose?
| type WSHTTPClientConfig struct { | ||
| HTTPHeaders map[string]interface{} `yaml:"headers,omitempty"` | ||
| BasicAuth HTTPBasicAuth `yaml:"basic_auth,omitempty"` | ||
| BearerToken string `yaml:"bearer_token,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.
This is a secret so you should be using Secret type from the common/config: https://github.com/prometheus/common/blob/d80d8544703e59a080a204b6f7429ac6561fb24f/config/config.go#L28
if you don't use Secret type in this case, the token will be logged by the logs and will be exposed in the config page.
| } | ||
|
|
||
| type WSHTTPClientConfig struct { | ||
| HTTPHeaders map[string]interface{} `yaml:"headers,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.
please use https://github.com/prometheus/common/blob/main/config/headers.go
they have secret type, support for files and validation logic methods that you can reuse those.
see https://github.com/prometheus/common/blob/d80d8544703e59a080a204b6f7429ac6561fb24f/config/headers.go#L116 on how to build headers from all sources.
|
|
||
| type HTTPBasicAuth struct { | ||
| Username string `yaml:"username"` | ||
| Password string `yaml:"password"` |
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.
password should be Secret type.
| } | ||
|
|
||
| func (c *HTTPBasicAuth) BasicAuthHeader() string { | ||
| return "Basic " + base64.StdEncoding.EncodeToString([]byte(c.Username+":"+c.Password)) |
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.
since we are converting the basic auth into a Header, do we need to support a basic auth?
we can just document that they can use Basic Auth with headers and let users set this header by supported methods, including files, etc.
|
@electron0zero Thanks |
According to #1081 this change adds websocket target support to monitor availability and functionality of websocket servers.
This probe provides features as follows:
New metrics:
CHANGES: