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

[dev.icinga.com #6799] GraphiteWriter replacement of (in)valid characters #1832

Closed
icinga-migration opened this issue Jul 27, 2014 · 5 comments
Labels
area/metrics General metrics handling bug Something isn't working

Comments

@icinga-migration
Copy link

This issue has been migrated from Redmine: https://dev.icinga.com/issues/6799

Created by mhoyer on 2014-07-27 18:38:45 +00:00

Assignee: (none)
Status: Rejected (closed on 2015-01-12 09:43:39 +00:00)
Target Version: (none)
Last Update: 2015-01-12 09:43:39 +00:00 (in Redmine)

Icinga Version: 2.0.1

As far as I can see in the code, string cleanup is made by SanitizeMetric function. While there are some useful replacements, I would like the "-" replacement to be removed. It is a valid character for graphite metrics and used very often in common naming-patterns for hosts or services. This leads to a naming-mismatch between icinga and graphite.
I would also suggest to remove some other characters in order to have a resilient system, preventing bad metrics from being sent.

We should remove: "!\"#$%&'()*+,./:;<=>?@[\]^`{|}~".

Current implementation:
void GraphiteWriter::SanitizeMetric(String& str)
{
boost::replace_all(str, " ", "_");
boost::replace_all(str, ".", "_");
boost::replace_all(str, "-", "_");
boost::replace_all(str, "\\", "_");
boost::replace_all(str, "/", "_");
}


Relations:

@icinga-migration
Copy link
Author

Updated by mfriedrich on 2014-07-28 16:00:26 +00:00

Basically that's 2 issues

  • one behavior change with '-' vs '_'
  • replace additional invalid characters

Any change requires metric updates for existing installations.

@icinga-migration
Copy link
Author

Updated by mhoyer on 2014-07-28 19:48:26 +00:00

@dnsmichi, thats true, it's two changes at all. I wasn't sure whether I should create one or two issues for it.
What do you think about the update path. Shall we supply a script to do so or is it enough to give a note on that?
In my case we run a huge graphite cluster with a very special distributed setup on different machines than our icinga instances. Building a migration script for all purposes (and there will be many different ones) might be a bit overblown.

@icinga-migration
Copy link
Author

Updated by mfriedrich on 2014-11-09 15:21:02 +00:00

  • Status changed from New to Assigned
  • Assigned to set to mhoyer

Data integrity must be provided at all cost. Therefore your commit has not been merged or reviewed, sorry about that. Since graphite is written in Python, it should be an exception to provide a script being Python 2.4 compatible at least. Can you do that?

@icinga-migration
Copy link
Author

Updated by mfriedrich on 2015-01-07 14:22:52 +00:00

  • Relates set to 8149

@icinga-migration
Copy link
Author

Updated by mfriedrich on 2015-01-12 09:43:39 +00:00

  • Status changed from Assigned to Rejected
  • Assigned to deleted mhoyer

Won't happen.

@icinga-migration icinga-migration added bug Something isn't working area/metrics General metrics handling labels Jan 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/metrics General metrics handling bug Something isn't working
Projects
None yet
Development

No branches or pull requests

1 participant