[dev.icinga.com #8149] graphite writer should pass "-" in host names and "." in perf data

[icinga-migration]

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

Created by kgremliza on 2015-01-02 11:43:21 +00:00

Assignee: mfriedrich
Status: Resolved (closed on 2015-09-06 09:25:04 +00:00)
Target Version: 2.4.0
Last Update: 2015-09-06 09:25:04 +00:00 (in Redmine)

---

1, Current implementation of GraphiteWriter::EscapeMetric replaces "" in final graphite strings. Usually host names ar e part of this. Valid strings like "host-a.icinga.check" will be renamed to "host_a...". "" is a valid hostname char.
It should remain intact.

2. Current implementation of GraphiteWriter::EscapeMetric replaces "." in final graphite strings. Dots are the graphite separator top build the graphite data hierarchy. If perfdata strings are like "df.root.kbytes" for obvious reasons they will be replaced to "df_root_kbytes".

File: lib/perfdata/graphitewriter.cpp

String GraphiteWriter::EscapeMetric(const String& str)
{
    String result = str;

    boost::replace_all(result, " ", "_");
    boost::replace_all(result, ".", "_");
    boost::replace_all(result, "-", "_");
    boost::replace_all(result, "\\", "_");
    boost::replace_all(result, "/", "_");

    return result;
}

should be:

String GraphiteWriter::EscapeMetric(const String& str)
{
    String result = str;

    boost::replace_all(result, " ", "_");
    boost::replace_all(result, "\\", "_");
    boost::replace_all(result, "/", "_");

    return result;
}

Changesets

2015-09-06 09:10:49 +00:00 by mfriedrich b10cb8a

Implement a better Graphite tree schema

This changes the entire tree, but with the prefix "icinga2"
not to conflict with existing installations. Includes
enable_legacy_mode and detailed documentation.

fixes #9461
fixes #8149

2015-10-20 06:06:25 +00:00 by (unknown) b77c9ed

Remove unnecessary default values

refs #9461
refs #8149

---

Relations:

• relates #6799
• relates #8149
• relates #8149

[icinga-migration]

Updated by gbeutner on 2015-01-07 08:20:59 +00:00

• Description updated

[icinga-migration]

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

• Relates set to 6799

[icinga-migration]

Updated by mfriedrich on 2015-01-07 14:25:16 +00:00

Similar to #6799 - any change requires metric updates for existing installations.

The second change regarding the dots is dangerous, I wouldn't allow that to be configured just by passing macro strings on-the-fly. Rather have a somewhat static format using the template strings in GraphiteWriter configuration itself.

[icinga-migration]

Updated by mfriedrich on 2015-01-12 12:28:37 +00:00

• Status changed from New to Rejected

After careful consideration we've decided not to implement this issue.

[icinga-migration]

Updated by kgremliza on 2015-01-12 12:34:26 +00:00

This may have been a careful consideration, but not a very wise one, as there seem to be no reason.

[icinga-migration]

Updated by dnebermann on 2015-03-10 16:28:03 +00:00

At least I could agree that for a host or metric name some characters could result in unexpected results like you named them in Bug #6799 ("!\"#$%&'()*+,./:;<=>?@[\]^`{|}~"). But I wouldn't thought of dash and dot. In case of dot you have to have in mind that the dot is a part of the path component!

Which were those considerations?

[icinga-migration]

Updated by kgremliza on 2015-03-10 16:55:47 +00:00

Hi,

exactly. Dot is part of the path component, so its use is totally intentional.

Take this output from one of my checks:

OK - Version:34 pool-a:ONLINE:33 pool-b:ONLINE:33 rpool:ONLINE:34 | 'pool-a.size'=140GB;;;; 'pool-a.alloc'=95.9GB;;;; 'pool-a.free'=43.6GB;;;; 'pool-a.alloc_pct'=68.50;80;90;;100 'pool-b.size'=99.5GB;;;; 'pool-b.alloc'=38.1GB;;;; 'pool-b.free'=61.4GB;;;; 'pool-b.alloc_pct'=38.29;80;90;;100 'rpool.size'=79.8GB;;;; 'rpool.alloc'=39.2GB;;;; 'rpool.free'=40.5GB;;;; 'rpool.alloc_pct'=49.12;80;90;;100

In a graphite env we have a data tree:

Stat..icinga..

We process data in a grafana dashboard with influx queries like:

/^$aggr\.$host\.icinga\.check_zpool\.$zpool\.alloc_pct$/

Having a dot in 'rpool.alloc' allows us to run separate queries for different types of data.

That’s why we like to have the "." preserved.

About the "-".
RFC 1912 says:

"Allowable characters in a label for a host name are only ASCII letters, digits, and the `-' character."

We have systems that use a '-' in their names. We also record data with collectd and fluentd into our influxDB.
Having icinga translate the dash would result in two nodes for the same host. It would also modify names like pool-a into pool_a confusing people without any need, I think.

Stat..icinga..
Stat.<host_a>.icinga..

We simply change the graphite.cpp module to not modify '.' and '-' and we are good. We use this stuff quite a lot and didn't encounter any problems yet.
I guess you will probably agree.

Would be nice to hear your considerations.

Regards, Konstantin

[icinga-migration]

Updated by gbeutner on 2015-03-11 12:14:10 +00:00

• Status changed from Rejected to New

I'm re-opening the ticket for now. We'll have to re-evaluate this. :)

