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

fix prometheus output #793

Closed
wants to merge 2 commits into from

Conversation

davidbirdsong
Copy link
Contributor

if i understand the prometheus data model correctly, the current output
for this plugin is unusable

prometheus only accepts a single value per measurement. prior to this change, the range loop
causes a measurement to end up w/ a random value

for instance:

net,dc=sjc1,grp_dashboard=1,grp_home=1,grp_hwy_fetcher=1,grp_web_admin=1,host=sjc1-b4-8,hw=app,interface=docker0,state=live
bytes_recv=477596i,bytes_sent=152963303i,drop_in=0i,drop_out=0i,err_in=0i,err_out=0i,packets_recv=7231i,packets_sent=11460i
1457121990003778992

this 'net' measurent would have all it's tags copied to prometheus
labels, but any of 152963303, or 0, or 7231 as a value for
'net' depending on which field is last in the map iteration

this change expands the fields into new measurements by appending
the field name to the influxdb measurement name.

ie, the above example results with 'net' dropped and new measurements
to take it's place:
net_bytes_recv
net_bytes_sent
net_drop_in
net_err_in
net_packets_recv
net_packets_sent

i hope this can be merged, i love telegraf's composability of tags and
filtering

@davidbirdsong
Copy link
Contributor Author

also, if this gets accepted, can i have a hoodie?

@titilambert
Copy link
Contributor

Hello, I think this PR does the trick: #707

@davidbirdsong
Copy link
Contributor Author

I reviewed the PR, but only the comments. It appeared to only address the ability to prefix all measurement names. I never saw any mention of moving the field names into new measurements. Did I miss something?

@davidbirdsong
Copy link
Contributor Author

Also, I missed the tests. Fixing...

if i understand the prometheus data model correctly, the current output
for this plugin is unusable

prometheus only accepts a single value per measurement. prior to this change, the range loop
causes a measurement to end up w/ a random value

for instance:

net,dc=sjc1,grp_dashboard=1,grp_home=1,grp_hwy_fetcher=1,grp_web_admin=1,host=sjc1-b4-8,hw=app,interface=docker0,state=live
bytes_recv=477596i,bytes_sent=152963303i,drop_in=0i,drop_out=0i,err_in=0i,err_out=0i,packets_recv=7231i,packets_sent=11460i
1457121990003778992

this 'net' measurent  would have all it's tags copied to prometheus
labels, but any of 152963303, or 0, or 7231 as a value for
'net' depending on which field is last in the map iteration

this change expands the fields into new measurements by appending
the field name to the influxdb measurement name.

ie, the above example results with 'net' dropped and new measurements
to take it's place:
	net_bytes_recv
	net_bytes_sent
	net_drop_in
	net_err_in
	net_packets_recv
	net_packets_sent

i hope this can be merged, i love telegraf's composability of tags and
filtering
@davidbirdsong davidbirdsong force-pushed the prometheus-out-tuneups branch from 2340d48 to 1360036 Compare March 4, 2016 22:43
@davidbirdsong
Copy link
Contributor Author

prometheus_output related tests aren't failing now, just something related to spinning up kafka in docker.

@titilambert
Copy link
Contributor

Just relaunch the build ;)

@davidbirdsong
Copy link
Contributor Author

Done: I cloned your repo and checked out 64f9330b6690e3e82902f1507887b29f663f2979, built and re-ran. Curling the prometheus endpoint gives me unusable values:

net{dc="sjc1",grp_dashboard="1",grp_home="1",grp_hwy_fetcher="1",grp_web_admin="1",host="sjc1-b4-8",hw="app",hwy_pref="staging",interface="docker0",state="live"} 0
net{dc="sjc1",grp_dashboard="1",grp_home="1",grp_hwy_fetcher="1",grp_web_admin="1",host="sjc1-b4-8",hw="app",hwy_pref="staging",interface="em1",state="live"} 0

0 what?

also, those are the only values for 'net'. only the interface creates any cardinality in prometheus.

@davidbirdsong
Copy link
Contributor Author

I might have mis-understood your follow-up. Is the relaunch related to tests failing or whether my PR is necessary?

@titilambert
Copy link
Contributor

Related to test failing. If kafka failed you can just rebuild on circleci to get your build ok ;)

@sparrc
Copy link
Contributor

sparrc commented Mar 16, 2016

Thanks @davidbirdsong, but I'm going to close this in favor of #707, is there any reason to merge both?

@sparrc sparrc closed this Mar 16, 2016
@davidbirdsong
Copy link
Contributor Author

@sparrc can you take a closer look at this PR. Unless I'm completely mistaken, it addresses a completely different problem from #707.

In fact, I just built from that PR and find the problem I'm fixing still present.

Again, unless I'm missing something, the prometheus output plugin is pretty useless. A single 'net' measurement has it's value overwritten for each field in the influx metric.

Here's what telegraf presents to a prometheus server via it's output for the net plugin

net{host="sjc1-b4-8",interface="docker0"} 0
net{host="sjc1-b4-8",interface="em1"} 2.7782357037023e+13

so the measurement: net for interface: docker0 is 0. 0 what, bytes? this doesn't tell me anything. Also, the value changes randomly which I think can be explained by ranging over fields, go map ranges aren't ordered.

Please take another look at this.

@sparrc
Copy link
Contributor

sparrc commented Mar 16, 2016

oh, okay, please get the tests passing and I can review it

@sparrc sparrc reopened this Mar 16, 2016
@davidbirdsong
Copy link
Contributor Author

I'll work on the tests

I might take a little longer than normal. I submitted this prior to a big knee surgery, so I can only work on stuff in the when clarity returns for fleeting moments when the painkillers dosages wear off.

it usually connotes a single value type metric, appending just clutters
@davidbirdsong davidbirdsong force-pushed the prometheus-out-tuneups branch from 7961aa2 to cc530ae Compare March 18, 2016 04:53
@sparrc sparrc closed this in d09bb13 Mar 21, 2016
@sparrc
Copy link
Contributor

sparrc commented Mar 21, 2016

thank you @davidbirdsong

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