Skip to content
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

Offset should not be applied for explicitly set timeout #491

Closed
tdabasinskas opened this issue Jun 20, 2019 · 3 comments · Fixed by #492
Closed

Offset should not be applied for explicitly set timeout #491

tdabasinskas opened this issue Jun 20, 2019 · 3 comments · Fixed by #492

Comments

@tdabasinskas
Copy link
Contributor

tdabasinskas commented Jun 20, 2019

As per the following code, blackbox_exporter always applies --timeout-offset (defaulting to 0.5s) to the timeout:

var timeoutSeconds float64
if v := r.Header.Get("X-Prometheus-Scrape-Timeout-Seconds"); v != "" {
var err error
timeoutSeconds, err = strconv.ParseFloat(v, 64)
if err != nil {
http.Error(w, fmt.Sprintf("Failed to parse timeout from Prometheus header: %s", err), http.StatusInternalServerError)
return
}
}
if timeoutSeconds == 0 {
timeoutSeconds = 10
}
if module.Timeout.Seconds() < timeoutSeconds && module.Timeout.Seconds() > 0 {
timeoutSeconds = module.Timeout.Seconds()
}
timeoutSeconds -= *timeoutOffset
ctx, cancel := context.WithTimeout(context.Background(), time.Duration(timeoutSeconds*float64(time.Second)))

There's a problem with this setting in the following cases:

  • --timeout-offset=0: Prometheus might fail to scrape blackbox_exporter as the timeout (when combined with the network latency) might exceed Prometheus scrape timeout (that's the reason it's not 0 by default).
  • --timeout-offset=0.5: it's not possible to have HTTP modules that expect response faster than 0.5 (as timeout: 500ms effectively becomes timeout: 0ms).

Would it be possible to apply the offset (timeoutSeconds -= *timeoutOffset ) only when there's no explicitly defined timeout for a module? Meaning, that if the user explicitly set timeout field (which, as per the conditions, must be more than 0 and less than Prometheus scrape timeout) for some specific module, we can assume the user knows what he's doing and we use this timeout as is (without applying any offset).

An alternative can also be having per-module timeout-offset property, which allows the module to override not only the default time but default offset as well.

I can do a PR if agreed.

@brian-brazil
Copy link
Contributor

only when there's no explicitly defined timeout for a module?

We can do that, presuming the header-offset is still a limit.

@tdabasinskas
Copy link
Contributor Author

By header-offset you mean X-Prometheus-Scrape-Timeout-Seconds? If so, yes, that would still be the upper limit.

@brian-brazil
Copy link
Contributor

Yes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants