Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added Statsd metrics sending #12

Merged
merged 3 commits into from
Nov 17, 2017
Merged

Added Statsd metrics sending #12

merged 3 commits into from
Nov 17, 2017

Conversation

bofm
Copy link

@bofm bofm commented Nov 14, 2017

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
Copy link

coveralls commented Nov 14, 2017

Coverage Status

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

@gjcarneiro
Copy link
Owner

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
Copy link
Author

bofm commented Nov 14, 2017

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
Copy link
Owner

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
Copy link
Owner

Much much better, thank you!

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

@gjcarneiro gjcarneiro merged commit b42ed7a into gjcarneiro:master Nov 17, 2017
@gjcarneiro
Copy link
Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants