search

ricoberger / script_exporter

Prometheus exporter to execute scripts and collect metrics from the output or the exit status.
MIT License
354 stars 82 forks source link

Script Exporter max_timeout not taking effect if query or header timeouts not specified #77

Closed cah-rita-kassar closed 1 year ago

cah-rita-kassar commented 1 year ago

Configuring the maximum script timeout in Script Exporter's config.yaml file is not taking effect unless query or header timeouts are being specified alongside. Scripts are executing successfully even when their duration exceeds the max_timeout set.

I created a test_script that sleeps for 5 seconds before executing an echo command.

ping 192.0.2.0 -n 1 -w 5000 >nul
ECHO Script executed successfully

Then, in Script Exporter's config.yaml file, the script's max_timeout was set to 1 second.

scripts:
  - name: test_script
    command: .\test_script.bat
    timeout:
      max_timeout: 1.0
      enforced: true

The script was expected to be killed. Instead, it executed successfully.

image-2023-03-17-11-20-31-388

However, the max_timeout worked as expected when passed as a header in the GET request on Script Exporter's probe endpoint. As a result, the script was killed and did not return any value.

image-2023-03-17-13-14-37-832 
ts=2023-03-17T11:11:19.895Z caller=metrics.go:77 level=error msg="Run script failed" err="exit status 1"

The max_timeout also killed the test_script when the timeout was passed as a query in Script Exporter's probe endpoint.

image-2023-03-23-12-52-53-475 
ts=2023-03-23T10:23:41.217Z caller=metrics.go:77 level=error msg="Run script failed" err="exit status 1"

I think I found out how this is happening: https://github.com/ricoberger/script_exporter/blob/v2.11.0/pkg/exporter/scripts.go#L121

The code clearly shows that this will be the expected behavior

But is it in the intended behavior or is it a bug?

ricoberger commented 1 year ago

Hi @cah-rita-kassar, thanks for reporting. I'm with you and think it makes more sense to return the configured max timeout when the query parameter and header is not present. I created #78 to change the behavior accordingly.

@siebenmann since you contributed this feature, what do you think about it?

siebenmann commented 1 year ago

I agree with the change. I probably missed that this required the header set when I submitted the initial change (I don't think I touched that code at all), and always tested in a situation where I was submitting the header (or a timeout URL parameter).

ricoberger commented 1 year ago

Thanks for the fast feedback đŸ™‚