Skip to content

Add ntp collector#453

Merged
jaingaurav merged 1 commit intopython-diamond:masterfrom
BanjoInc:create_ntp_collector
Apr 29, 2016
Merged

Add ntp collector#453
jaingaurav merged 1 commit intopython-diamond:masterfrom
BanjoInc:create_ntp_collector

Conversation

@shortdudey123
Copy link
Copy Markdown
Member

Collect out of band stats from ntp

Uses output from ntpdate:

$ ntpdate -q pool.ntp.org
server 12.34.56.1, stratum 2, offset -0.000277, delay 0.02878
server 12.34.56.2, stratum 1, offset -0.000128, delay 0.02896
server 12.34.56.3, stratum 2, offset 0.000613, delay 0.02870
server 12.34.56.4, stratum 2, offset -0.000351, delay 0.02864
31 Apr 12:00:00 ntpdate[12]: adjust time server 12.34.56.2 offset -0.000128 sec
$

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage remained the same at 59.217% when pulling 2cc59d1 on BanjoInc:create_ntp_collector into cd851af on python-diamond:master.

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage decreased (-0.8%) to 58.41% when pulling e867c5a on BanjoInc:create_ntp_collector into cd851af on python-diamond:master.

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage decreased (-0.8%) to 58.41% when pulling 5b0734f on BanjoInc:create_ntp_collector into cd851af on python-diamond:master.

@shortdudey123
Copy link
Copy Markdown
Member Author

Hey @jaingaurav
Can you look at this PR? as well as a couple of my other ones? Thanks!!!

Comment thread src/collectors/ntp/ntp.py Outdated
offset_in_s = float(parts[9])
offset_in_ms = offset_in_s * 1000
offset_in_us = offset_in_s * 1000000
data['offset_s'] = {'val': offset_in_s, 'precision': 6}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So rather than having all these offset stats, how about you have the unit and precision in the config as an option and then use the unit conversion utils?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, do you have a good example of that?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shortdudey123
Copy link
Copy Markdown
Member Author

@jaingaurav made changes per feedback
Will squash this down when you are good with the PR

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage remained the same at 59.217% when pulling 3089f92 on BanjoInc:create_ntp_collector into cd851af on python-diamond:master.

@coveralls
Copy link
Copy Markdown

coveralls commented Apr 27, 2016

Coverage Status

Coverage remained the same at 59.217% when pulling d79ded1 on BanjoInc:create_ntp_collector into 7578c1b on python-diamond:master.

Comment thread src/collectors/ntp/ntp.py Outdated
# offset is in seconds, convert is to nanoseconds and miliseconds
offset_in_s = float(parts[9])

data['offset_s'] = {'val': offset_in_s,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So maybe I wasn't quite clear, but my suggestion was why not use a configuration option such as units and then only have a single offset metric published, or am I missing something here? Also you may want to expose a precision option too then.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was looking to have atleast msec and usec exposed but i just exposed sec as well

Is it safe to make the assumption that people are ok with their metric changing scale if they change the config option on a single metric namespace? (i.e. having sec and msec on the same metric name)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So why do you want both msec and usec exposed?

Regarding your latter point, you can stick with the style that the uptime collector has or use a suffix like you've currently done.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So why do you want both msec and usec exposed?

Actually, i looked a bit more and in reality i only need one

you can stick with the style that the uptime collector has or use a suffix like you've currently done.

I looked more at the uptime collector, and it sends the metrics in a different namespace depending on the scale you use. I missed that so its probably better if i follow that style :)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jaingaurav updated

@shortdudey123 shortdudey123 force-pushed the create_ntp_collector branch 2 times, most recently from cdc379e to 7ed10b2 Compare April 27, 2016 18:08
@coveralls
Copy link
Copy Markdown

coveralls commented Apr 27, 2016

Coverage Status

Coverage remained the same at 59.217% when pulling 7ed10b2 on BanjoInc:create_ntp_collector into c0544c0 on python-diamond:master.

@coveralls
Copy link
Copy Markdown

coveralls commented Apr 29, 2016

Coverage Status

Coverage remained the same at 59.217% when pulling d6ef032 on BanjoInc:create_ntp_collector into d0e6bc8 on python-diamond:master.

@shortdudey123
Copy link
Copy Markdown
Member Author

@jaingaurav changed server_count to server.count and squashed :)

Collect out of band stats from ntp

Uses output from ntpdate:

```
$ ntpdate -q pool.ntp.org
server 12.34.56.1, stratum 2, offset -0.000277, delay 0.02878
server 12.34.56.2, stratum 1, offset -0.000128, delay 0.02896
server 12.34.56.3, stratum 2, offset 0.000613, delay 0.02870
server 12.34.56.4, stratum 2, offset -0.000351, delay 0.02864
31 Apr 12:00:00 ntpdate[12]: adjust time server 12.34.56.2 offset -0.000128 sec
$
@coveralls
Copy link
Copy Markdown

coveralls commented Apr 29, 2016

Coverage Status

Coverage remained the same at 59.217% when pulling 98dd405 on BanjoInc:create_ntp_collector into d0e6bc8 on python-diamond:master.

@jaingaurav jaingaurav merged commit 6120813 into python-diamond:master Apr 29, 2016
@jaingaurav
Copy link
Copy Markdown
Member

Thanks a lot

@shortdudey123 shortdudey123 deleted the create_ntp_collector branch April 29, 2016 16:19
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