Add fail_if_body_too_large http probe option
#1276
Conversation
hashworks
force-pushed
the
feature/fail_if_body_too_large
branch
from
81b9317 to
1e0ff2b
Compare
| @@ -550,7 +550,7 @@ func ProbeHTTP(ctx context.Context, target string, module config.Module, registr | |||
|
|
|||
| if !requestErrored { | |||
| _, err = io.Copy(io.Discard, byteCounter) | |||
| if err != nil { | |||
| if err != nil && (errors.Is(err, &http.MaxBytesError{}) || httpConfig.GetFailIfBodyTooLarge()) { | |||
There was a problem hiding this comment.
Please check if this is okay or if I should move this to another if so it's clearer.
There was a problem hiding this comment.
I would prefer another if for clarity
| @@ -450,6 +451,13 @@ func (s *HeaderMatch) UnmarshalYAML(unmarshal func(interface{}) error) error { | |||
| return nil | |||
| } | |||
|
|
|||
| func (h HTTPProbe) GetFailIfBodyTooLarge() bool { | |||
There was a problem hiding this comment.
Alternatively I could do the nil check in UnmarshalYAML, but then it could panic if someone changes the variable under the hood later on.
There was a problem hiding this comment.
To preserve the current behaviour, FailIfBodyTooLarge should be set to true by default.
you should be able to set it to true by default in DefaultHTTPProbe at https://github.com/prometheus/blackbox_exporter/blob/master/config/config.go#L51
if we make this bool instead of *bool, we do't have to worry about nil, and can remove this whole function.
This can be used in conjunction with `body_size_limit` when it is expected that endpoints return a body larger than the specified limit, e.g. streaming endpoints that potentially return infinite data. Signed-off-by: Justin Kromlinger <[email protected]>
hashworks
force-pushed
the
feature/fail_if_body_too_large
branch
from
1e0ff2b to
2d225ab
Compare
|
Do you want any changes here? Or is this a feature you don't want merged? |
Signed-off-by: Justin Kromlinger <[email protected]>
hashworks
force-pushed
the
feature/fail_if_body_too_large
branch
from
2a3f154 to
1512b95
Compare
|
can you share more on the use-case? and what problem this flag will solve? once I better understand the "why?" behind the change, I will find some time to review. |
Signed-off-by: Justin Kromlinger <[email protected]>
At work, we need to probe endpoints that endlessly stream data. Without this flag every probe will fail after some time, since Blackbox Exporter expects the data stream to finish otherwise. We want to measure successful requests and "time to X bytes". |
| })) | ||
| defer ts.Close() | ||
|
|
||
| for _, failIfBodyTooLarge := range []bool{true, false} { |
There was a problem hiding this comment.
can we turn into a proper table test with config, and the expectation, see TestFailIfNotSSLLogMsg for reference
| @@ -211,6 +211,7 @@ type HTTPProbe struct { | |||
| NoFollowRedirects *bool `yaml:"no_follow_redirects,omitempty"` | |||
| FailIfSSL bool `yaml:"fail_if_ssl,omitempty"` | |||
| FailIfNotSSL bool `yaml:"fail_if_not_ssl,omitempty"` | |||
| FailIfBodyTooLarge *bool `yaml:"fail_if_body_too_large,omitempty"` | |||
There was a problem hiding this comment.
any reason to use *bool here instead of just bool like FailIfNotSSL??
| @@ -450,6 +451,13 @@ func (s *HeaderMatch) UnmarshalYAML(unmarshal func(interface{}) error) error { | |||
| return nil | |||
| } | |||
|
|
|||
| func (h HTTPProbe) GetFailIfBodyTooLarge() bool { | |||
There was a problem hiding this comment.
To preserve the current behaviour, FailIfBodyTooLarge should be set to true by default.
you should be able to set it to true by default in DefaultHTTPProbe at https://github.com/prometheus/blackbox_exporter/blob/master/config/config.go#L51
if we make this bool instead of *bool, we do't have to worry about nil, and can remove this whole function.
| @@ -550,7 +550,7 @@ func ProbeHTTP(ctx context.Context, target string, module config.Module, registr | |||
|
|
|||
| if !requestErrored { | |||
| _, err = io.Copy(io.Discard, byteCounter) | |||
| if err != nil { | |||
| if err != nil && (errors.Is(err, &http.MaxBytesError{}) || httpConfig.GetFailIfBodyTooLarge()) { | |||
There was a problem hiding this comment.
I would prefer another if for clarity
|
please rebase to fix the failing CI. |
This can be used in conjunction with
body_size_limitwhen it is expected that endpoints return a body larger than the specified limit, e.g. streaming endpoints that potentially return infinite data.