jmcgrath207 / k8s-ephemeral-storage-metrics

Prometheus ephemeral storage metrics exporter
https://jmcgrath207.github.io/k8s-ephemeral-storage-metrics/
MIT License
85 stars 35 forks source link

set percentage node used instead of available #95

Closed miminar closed 2 months ago

miminar commented 2 months ago

First of all, thanks for this exporter. We find it very useful.

While implementing alerting rules based on the metrics provided, I found out an inconsistency between the metric's description Percentage of ephemeral storage used on a node and the actual value returned, which is Percentage of ephemeral storage available on a node. Either the description or the calculation is wrong.

This PR attempts to fix the latter because it makes both ephemeral_storage_{node,container_limit}_percentage metrics consistent with setValue = (usedBytes / c.limit) * 100.0 (the value calculated for container if its limit is set).

What do you think?

jmcgrath207 commented 2 months ago

Hi @miminar. Thank you, I am always glad to hear from happy users!

I made this test script and it produces the same value. So I need some more context on what I am doing wrong.

I also had some trouble finding Percentage of ephemeral storage available on a node in the library, could you link that code here?

Thanks!

package main

import (
    "fmt"
    "math"
)

var (
    availableBytes float64
    capacityBytes  float64
)

func main() {

    availableBytes = 50.0
    capacityBytes = 100.0

    setValue := (availableBytes / capacityBytes) * 100.0

    a := fmt.Sprintf("Old value is: %f\n", setValue)
    print(a)

    setValue = math.Max(capacityBytes-availableBytes, 0.) * 100.0 / capacityBytes

    a = fmt.Sprintf("New value is: %f\n", setValue)
    print(a)
}

Output

Old value is: 50.000000
New value is: 50.000000
miminar commented 2 months ago

Hi @miminar. Thank you, I am always glad to hear from happy users! availableBytes = 50.0 capacityBytes = 100.0

Nice, that's the only case where they are equal :-). Try with capacityBytes=450..

I also had some trouble finding Percentage of ephemeral storage available on a node in the library, could you link that code here?

That's the thing. There is just "Percentage of ephemeral storage used on a node", but the returned value is the percentage of the available ephemeral storage, not used.

This PR changes the calculation to return the percentage used. But if you like, I could change it to just update the metric's help to "Percentage of ephemeral storage available on a node". However, in that case, this calculation would have to be changed as well.

jmcgrath207 commented 2 months ago

@miminar Ah, I see the error of my ways. I agree; let's fix the calculation. Thank you for going into detail!

I have approved this for e2e testing.

lozbrown commented 2 months ago

When I told you this months ago you rejected the issue :-(

jmcgrath207 commented 2 months ago

@lozbrown That is correct, and mistakes were made. However, there wasn't clarity around why I needed it, so my opinion changed here. I hope you don't take this personally, and I try my best to hear every case.

https://github.com/jmcgrath207/k8s-ephemeral-storage-metrics/issues/41#issuecomment-1854018515

That said, you raised some good feature ideas in that issue. We've added them, and I thank you for that. https://github.com/jmcgrath207/k8s-ephemeral-storage-metrics/pull/59

My suggestion for raising issues in the future, @lozbrown, is to focus on one topic and keep it as concise as possible. This will make it easier for the maintainer to focus on what needs to happen.

jmcgrath207 commented 2 months ago

@miminar Thank your contribution, the e2e test passed. I will release this later tonight.

lozbrown commented 2 months ago

I was only kidding, definitely no offence taken and I hope you haven't taken any either.

I'm glad this change has been made and I'll look to upgrade again soon and remove my workarounds.

This is a really useful tool, thanks again for all the work on this

jmcgrath207 commented 2 months ago

@lozbrown Awesome, Glad to hear. Thanks for understanding.

miminar commented 2 months ago

Thank you for review and merge! Would you be interested in another contribution adding an optional PrometheusRule à la KubePersistentVolumeFillingUp to chart templates?

jmcgrath207 commented 2 months ago

@miminar Sounds good to me!

jmcgrath207 commented 2 months ago

@miminar @lozbrown

Just release this PR in 1.10.2.