nsqadmin: not respecting "use-statsd-prefixes" flag

[kesutton]

I have just upgraded to NSQ 0.3.6, looking for the code fixed in #655.

However, it now seems as if the "use-statsd-prefixes" field is not being honored by nsqadmin.

Here is the output from NSQ Admin, running with both a true and false value for "use-statsd-prefixes":

[root@utility1 nsq]# nsqadmin --graphite-url="http://stats.crucible.dev.net:8000" --lookupd-http-address="utility1.crucible.dev.net:4161" --statsd-interval=1s --use-statsd-prefixes=true --statsd-prefix="nsq.%s"
[nsqadmin] 2015/09/28 14:33:00.074970 nsqadmin v0.3.6 (built w/go1.5.1)
[nsqadmin] 2015/09/28 14:33:00.075289 HTTP: listening on [::]:4171
[nsqadmin] 2015/09/28 14:33:01.788801 200 GET / (172.16.200.118:56701) 1.139171ms
[nsqadmin] 2015/09/28 14:33:01.922098 200 GET /static/bootstrap.min.css (172.16.200.118:56701) 14.103816ms
[nsqadmin] 2015/09/28 14:33:01.948642 200 GET /static/base.css (172.16.200.118:56702) 172.897µs
[nsqadmin] 2015/09/28 14:33:01.952700 200 GET /static/main.js (172.16.200.118:56704) 4.191885ms
[nsqadmin] 2015/09/28 14:33:01.954154 200 GET /static/vendor.js (172.16.200.118:56703) 5.683797ms
[nsqadmin] 2015/09/28 14:33:02.108851 200 GET /static/nsq_blue.png (172.16.200.118:56703) 574.021µs
[nsqadmin] 2015/09/28 14:33:02.113791 CI: querying nsqlookupd http://utility1.crucible.dev.net:4161/topics
[nsqadmin] 2015/09/28 14:33:02.117304 200 GET /api/topics (172.16.200.118:56704) 3.618575ms
[nsqadmin] 2015/09/28 14:33:02.120256 200 GET /fonts/glyphicons-halflings-regular.woff2 (172.16.200.118:56701) 189.417µs
[nsqadmin] 2015/09/28 14:33:02.132503 200 GET /static/main.js.map (172.16.200.118:56702) 12.481754ms
[nsqadmin] 2015/09/28 14:33:02.161327 GRAPHITE: http://stats.crucible.dev.net:8000/render?format=json&from=-2sec&target=sumSeries%28nsq.%2A.topic.test.message_count%29&until=-1sec
[nsqadmin] 2015/09/28 14:33:02.177694 ERROR: panic in HTTP handler - runtime error: index out of range
[nsqadmin] 2015/09/28 14:33:02.177726 500 GET /api/graphite?metric=rate&target=sumSeries(nsq.*.topic.test.message_count) (172.16.200.118:56703) 1.454µs
^C[nsqadmin] 2015/09/28 14:33:15.466658 HTTP: closing [::]:4171

