Move stat key name sanitization to Graphite backend. #155

Merged
merged 1 commit into from Feb 6, 2015

Conversation

Projects
None yet
7 participants
Contributor

mheffner commented Sep 24, 2012

Taking a stab at moving the metric name sanitization to the backend, as described originally in #110 (also refs #154).

This moves the Graphite-specific stat name regular expressions to the Graphite backend. Other backends should implement similar stat name expression cleanups based on their permitted character codes.

One issue that is not handled: Since the original code was performing these name changes at the time of ingress, it was possible that two stat reports could map to the same name. For example, the following two packets would map to the same timing metric:

glork:320|ms
gl+ork:320|ms

With this patch, these metrics will be tracked separately, but will both be submitted to Graphite using the same name "glork". Therefore, the second metric will likely overwrite the first one. It's not clear if this is actually behavior users are (or should be) depending on. The backend could go through and merge all metrics whose names map to the same key after sanitization, however, this might be tricky for metrics like gauges that track the last value (would require a timestamp). Thoughts?

Owner

mrtazz commented Oct 20, 2012

Sorry for the delay, looks pretty good. Since the sanitization is now a distinct function, we can haz tests? :)

Contributor

mheffner commented Oct 27, 2012

@mrtazz Yeah, I'll take a look at adding some tests to this.

huyl commented Mar 29, 2013

Hello, what's the status on this PR?

Contributor

mheffner commented Mar 29, 2013

@huyl I just rebased the original patch and added some tests to verify the name regexp is running.

@draco2003 @mrtazz This is the updated pull request I spoke with you about. The metric name regexp is now applied in the graphite backend. There is still the open issue of whether anyone is depending on the fact that multiple metric names could map to the same name post transform. If that's the case, then the graphite backend would need to merge the metric buckets.

Contributor

draco2003 commented Apr 10, 2013

@mheffner this looks good, can we add a config setting that defaults to the current way, and then we can update the backends to check for that setting and do the sanitization if it wasn't already done?
This lets us not break the current functionality, but allows us to deprecate it out down the road.

Thanks!

cc. @Dieterbe

Contributor

mheffner commented Apr 10, 2013

@draco2003 Yeah, I'll take a stab at that.

@mheffner mheffner Stat key name sanitization is now configurable at the top-level.
Setting keyNameSanitize to false pushes the requirement of sanitizing
key names to the backends. This permits backends that have less strict
character set requirements to take advantage of an expanded stat name
character set. The default behavior remains the same as collisions in
key name space are not handled if two different stat names map to the
same sanitized key name.
2d8aaf0
Contributor

mheffner commented Sep 12, 2013

@draco2003 I finally pushed a new version of this that makes the top-level key name sanitization configurable -- on by default. Let me know what you think!

Hello @mheffner @draco2003, is there any particular reason why this PR never moved forward?

Contributor

shaylang commented Sep 17, 2014

Hi,
When this PR is planned to be pushed ?
Thanks,
Shay

Contributor

mheffner commented Sep 17, 2014

@ivantopo @shaylang This probably needs some cleanup to get it merged into master again, if you're interested it could probably use some assistance. I've been busy with other projects recently.

A cleaned up PR may be more attractive given the age of this.

Contributor

shaylang commented Sep 22, 2014

i have created an updated pull request to current master
#451
how can we continue?

Owner

pathzzrd commented Sep 22, 2014

@shaylang looking to rope other maintainers in, but we're on it now.

ivantopo referenced this pull request in kamon-io/docker-grafana-graphite Oct 11, 2014

Closed

patched statsd #8

@pathzzrd pathzzrd merged commit 2d8aaf0 into etsy:master Feb 6, 2015

1 check passed

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