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

Rollback GraphiteSanitize to replacing whitespaces #1099

Merged
merged 1 commit into from Mar 10, 2017

Conversation

Projects
None yet
4 participants
@arteam
Member

arteam commented Mar 9, 2017

As reported in #1098 the current sanitizer is too aggressive. It strips dots which play an important role in the Graphite metrics name convention. Until we develop a better algorithm, let's do only basic sanitization.

Rollback GraphiteSanitize to replacing whitespaces
As reported in #1098 the current sanitizer is too aggressive. It strips
dots which play an important role in the Graphite metrics name convention.

Until we develop a better algorithm, let's do only basic sanitization.
@arteam

This comment has been minimized.

Show comment
Hide comment
@arteam
Member

arteam commented Mar 9, 2017

@jplock jplock added this to the 3.2.1 milestone Mar 9, 2017

@jplock

This comment has been minimized.

Show comment
Hide comment
@jplock

jplock Mar 9, 2017

Member

LGTM

Member

jplock commented Mar 9, 2017

LGTM

@nickbabcock

This comment has been minimized.

Show comment
Hide comment
@nickbabcock

nickbabcock Mar 9, 2017

Contributor

I feel like we should keep the ability to create metric names that don't crash graphite or else issues like #637 becomes relevant again. IMO, rollback the behavior but expose GraphiteSanitize#sanitize so that users can create metric names that won't crash graphite.

Contributor

nickbabcock commented Mar 9, 2017

I feel like we should keep the ability to create metric names that don't crash graphite or else issues like #637 becomes relevant again. IMO, rollback the behavior but expose GraphiteSanitize#sanitize so that users can create metric names that won't crash graphite.

@husseincoder

This comment has been minimized.

Show comment
Hide comment
@husseincoder

husseincoder Mar 9, 2017

Contributor

Awesome .. thanks alot for acting so fast on this :)

Contributor

husseincoder commented Mar 9, 2017

Awesome .. thanks alot for acting so fast on this :)

@arteam

This comment has been minimized.

Show comment
Hide comment
@arteam

arteam Mar 10, 2017

Member

I feel like we should keep the ability to create metric names that don't crash graphite or else issues like #637 becomes relevant again. IMO, rollback the behavior but expose GraphiteSanitize#sanitize so that users can create metric names that won't crash graphite.

Users can override the sanitize method on graphite senders, so it should be possible to provide a custom sanitization.

Member

arteam commented Mar 10, 2017

I feel like we should keep the ability to create metric names that don't crash graphite or else issues like #637 becomes relevant again. IMO, rollback the behavior but expose GraphiteSanitize#sanitize so that users can create metric names that won't crash graphite.

Users can override the sanitize method on graphite senders, so it should be possible to provide a custom sanitization.

@arteam arteam merged commit 9e47bca into 3.2-development Mar 10, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@arteam arteam deleted the rollback_to_whitespace_serializing branch Mar 10, 2017

@nickbabcock

This comment has been minimized.

Show comment
Hide comment
@nickbabcock

nickbabcock Mar 10, 2017

Contributor

But the issue is that the Graphite senders do not see the individual named parts of a metric, only the whole name, so the problem remains. Ideally one would sanitize the individual parts like I showed in #1098. I thought it would be convenient to have the code in the base metric library because anyone who is using graphite and may have metrics remotely based on user input should be sanitizing them.

Contributor

nickbabcock commented Mar 10, 2017

But the issue is that the Graphite senders do not see the individual named parts of a metric, only the whole name, so the problem remains. Ideally one would sanitize the individual parts like I showed in #1098. I thought it would be convenient to have the code in the base metric library because anyone who is using graphite and may have metrics remotely based on user input should be sanitizing them.

@arteam

This comment has been minimized.

Show comment
Hide comment
@arteam

arteam Mar 10, 2017

Member

That's true. I just wanted to rollback to the original behaviour so non-TCP senders are not broken.

Member

arteam commented Mar 10, 2017

That's true. I just wanted to rollback to the original behaviour so non-TCP senders are not broken.

@nickbabcock

This comment has been minimized.

Show comment
Hide comment
@nickbabcock

nickbabcock Mar 10, 2017

Contributor

Yeah and that was the right decision. I'll kick around a PR for users to opt into an individual name graphite sanitizer 😄

Contributor

nickbabcock commented Mar 10, 2017

Yeah and that was the right decision. I'll kick around a PR for users to opt into an individual name graphite sanitizer 😄

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