Caching context deadline exceeded result about sql_exporter

[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]

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. 👍

[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. 👍 ⚡