[icinga-migration]

Updated by mfriedrich on 2015-04-29 15:59:03 +00:00

The thing which bugs me the most is upgrade safety for existing installations. I still don't see how this should work.

[icinga-migration]

Updated by kgremliza on 2015-04-30 08:44:33 +00:00

Yes, now I understand your objections.
Would it be possible to have a switch, so people could decide for themselves?

Btw. We moved to InfluxDB, still using carbon interface, because we were silently loosing data points in carbon.
It is much faster and provides an application cluster for reliability.
However in a new 0.9.0 on InfluxDB there is a new feature called tags, which is either hard to express in a carbon string or breaks our naming completely.

It would be great if we could have a json formatted output that could be pointed to some socket ?

Regards, Konstantin

[icinga-migration]

Updated by mfriedrich on 2015-04-30 09:56:12 +00:00

kgremliza wrote:

Yes, now I understand your objections.

You should always keep that in mind that upgrade safety is the primary target when developing software used in production environments :)

Would it be possible to have a switch, so people could decide for themselves?

From a maintenance point of view, I doubt that. The design decision should've been fixed before going with 2.0.0 stable, but I don't think this is something one should be able to control through a switch. Makes support and development tremendously hard.

Btw. We moved to InfluxDB, still using carbon interface, because we were silently loosing data points in carbon.
It is much faster and provides an application cluster for reliability.
However in a new 0.9.0 on InfluxDB there is a new feature called tags, which is either hard to express in a carbon string or breaks our naming completely.

It would be great if we could have a json formatted output that could be pointed to some socket ?

Different topic, only InfluxDB related. Open a new issue discussing InfluxDB capabilities please. 0.9.0 is still alpha afaik and not yet fully released.

[icinga-migration]

Updated by pdf on 2015-05-09 05:29:26 +00:00

I definitely want replacement characters to be configurable.

• Users should be able to set a preference on the GraphiteWriter with an array of strings that will be replaced, and possibly a preference for the replacement character, though the latter is less important
  > * Alternatively, allow specifying a regex to replace, this would be more efficient, but error-prone for the end-user (I'm surprised you're not using a single regex, rather than invoking replace_all multiple times in the existing code - it's just a character set, so it's no harder to maintain)
• The default values for these preferences should be initialized to match the current replacement list, so that existing installs are unaffected
• When documenting the feature, a warning should be included to the effect that adjusting these values may result in unexpected behaviour, and should only be done prior to activating the graphite feature
• Default config files should not include theses preferences, they must be added explicitly.

In reality, it's likely no one will go looking for these preferences unless they have a use-case. For those with a use-case, this is pretty important, and you're unlikely satisfy everyone with a rigid, static set of replacements.

[icinga-migration]

Updated by tgelf on 2015-06-19 16:19:11 +00:00

• Relates set to 9461

[icinga-migration]

Updated by tgelf on 2015-06-19 16:33:04 +00:00

I'd really like to see this implemented, ideally together with #9461. But please be careful, do it as explained in the initial description - not as suggested in the subfollowing discussion. Allow for dots in perfdata labels, but continue to escape them in hosts, services and other custom variables eventually configured to be part of the host/service_name_template.

Reason: filters in graphite do not play nice with variable dot counts. Immagine this filter:

icinga2.*.services.*.check_disk.perfdata.*.value

It would work with this tree:

icinga2.www_some_domain.services.*.check_disk.perfdata.*.value
icinga2.some_domain.services.*.check_disk.perfdata.*.value

But it would be nearly impossible to filter a tree looking as follows:

icinga2.www.some.domain.services.*.check_disk.perfdata.*.value
icinga2.some.domain.services.*.check_disk.perfdata.*.value

Cheers,
Thomas

[icinga-migration]

Updated by mfriedrich on 2015-06-23 13:36:41 +00:00

• Target Version set to Backlog

[icinga-migration]

Updated by mfriedrich on 2015-07-02 18:55:05 +00:00

• Relates set to 9515

[icinga-migration]

Updated by mfriedrich on 2015-09-06 08:30:36 +00:00

• Status changed from New to Assigned
• Assigned to set to mfriedrich
• Target Version changed from Backlog to 2.4.0

This will be resolved as part of #9461 as this feature allows to 1) use dashes in names 2) does not escape dots in perfdata labels.

[icinga-migration]

Updated by mfriedrich on 2015-09-06 09:05:07 +00:00

Details: https://dev.icinga.org/issues/9461#note-21

[icinga-migration]

Updated by mfriedrich on 2015-09-06 09:25:04 +00:00

• Status changed from Assigned to Resolved
• Done % changed from 0 to 100

Applied in changeset b10cb8a.