search

justwatchcom / sql_exporter

Flexible SQL Exporter for Prometheus.
MIT License
402 stars 109 forks source link

Add synchronous jobs #10

Closed xxorde closed 2 years ago

xxorde commented 7 years ago

We needed a possibility to execute some jobs / queries synchronous every time /metrics is requested. So I added the infrastructure to execute jobs with an interval <=0 synchronous when the http-handler is called.

interval: '0s' # an interval <= 0 will make the queries synchronous

I hope my patch does not brings any drawback, just had to change some parts after importing your current version.

It would be great if you are interested in this feature and we could start a discussion.

xxorde commented 7 years ago

@dominikschulz what do you think about the feature in general and my patch?

Update: I rebased all commits to only have one and removed the debug output by using NewNopLogger(). I hope the PR is much easier to review now.

dominikschulz commented 7 years ago

Sorry for the long delay. I was thinking about this PR a few times already, but I don't really like the current approach. It feels to hacky, adding too many special cases.

We understand the need for synchronous queries, but I'd like to find a way that feels more natural.

bobrik commented 7 years ago

I'd really love to see synchronous queries. Async queries can easily misalign with scraping intervals and cause steady rates to appear as sawtooth.

xxorde commented 7 years ago

@dominikschulz I would really appreciate if you can find a more clean way to do this. For someone who is a more experienced developer and knows the involved libraries it might just be a few lines of code. There is not much logic involved.

What do you think about the way to identify a synchronous job by setting the interval to '0' ?

@bobrik that was our problem and the intention to add that feature!

df7cb commented 6 years ago

I just pushed a rebased and polished up version of this patch to the original location. (Sorry for deleting it some time ago.) It now also updates the README file to mention interval 0. Could you maybe have another look to see if it fits your idea, or what would need changing? We are running this patch in https://github.com/credativ/elephant-shed and would love to see it merged. Thanks!

df7cb commented 2 years ago

Fwiw, #51 is about snowflake support, not synchronous jobs, so this PR should have been left open.

dewey commented 2 years ago

Thanks, I'll take a look what went wrong there :\

mbanck-ntap commented 4 months ago

@dewey Well, looks like this PR is still marked merged while in fact it has not been merged yet?

mbanck-ntap commented 4 months ago

I have re-submitted this as #131 now as untangling this one looks difficult.

dewey commented 4 months ago

Thank you, much appreciated! I had a hard time to figure out how to undo that.