[root@utility1 nsq]# nsqadmin --graphite-url="http://stats.crucible.dev.net:8000" --lookupd-http-address="utility1.crucible.dev.net:4161" --statsd-interval=1s --use-statsd-prefixes=false --statsd-prefix="nsq.%s"
[nsqadmin] 2015/09/28 14:33:20.252003 nsqadmin v0.3.6 (built w/go1.5.1)
[nsqadmin] 2015/09/28 14:33:20.252366 HTTP: listening on [::]:4171
[nsqadmin] 2015/09/28 14:33:22.111209 200 GET / (172.16.200.118:56709) 1.117955ms
[nsqadmin] 2015/09/28 14:33:22.243650 200 GET /static/bootstrap.min.css (172.16.200.118:56709) 14.126061ms
[nsqadmin] 2015/09/28 14:33:22.269607 200 GET /static/base.css (172.16.200.118:56710) 154.28µs
[nsqadmin] 2015/09/28 14:33:22.273389 200 GET /static/main.js (172.16.200.118:56712) 4.05738ms
[nsqadmin] 2015/09/28 14:33:22.276272 200 GET /static/vendor.js (172.16.200.118:56711) 5.646069ms
[nsqadmin] 2015/09/28 14:33:22.432552 200 GET /static/nsq_blue.png (172.16.200.118:56712) 596.538µs
[nsqadmin] 2015/09/28 14:33:22.437141 CI: querying nsqlookupd http://utility1.crucible.dev.net:4161/topics
[nsqadmin] 2015/09/28 14:33:22.437359 200 GET /fonts/glyphicons-halflings-regular.woff2 (172.16.200.118:56709) 162.377µs
[nsqadmin] 2015/09/28 14:33:22.444791 200 GET /api/topics (172.16.200.118:56711) 7.740818ms
[nsqadmin] 2015/09/28 14:33:22.449087 200 GET /static/main.js.map (172.16.200.118:56710) 11.959882ms
[nsqadmin] 2015/09/28 14:33:22.492023 GRAPHITE: http://stats.crucible.dev.net:8000/render?format=json&from=-2sec&target=sumSeries%28nsq.%2A.topic.test.message_count%29&until=-1sec
[nsqadmin] 2015/09/28 14:33:22.498486 ERROR: panic in HTTP handler - runtime error: index out of range
[nsqadmin] 2015/09/28 14:33:22.498513 500 GET /api/graphite?metric=rate&target=sumSeries(nsq.*.topic.test.message_count) (172.16.200.118:56711) 677ns

In both cases, NSQ doesn't seem to be appending the "stats.gauge" prefix that occurs in this file:

nsq/nsqadmin/static/js/lib/handlebars_helpers.js

Line 17 in 2a8dab2

prefix = 'stats.gauges.' + prefix;

However, when "use-statsd-prefixes" is set to true, it should be appending "stats.gauges" or "stats.counters" wherever necessary.

Is it possible my PR for issue #655 broke something? Or is the combination of configuration parameters not correct?

[mreiferson]

This is a bug... it doesn't look like we're passing the value of that flag to the client, so AppState.get('USE_STATSD_PREFIXES') always returns false-y.

@kesutton any interest in submitting a patch?

[kesutton]

I am attempting to fix this, but for some reason when I build the updated version to test locally, it doesn't pull in the updated index.html file.

Here are the steps I am executing to build nsqadmin:

gpm install #grabs the dependencies successfully
kesutton: ~/workspace/go/src/github.com/nsqio/nsq$> make clean all
rm -fr build
go build -o /Users/kellysutton/workspace/go/src/github.com/nsqio/nsq/build/apps/nsqd ./apps/nsqd
go build -o /Users/kellysutton/workspace/go/src/github.com/nsqio/nsq/build/apps/nsqlookupd ./apps/nsqlookupd
go build -o /Users/kellysutton/workspace/go/src/github.com/nsqio/nsq/build/apps/nsqadmin ./apps/nsqadmin
go build -o /Users/kellysutton/workspace/go/src/github.com/nsqio/nsq/build/apps/nsq_pubsub ./apps/nsq_pubsub
go build -o /Users/kellysutton/workspace/go/src/github.com/nsqio/nsq/build/apps/nsq_to_nsq ./apps/nsq_to_nsq
go build -o /Users/kellysutton/workspace/go/src/github.com/nsqio/nsq/build/apps/nsq_to_file ./apps/nsq_to_file
go build -o /Users/kellysutton/workspace/go/src/github.com/nsqio/nsq/build/apps/nsq_to_http ./apps/nsq_to_http
go build -o /Users/kellysutton/workspace/go/src/github.com/nsqio/nsq/build/apps/nsq_tail ./apps/nsq_tail
go build -o /Users/kellysutton/workspace/go/src/github.com/nsqio/nsq/build/apps/nsq_stat ./apps/nsq_stat
go build -o /Users/kellysutton/workspace/go/src/github.com/nsqio/nsq/build/apps/to_nsq ./apps/to_nsq

Then I run the built binary via:

build/apps/nsqadmin --lookupd-http-address=127.0.0.1:4161 --use-statsd-prefixes=true --graphite-url="http://stats.crucible.iron.net:8000"

