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

Already on GitHub? Sign in to your account

Configurable metric namespace #137

Merged
merged 15 commits into from Nov 25, 2012

Conversation

Projects
None yet
8 participants
Owner

mrtazz commented Aug 8, 2012

A first stab at a clean implementation of key namespacing based on ideas brought up in other pull requests. This whole pull request handles namespaces in the graphite backend only. Other backends can handle their respective namespacing differently.

There is now an option available to change the global stats prefix to something else, as well as not having one by setting it to the empty string (""). The same goes for counters, timers and gauges which are now located under their type identifiers per default.

Dieterbe and others added some commits Jun 18, 2012

@Dieterbe Dieterbe make prefixes for all metrics configurable a188a69
@mrtazz mrtazz Merge branch 'configurable-prefix' of https://github.com/Dieterbe/statsd
 into configurable-metric-namespace

Conflicts:
	backends/graphite.js
	exampleConfig.js
3903ddd
@mrtazz mrtazz modify key namespacing to be easier to configure
This makes some changes to what the default key namespaces are and how they are
configured. Basically it puts the config under the graphite key and doesn't
need the user to take care of where dots are set.
545d780

This pull request fails (merged 545d780 into 5a36ca0).

This pull request passes (merged 7247c70 into 5a36ca0).

Contributor

otac0n commented Aug 8, 2012

One thing: If we are going to take a backwards incompatible change anyways, could we organize counters in the same way we organize timers?

That is, a timer name like foo.timetaken becomes stats.timers.foo.timetaken.{min|max|count|mean|mean_90|...}, so can we do the same for counters? Namely, foo.ordercreated should (in my opinion) become stats.counters.foo.ordercreated.{rate|count}.

Owner

mrtazz commented Aug 8, 2012

I think the {rate/count} thing is a good idea. I really don't like how the
stats_counts is sent to Graphite atm.

On Wed, Aug 08, 2012 at 07:50:40AM -0700, John Gietzen wrote:

One thing: If we are going to take a breaking change, could we organize
counters more like timers?

That is, a timer name like foo.timetaken becomes
stats.timers.foo.timetaken.{min|max|count|mean|mean_90|...}, so can we do
the same for counters? Namely, foo.ordercreated should (in my opinion)
become stats.counters.foo.ordercreated.{rate|count}.

--
Reply to this email directly or [1]view it on GitHub.

References

Visible links

  1. #137 (comment)
Contributor

otac0n commented on backends/graphite.js in 545d780 Aug 8, 2012

This way of coalescing will prevent someone from using an empty prefix.

These should be config.graphite.prefixFoo !== undefined ? "foos" : config.graphite.prefixFoo.

Owner

mrtazz replied Aug 8, 2012

I'm not sure I understand that correctly. But your line does the complete opposite. If it's not undefined (has a value), it's set to "foos" and otherwise it's set to the key ( which is undefined then)?

Contributor

otac0n replied Aug 8, 2012

Ah, yeah, I have the Boolean logic backwards, but the point is that a simple || operator will not have the desired effect here.

Owner

mrtazz commented Aug 8, 2012

@otac0n The changes are not backwards incompatible as a whole. Only the numStats and stats_counts keys are located slightly different to respect the cleaned up namespacing. But you can still configure it to put the keys in the old place.

And an empty prefix is possible by setting it to "". This way it will not get pushed into the namespace.

Contributor

otac0n commented Aug 8, 2012

@mrtazz I'm saying that the code that is currently checked in will treat "" as the same as "unset" and will coalesce it to "stats" or whatever.

That is, "" || foo will always return foo. Check the lines of code I annotated.

Contributor

timbunce commented Aug 13, 2012

Just a note to say I'm really looking forward to this pull request, or something very like it, landing soon.

Contributor

mheffner commented Aug 13, 2012

In addition to this it would be nice to have a version of the "globalPrefix" variable at the top-level that was respected across backends. I think a single scoping namespace (e.g, "stg", "prod", "test") is the most common use case for name spacing and it would be nice to not have to specify that for each backend.

The default could be "" and the full namespace would be concatenated with the backend's globalPrefix: globalPrefix + backend[globalPrefix] + metric_name.

