Caching context deadline exceeded result

[Maros112358]

Describe the bug
When context deadline is exceeded while using min_interval > 0, the sql_exporter caches the failed result.

To Reproduce
Steps to reproduce the behavior:
1, use the config below
2, go to /metrics
3, wait for timeout → scrape_duration 10 seconds + context deadline exceeded
4, refresh the page → scrape duration under 1 second + context deadline exceeded

Expected behavior
I expect sql_exporter to try to query the database again when the context deadline was exceeded during the last query.

Configuration

jobs:
  - job_name: my_job
    collectors: [mssql_timeout]
    static_configs:
      - targets:
          'my_target': 'my_connection_string'
collectors:
  - collector_name: mssql_timeout
    metrics:
      - metric_name: timeout_metrics
        type: gauge
        help: 'wait for 3 minutes'
        values: [xxx]
        query: |
          WAITFOR DELAY '00:03:00';

Additional context
First scrape:

# HELP scrape_duration_seconds How long it took to scrape the target in seconds
# TYPE scrape_duration_seconds gauge
scrape_duration_seconds{job="my_job",target="my_target"} 10.040869167
# HELP scrape_errors_total Total number of scrape errors per job, target, collector and query
# TYPE scrape_errors_total counter
scrape_errors_total{collector="mssql_timeout",job="my_job",query="timeout_metrics",target="my_target"} 10
# HELP up 1 if the target is reachable, or 0 if the scrape failed
# TYPE up gauge
up{job="my_job",target="my_target"} 1

Second scrape:

# HELP scrape_duration_seconds How long it took to scrape the target in seconds
# TYPE scrape_duration_seconds gauge
scrape_duration_seconds{job="my_job",target="my_target"} 0.374500292
# HELP scrape_errors_total Total number of scrape errors per job, target, collector and query
# TYPE scrape_errors_total counter
scrape_errors_total{collector="mssql_timeout",job="my_job",query="timeout_metrics",target="my_target"} 11
# HELP up 1 if the target is reachable, or 0 if the scrape failed
# TYPE up gauge
up{job="my_job",target="my_target"} 1

[burningalchemist]

Hey @Maros112358, thanks for reporting another issue! I'll have a look soon. I'll try to squeeze in the fix into the v0.13 release. 👍

[burningalchemist]

I've managed to reproduce the issue, and came up with a fix. I'll test it properly first to make sure we don't get any unexpected regressions. Maybe you can also test it on your end as well (you'd need to checkout the branch in the PR and build the exporter). That'd be helpful as I cannot test against MSSQL (there's no instance I can access, unfortunately).

We prefer to return partial results, so if among multiple metrics configured, there's at least one successful, we return this result for the cache period. If there's none (e.g. it's a single metric, or all of them failed), we try to fetch fresh metrics on the next scrape until the cache has some goods.

Please let me know if you see any flaws in my reasoning. :)

P.S. I also learnt that mssql driver doesn't respect proper context handling. This is the original issue, and this is a recent reference to it. But it doesn't make it different, just a fun fact.

[Maros112358]

Hello @burningalchemist, thank you for looking into it.

We prefer to return partial results, so if among multiple metrics configured, there's at least one successful, we return this result for the cache period.

Yes. Maybe it would also make sense, that in case of partial results (some failed, some succeeded) if result for specific metric/target is succesful, we return this result for the cache period. In case of partial results if the result was not succesful, we try to fetch fresh metrics on the next scrape until we succeed. What do you think?

If there's none (e.g. it's a single metric, or all of them failed), we try to fetch fresh metrics on the next scrape until the cache has some goods.

Yes, exactly. :)

[Maros112358]

I just checked the branch fix/cache-false and it works as expected. Great work. 👍 ⚡

[burningalchemist]

Yes. Maybe it would also make sense, that in case of partial results (some failed, some succeeded) if result for specific metric/target is succesful, we return this result for the cache period. In case of partial results if the result was not succesful, we try to fetch fresh metrics on the next scrape until we succeed. What do you think?

Ah, this is tricky. The exporter is generally a stateless service (which also justifies its small footprint). We have a temporary cache storage to prevent spamming databases with requests. We can also prevent populating cache with invalid metrics (this PR), so if the cache is empty - we try to fetch something instead of waiting for cache to expire.

The alternative is - if there's one invalid metric, we decline the whole scrape. This is more strict, but sometimes just wasting already retrieved info is cruel. Also, some users asked for the partial returns. But these are our only options without introducing more complexity.

Watching for specific metric results, unfortunately, requires adding some state machine and store more metadata in memory between scrapes. This would significantly increase the complexity of this solution and is undesired.

We increase scrape_errors_total for invalid metrics, so I think this can be used as a signal to check if something needs to be addressed.

[burningalchemist]

I just checked the branch fix/cache-false and it works as expected. Great work. 👍 ⚡

Cool, thanks for testing it! 🙏 I'll review the PR again with a fresh mind and hope to merge it soon. 👍