When I browse to localhost:4171, the old index.html file appears. Is there an extra step I need to run to get the build process to read in the static/ folders down nsqadmin?

[mreiferson]

nsqadmin has a slightly different workflow than the other tools, see https://github.com/nsqio/nsq/tree/master/nsqadmin#working-locally

[kesutton]

I was able to update the code to make this work, and in doing so, found another "bug". For counters, Statsd appends ".count" and ".rate" to the client's stat key. Nsqadmin doesn't necessarily care about the ".rate" metric, but without the ".count" suffix on each counter-type key, nsqadmin can not find the relevant statistic in graphite.

I am assuming that there are some statsd implementations that do not append .count and .rate, and thus I think nsqadmin should only assume the ".count" suffix exists when "stats.counters" is the prefix. So I have added code that checks if the statistic is a "counter", AND if "use-statsd-prefixes" is true (I understand that the conf field name doesn't truly reflect all the functionality associated with it, now that it truly means "use statsd prefixes AND suffixes, where necessary"). If true, then ".count" is appended to the overall key.

What do you think about this choice of implementation, and the fact that the config field name doesn't fully represent the functionality associated with it. Changing it wouldn't break backward binary compatibility, but conf files would need to be updated, along with all documentation files.

[mreiferson]

@kesutton maybe a more future proof solution might be to allow an sprintf style flag for each of counters and gauges, this way a user could specify stats.counters.%s.count, etc.

What do you think?

[mreiferson]

(and we can deprecate the --use-statsd-prefixes in a future release)

[kesutton]

I believe the following set of parameters would make the most sense:

--nsq-statsd-prefix="nsq.%s"
The prefix used by nsqd (--statsd-prefix) for keys sent to statsd (%s for host replacement).
(This is essentially a name update to the statsd-prefix nsqadmin conf flag, and is not required. I just thought it more clearly represented the meaning of this field.)

--statsd-counter-format="stats.counters.%s.count"
The "counter" stats formatting applied by the implementation of statsd. Leave blank if none.

--statsd-gauge-format="stats.gauges.%s"
The "gauge" stats formatting applied by the implementation of statsd. Leave blank if none.

Deprecated:
--use-statsd-prefixes: use statsd-counter-format and statsd-gauge-format
--statsd-prefix: use nsq-statsd-prefix

I believe the default for nsq-statsd-prefix should remain "nsq.%s". However, I'm not sure what you think the default should be for the other two. Right now, it looks like use-statsd-prefixes defaults to true, indicating that the two new ones should have a set, non-blank default as well. However, that leads to the situation where a user who does not want those formats enforced having to specify the flags, but leave the values empty, which is odd.

What do you think?

[kesutton]

Also, with regard to deprecating flags - Does the old functionality have to be valid still? This change requires a few function rewrites, which would break the functionality the deprecated flags imply. It would be odd, but doable, to write code so it still works, it just wouldn't be as pretty.

[mreiferson]

I don't think it makes sense to change the name of --statsd-prefix to --nsq-statsd-prefix, however we can deprecate it and introduce --statsd-gauge-format and --statsd-counter-format and default them to stats.gauges.nsq.%s and stats.counters.nsq.%s.count, respectively.

[kesutton]

What about respecting the "use-statsd-prefixes" flag? Shall I just ignore/remove any functionality related to that flag specfically? Keep in mind the functionality that currently exists is broken with regard to the missing ".count" suffix.

[mreiferson]

It should all be implemented in a backwards compatible way, unfortunately (for the code). We can clean it up when we remove the deprecated options (ping #367)

[kesutton]

I would suggest changing the default of use-statsd-prefixes to "false" then, rather than true.

[kesutton]

Hm... now that I think about it, I'm confused to how to mold these two overlapping fields together. I think leaving the default as "true" is best. In the interim time period, if the user sets "use-statsd-prefixe" to false, then the default counter/gauge formats will NOT be applied. Otherwise, the counter/gauge formats will be applied.

[mreiferson]

see #662