Navigation Menu

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

Should a stat name with a ":" error? #25

Closed
arcoraven opened this issue Feb 24, 2016 · 4 comments
Closed

Should a stat name with a ":" error? #25

arcoraven opened this issue Feb 24, 2016 · 4 comments

Comments

@arcoraven
Copy link

I ran into an issue where I was sending stats to a statname with a host IP/port where I replaced the . in the IP with _ but forgot the port delimiter :, like so: stats.gauges.<service>.production.11_22_33_44:6800

Because the : is treated as a delimiter in statsd, it was ignoring the gauge value I passed in and logging the statname stats.gauges.<service>.production.11_22_33_44 with the value 6800. Should the statsd client reject a statname with a : (and possibly other delimiters I may not be aware of)?

@dropwhile
Copy link
Member

Invalid characters would probably be anything outside of ^[a-zA-Z_-0-9.].

Given that checking every character of a metric name would add some amount of overhead, I am honestly not sure it is worth it for the general case (for a given case it may be very worth it). I think sanitization of the state name may best be handled in the application itself -- since the app author can decide what behavior makes sense for them: stripping bad chars, panic, silently drop, log and drop the stat, or return an error.

It is conceivable that some people may also desire unfiltered names. Perhaps if they are using a work-a-like protocol, or are abusing the stat name field to shoehorn in some custom behavior.

If it were to be implemented, I would probably do it an optional way similar to the recent SetSamplerFunc code, with a default of none.

So while I am not convinced it is necessary (first time I have heard this request), I think adding a line or two to the documentation regarding characters to avoid in the stat name, or adding a helper standalone function (statsd.CheckName func(string) err or something) to assist authors in doing their own validation, would be very reasonable.

@dropwhile
Copy link
Member

Just an update on this.

I added a util name checker function, and showed an example of usage in the test-client code.

I have also created a stat-validation branch, to see how the code worked out. I am still not sure if I want to merge it or not yet, but it is there for you to look at if you are interested.

@arcoraven
Copy link
Author

I'm able to get by just making sure I sanitize my stat names correctly, but thanks for the update. I'm sure others may find that validation helpful.

@dropwhile
Copy link
Member

I will close this issue for now. If others run into the same problem, I can reference this and/or merge that branch with pluggable validation.

Thanks again for the feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

2 participants