Skip to content

Comments

Fix invalid metric name panic reading powercap dir#1800

Closed
chicknsoup wants to merge 8 commits intoprometheus:masterfrom
chicknsoup:master
Closed

Fix invalid metric name panic reading powercap dir#1800
chicknsoup wants to merge 8 commits intoprometheus:masterfrom
chicknsoup:master

Conversation

@chicknsoup
Copy link

@chicknsoup chicknsoup commented Jul 27, 2020

Issue:
When the rz.Name contains invalid metric character ("-") like this:
# cat /sys/class/powercap/intel-rapl:0/name
package-15

Node exporter will panic

panic: "node_rapl_package-15_joules_total" is not a valid metric name

Signed-off-by: chinhnc chicknsoupuds@gmail.com

Issue:
When the rz.Name contains invalid metric character ("-") like this:
# cat /sys/class/powercap/intel-rapl:0/name
package-15

Node exporter will panic

```
panic: "node_rapl_package-15_joules_total" is not a valid metric name
```

Signed-off-by: chinhnc <chicknsoupuds@gmail.com>
Signed-off-by: chinhnc <chicknsoupuds@gmail.com>
Signed-off-by: chinhnc <chicknsoupuds@gmail.com>
@chicknsoup
Copy link
Author

How to fix the incompatible module? @roidelapluie

go: found github.com/prometheus/prometheus/util/strutil in github.com/prometheus/prometheus v2.5.0+incompatible

@roidelapluie
Copy link
Member

The fix here is maybe putting that name in a label value and fixing the metric name

Signed-off-by: chinhnc <chicknsoupuds@gmail.com>
Formatting

Signed-off-by: chinhnc <chicknsoupuds@gmail.com>
@chicknsoup
Copy link
Author

The fix here is maybe putting that name in a label value and fixing the metric name

Already put the name in a label-value pair. But, not sure how to fix the failed tests.

Removed old rapl metrics

Signed-off-by: chinhnc <chicknsoupuds@gmail.com>
Signed-off-by: chinhnc <chicknsoupuds@gmail.com>
Signed-off-by: chinhnc <chicknsoupuds@gmail.com>
@discordianfish
Copy link
Member

This would be a breaking change and 'name' probably isn't the best label name. But yeah I feel like this should probably be a label, not change to the metrics name. @SuperQ @pgier wdyt?

@brian-brazil
Copy link
Contributor

I happened across this independently, looking at the docs (https://www.kernel.org/doc/html/latest/power/powercap/powercap.html) "power zone" is the name so some variant of that as the label name could work.

@SuperQ
Copy link
Member

SuperQ commented Aug 20, 2020

There's also an issue in the procfs library that needs to be fix: prometheus/procfs#320

@uniemimu
Copy link
Contributor

uniemimu commented Aug 20, 2020 via email

@SuperQ
Copy link
Member

SuperQ commented Jan 24, 2021

Closing in favor of using the procfs fix.

@SuperQ SuperQ closed this Jan 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants