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

Sanitize key when globalKeySanitize is true #643

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ljestrada
Copy link

The existing code prevents tagged metrics from being posted even though the keyNameSanitize key in the configuration file is set to true.

Made a minor modification to have the if statement match the configuration option.

@BlueHatbRit
Copy link
Member

Hmm, not sure how this ever slipped through. It seems quite simple, I'm wondering if we've misunderstood the relation between globalKeySanitize and keyNameSanitize. I'll take a deeper look at the docs and if this is all good then I'll merge it in.

PS: Sorry for the delay, this project's been dead for a while and is now being reivived.

@BlueHatbRit BlueHatbRit added the bug A bug within statsd or it's bundled proxies label Jul 12, 2019
@DanCech
Copy link
Contributor

DanCech commented Mar 26, 2020

The problem with this fix is that the intent of the function is to sanitize the key name to make it graphite-safe if it wasn't already sanitized due to global key sanitize being enabled.

If globalKeySanitize is true then the key has already been sanitized and we don't need to process it again.

If globalKeySanitize is false then the key may contain unsafe characters and we need to sanitize it before sending to graphite.

The minimal patch to support tagged series is to add ; and = as allowed characters in the sanitize regular expressions, so they don't get stripped when sanitizing. I'll open a PR for that, and also for a larger patch that properly handles tags.

@BlueHatbRit
Copy link
Member

Hey @DanCech thanks for this and the PR submissions, I'll give them a review asap. As you can imagine things are quite busy right now with covid-19 but this is on my list and it hasn't been forgotten. I'll look to check them over and integrate them as soon as I can.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A bug within statsd or it's bundled proxies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants