gjcarneiro / yacron

A modern Cron replacement that is Docker-friendly
MIT License
449 stars 38 forks source link

Added Statsd metrics sending #12

Closed bofm closed 6 years ago

bofm commented 6 years ago

Added the ability to send job related metrics to Statsd. example config:

jobs:
  - name: test
    command: echo "hello"
    schedule: "* * * * *"
    statsd:
      host: my-statsd.example.com
      port: 8125
      prefix: my.cron.test

With the config above Yacron will send these metrics to Statsd over UDP:

my.cron.test.start:1|g  #  this one is sent when the job starts
my.cron.test.stop:1|g  # this one and the ones below are sent when the job stops
my.cron.test.success:1|g
my.cron.test.duration:123|ms|@0.1
coveralls commented 6 years ago

Coverage Status

Coverage increased (+0.2%) to 93.978% when pulling d4bcf2bb367da9129ba46cdd8d1ae8218b5de796 on bofm:statsd into 89013f3f985ec8436e7c4dccd25f9c443e0bf9e1 on gjcarneiro:master.

gjcarneiro commented 6 years ago

That's a good idea, thanks for the contribution!

I need to finish yacron 0.6 (rc1 was tagged) before merging this feature.

On the code itself, I don't like the RunningJobWithStatsd subclassing RunningJob approach. Instead you should create a new class and call it from the regular RunningJob. See https://en.wikipedia.org/wiki/Composition_over_inheritance

bofm commented 6 years ago

I don't like the RunningJobWithStatsd subclassing RunningJob approach

I could create a StatsdReporter and add it to REPORTERS list, but the reporters are only triggered at the end of the job execution. And we'd still need to send a metric right after the job start. This way Statsd-related code would split in two parts. Or it would be necessary to change the existing RunningJob class.

Maybe I don't understand it well. Could you please clarify how would you want it to be done.

gjcarneiro commented 6 years ago

Well, you don't need to call it a "reporter", just call it, I don't know, StatsdJobMetric

class StatsdJobMetric:
   def job_started(self): ...
   def job_stopped(self, succeded: bool): ...

In RunningJob.__init__ you can create StatsdJobMetric

def __init__(self):
    ...
    self.statsd_metric = StatsdJobMetric()

Then call self.statsd_metric.job_started() and self.statsd_metric.job_stopped() at the appropriate places. No subclassing needed ;-)

I don't care too much about the names, but I must insist on no inheritance please.

gjcarneiro commented 6 years ago

Much much better, thank you!

May take me a few days for me to merge, but looks perfect as it is.

gjcarneiro commented 6 years ago

I mention you in HISTORY.rst as "bofm", let me know if you prefer to be referred to by another name. Thanks!