[inputs.system] new uptime metric with wrong type, renamed cpu metrics and other

[Daryes]

Use Case

Telegraf version : unreleased yet (would be 1.39.0)
System : multiples
module : inputs.system

Pull discussion : #18533

Hello,

First, thanks for the work done to include some OS informations, this will allow to drop the usage of scripts retrieving said data.

This said, I will chime on some of the latest changes applied to the inputs.system module.

1. New "uptime" metric wrongly changed to the gauge type.

Unless I'm mistaken, but given the code, the new "uptime" metric is registered as a gauge type, while it should have stayed a counter.
Influxdb and others does not care about this, but it is another matter for prometheus and alike :
the type is send back as a comment with the metric, and used by prometheus.

Given the prometheus documentation :

• A counter is a cumulative metric that represents a single monotonically increasing counter whose value can only increase or be reset to zero on restart.
  (...). Do not use a counter to expose a value that can decrease
• A gauge is a metric that represents a single numerical value that can arbitrarily go up and down.

The opposite can be said for a gauge, do not use a gauge for a metric that only increases.
Which is the case for the "uptime" metric, its value never decreases, aside from a restart.

Its type should be corrected back to "counter".

2. Deprecation of the metric "uptime_format"

I've seen the part of the discussion relating the deprecation since 7 years.

but honestly, I would like this to be kept, as the wording format is different from what can be achieved with grafana and others.
They usually allow only a static format which cannot be changed or easily altered, while uptime_format has a much more concise and/or understandable result for anybody.

• grafana (duration s): 1 week, 6 days, 8 hours
• grafana (duration d): 13 d 07:47:31
• uptime_format : 13 days, 7:47

After 7 years, it should be grandfathered instead of removed.

Also, its true this is a string metric, which should be not encouraged.
Its proper place would be in fact as a tag/label on the "uptime" metric, but it will cause some problems when used as a tag/label, as its value will change each time when gathered.
This would create new series (tag+metric) each time, each minute (or /10s), which would have other consequences.
So there is also not much point to think about moving it as a tag/label on the "uptime" metric.

3. Renaming of the "cpu" to "cpu_logical"

I think this is a mistake to rename a metric existing since the first version of Telegraf without a real reason.
Normalizing against the recently introduced "cpu_physical" metric is not enough to warrant this, given the impact : cpu, ram, drive, network and system are the first series of metrics used when monitoring anything.
I will not go in the debate where the term logical is not always applicable depending of the cpu, nor with physical vs socket.

One of the selling argument of telegraf is its stability with its metrics, implicitly, the metric names staying unaltered.
Changing this name will require to update all existing dashboard using it, either directly or in formulaes.
There is no need to open such a can of worm.

Thanks.

ps : would it be possible to implement a warning in the CI against the change of existing metrics name, to remind about the impact ?
At first glance, I'm not seeing a simple way for that, but maybe someone has an idea.

Expected behavior

N/A

Actual behavior

N/A

Additional info

N/A

[srebhan]

First of all thank you for reaching out @Daryes!

It would be helpful for us to split this issue into three, one for each of the points above. This way we can link each PRs to the corresponding issue and can keep track of what needs to be done. Can you split the issue please!?

Anyway let me quickly comment on your points:

1. New "uptime" metric wrongly changed to the gauge type.

I do agree here and we should change the type in a PR.

2. Deprecation of the metric "uptime_format"

For me this is the most controversial point in this item. While you may prefer the time formatting the string itself is useless in terms of timeseries data as you cannot "compute" anything from it without parsing (something you get for free with the new format). Furthermore, the grafana d format is pretty close, isn't it?

Adding it as a tag is out of question as the cardinality is basically unlimitted.

So the question is does the small benefit of having exactly this format really justify the additional code?

3. Renaming of the "cpu" to "cpu_logical"

The thought behind this is to better differentiate the n_cpus field from the physical counterpart as the original naming is unspecific. However, I do agree that changing names for the sake of changing names is not a good thing. We should revert this change I think.

ps : would it be possible to implement a warning in the CI against the change of existing metrics name, to remind about the impact ?

This is what unit-tests are for but I do agree that we can do better. PRs are welcome!

[Daryes]

@srebhan
It would be helpful for us to split this issue into three, one for each of the points above. This way we can link each PRs to the corresponding issue and can keep track of what needs to be done. Can you split the issue please!?

As requested, I've created 3 issues (and a bonus request)

Given the discussion should continue in each of them, I'm closing this issue, feel free to reopen if required.