This is probably best to do in a separate issue/PR, but just thought I'd see what people thought.

Owner

mrtazz commented Aug 13, 2012

sounds like a reasonable idea. I'm going to add something like this to the Pull
request. I think this is the right place to discuss it.

On Mon, Aug 13, 2012 at 09:38:17AM -0700, Mike Heffner wrote:

In addition to this it would be nice to have a version of the
"globalPrefix" variable at the top-level that was respected across
backends. I think a single scoping namespace (e.g, "stg", "prod", "test")
is the most common use case for name spacing and it would be nice to not
have to specify that for each backend.

The default could be "" and the full namespace would be concatenated with
the backend's globalPrefix: globalPrefix + backend[globalPrefix] +
metric_name.

This is probably best to do in a separate issue/PR, but just thought I'd
see what people thought.

--
Reply to this email directly or [1]view it on GitHub.

References

Visible links

  1. #137 (comment)
Contributor

otac0n commented Aug 13, 2012

Sounds good, but I think they should be concatenated in the opposite order.
Reason 1: If you are programmatically accessing the data from multiple backends, it would be simplest to use a "ends with" check to find a particular stat. Like stats.Where(s => s.EndsWith("prod.logins.failures.count")).

Reason 2: The backend-specific prefix is intended to help separate StatsD stats from other sources, and should come first.

Reason 3: It would be silly (in graphite) to have 3 folders (prod, test, debug) that contained exactly one folder each (stats).

So, for those reasons, I feel that the key should look something like stats.prod.your.key.here

Contributor

mheffner commented Aug 14, 2012

@otac0n Agreed, that sounds like a better order.

Wondering if this is going to be pulled soon (or something like it). Waiting for this feature before we can implement statsd. Thanks!

Contributor

timbunce commented Sep 18, 2012

I just want to echo @type11error and say that we're also waiting for something like this to land soon.
And while i'm here, I agree with @otac0n (stats.prod.your.key.here).

Owner

mrtazz commented Sep 20, 2012

Sorry for the lag. I want to have this in master at the beginning of next week at the latest. Want to make it a bit more backwards compatible before merging.

@mrtazz mrtazz Merge branch 'master' into configurable-metric-namespace
Conflicts:
	backends/graphite.js
	exampleConfig.js
d9fa4c0

@ghost ghost assigned mrtazz Sep 25, 2012

mrtazz added some commits Sep 25, 2012

@mrtazz mrtazz add a flag for namespacing backwards compatibility
This introduces the `graphite.legacyNamespace` boolean flag which can be used
to maintain backwards compatibility to how stats are sent to graphite.
b35ab92
@mrtazz mrtazz update tests to reflect non-legacy namespace 55126a2
@mrtazz mrtazz add tests for graphite legacy namespace 3d15401
@mrtazz mrtazz add set type to configurable namespace 5f50a06
Owner

mrtazz commented Sep 25, 2012

The above commits add the possibility to continue to use the legacy namespace. The legacy namespace will probably stay default for now and the new one will have to be explicitly enabled. I want to give this some more testing time and also try to add a good implementation of the global prefix across backends.

@alfredodeza alfredodeza referenced this pull request in WoLpH/python-statsd Oct 1, 2012

Closed

Naming on metrics is not consistent #17

Contributor

timbunce commented Oct 25, 2012

Hello @mrtazz, any news on this? Anything we can do to help?

Contributor

otac0n commented Oct 29, 2012

We have pulled this branch into our production environment. It is working great for us, so far. I hope to see this land in master soon! Thanks, @mrtazz.

Owner

mrtazz commented Nov 1, 2012

@otac0n awesome. Are you running it with custom namespaces?

obazoud commented Nov 23, 2012

Awesome feature!
and I would love to see this feature merged in master.

Contributor

Dieterbe commented Nov 25, 2012

  • another readme typo: legacyNamspace
  • would it be possible to include a small script that moves whisper files around from the legacy locations to the new defaults? i bet that would be useful for pretty much every statsd user.
Owner

mrtazz commented Nov 25, 2012

Good catch. Yeah an upgrade script from the basic old to new namespace would be helpful I think.

@mrtazz mrtazz merged commit dbd0597 into master Nov 25, 2012

1 check passed

default The Travis build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment