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

Allow more characters in graphite tags #9249

Merged
merged 13 commits into from
May 18, 2021
Merged

Allow more characters in graphite tags #9249

merged 13 commits into from
May 18, 2021

Conversation

G-regL
Copy link
Contributor

@G-regL G-regL commented May 7, 2021

Required for all PRs:

  • Updated associated README.md.
  • Wrote appropriate unit tests.

resolves #9248

Added a new function to the Graphite Serializer that will use a specifc sanitzer for tags, along with supporting changes elsewhere.

As it's a relatively breaking change, it's hidden behind a (currently poorly named) feature flag called "graphite_tag_new_sanitize". That name needs to be changed, but I just can't for the life of me think of a better one.

@telegraf-tiger telegraf-tiger bot added feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin new plugin plugin/serializer labels May 7, 2021
Copy link
Member

@helenosheaa helenosheaa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good. Just a few comments around different naming for clarity, linter errors and not adding a bool option to allow for future extension.

@helenosheaa
Copy link
Member

I think you could link this PR to #8877 also

Copy link
Member

@helenosheaa helenosheaa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks for making those changes so quickly!

@G-regL
Copy link
Contributor Author

G-regL commented May 14, 2021

No problem. I really want this feature, as I'm about to go through a massive new deployment and would love tag values to look right. Any idea what release it will apply to?

@helenosheaa
Copy link
Member

I think it will make it into the next feature release v1.19.0 which will be in June

Copy link
Contributor

@ssoroka ssoroka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. the regexes are a bit dense but the test coverage looks decent. One comment from me.

@Hipska Hipska linked an issue May 17, 2021 that may be closed by this pull request
@Hipska Hipska added the fix pr to fix corresponding bug label May 17, 2021
@Hipska Hipska added area/graphite plugin/serializer and removed feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin labels May 17, 2021
@Hipska Hipska requested a review from ssoroka May 17, 2021 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/graphite fix pr to fix corresponding bug plugin/serializer
Projects
None yet
4 participants