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

rename depth and backend_depth to match other *_counter ? #7

Open
tamsky opened this issue Apr 14, 2016 · 3 comments
Open

rename depth and backend_depth to match other *_counter ? #7

tamsky opened this issue Apr 14, 2016 · 3 comments

Comments

@tamsky
Copy link

tamsky commented Apr 14, 2016

I feel like the metric names could be made more consistent.
Every gauge exported by nsq_exporter uses _counter as its suffix, except depth and backend_depth.

Any thoughts on updating their names?

When aggregating, it's somewhat jarring to see two metrics with different suffixes being added together:

The expression (depth + in_flight_count) looks strange...
even though they're both gauges.

@zwopir
Copy link
Contributor

zwopir commented Apr 15, 2016

Hi Marc,

you are right. The metrics naming is inconsistent. The naming is taken from the underlaying stats-endpoint call results - so it's inconsistent "upstream".
I would suggest renaming in_flight_count to in_flight since the metrics aren't counter in a prometheus way.

@tamsky
Copy link
Author

tamsky commented Apr 16, 2016

Thanks for looking at this. And sorry in advance for the giant reply....

As far as metric names go, for gauges, the preference is for them to mention the plural of whatever they are counting.
In nsq, most of the time we're counting "messages", so I'd think we'd want to use that, at least when it's appropriate.

take your pick, but everything to the right seems better...
depth << depth_length << queue_length << queued_messages
backlog << backlog_length << backlog_queue_length << backlogged_messages
in_flight_count << in_flight_length << in_flight_messages

Digging deeper into this, it has more surface area than I would have thought.

nsq_exporter declares all of its metrics as gauges, but in nsqd they are not all gauges.
Internally nsqd doesn't yet distinguish between gauges and counters, but for all practical purposes, it has both, it just doesn't do a good job exposing which are which yet.

[ I only discovered this after looking at the nsqd code ]

Perhaps we can work together to improve nsq_exporter so it properly conveys the actual type for each metric?

As well, I can work on improving nsqd so it keeps its metrics tidy as counters or gauges, and uses better, more consistent names for them.

I would suggest renaming in_flight_count to in_flight since the metrics aren't counter in a prometheus way.

For now, I'd see these as beneficial changes for nsq_exporter:

  1. removing _count from all the Gauge metrics that nsq_exporter declares (most of them are actually counter-types in nsqd, so using _total would actually be the most appropriate, excepting depth/backlog/in_flight which are gauges.)
  2. renaming sample_rate to sample_rate_percent [*]

@tamsky
Copy link
Author

tamsky commented Apr 17, 2016

I just found nsqadmin has a helper function for statsd that maps the metrics as gauges or counters:

https://github.com/nsqio/nsq/blob/abecbef00cb4a6a880e26a223646c206e71f71e6/nsqadmin/static/js/lib/handlebars_helpers.js#L23-L36

which totally disproves my earlier bold statement:

most of them are actually counter-types

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

No branches or pull requests

2 participants