-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
testutil/promlint: allow Kelvin as a base unit for color temperature #761
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. I can also see technical and scientific use cases where you really want K and not C. (I had one back in my university days, time series of temperatures between 2K and 5K, but Prometheus didn't exist back then.)
But I guess a lot of discussion already went into this when the code still lived in prometheus/prometheus.
I'll leave this open for a while to give people a chance to chime in.
Signed-off-by: Matt Layher <mdlayher@gmail.com>
Done and done, thanks! |
For Colour temperature I can see the argument, but the whole point of having one chosen base unit so that we don't end up with a mix of Kelvin and Celsius. Accordingly I don't see a reason to make an exception here, as with any other temperature unit it is easy to convert at the display stage. The wording as proposed here also indicates that Kelvin is in fact preferred, which is not the case. |
I actually prefer Kelvin. But I acknowledge that this is not the case for everyone. I think temperature is special, not only because it involves a number not everyone knows for the top of their head (273.15) but because the conversion involves an addition rather than a multiplication. Common mathematical operations with temperature, like exp(–E/kT) don't make sense with Celsius. I think the preferred unit depends on the context, and I think that's well expressed by this PR. If you want to improve the wording, please make a suggestion. |
I don't think temperate is special, and the same sort of argument could be made that we should allow Fahrenheit and milliseconds and minutes and bits because that's how the implementer thinks of it - which defeats the whole purpose of having exactly one base unit. If someone would prefer Kelvin displayed in Grafana they're free to do so, but that doesn't mean that that is what also goes in the exposition. Both the Prometheus docs and the Openmetrics draft only allow for Celsius, and both also happen to explicitly mention Kelvin. Allowing this would be a significant change in best practices, as it'd basically be abandoning the notion of base units. |
I completely disagree with this statement, but I don't want to sink an enormous amount of time into this fairly low-significance discussion.
Openmetrics isn't finalized, and Prometheus uses the word "should" and "preferred" not "allow". The code comments here make pretty clear that Celsius is more common than Kelvin. Still I was never entirely happy with the statement on prometheus.io, but I could tolerate it as merely a recommendation. Now this PR brought up another case that is less far fetched than the low-temperature experiments I remember from my time in the lab. I think the linter should not complain about Kelvin used as a unit. If the only issue you have here is that the comment in the code (which the user of the linter doesn't even see), then please make a suggestion to improve. If you want the linter to complain about Kelvin, then we have a problem. |
I want the linter to continue to complain about Kelvin, for the same reasons we complain about minutes, bits, and Fahrenheit. |
Triggered by prometheus/client_golang#761 Signed-off-by: beorn7 <beorn@grafana.com>
I have explained why the reasons are different for Kelvin. I guess we have sufficiently discussed this fairly lightweight issue. Time to call a vote. |
Triggered by prometheus/client_golang#761 Signed-off-by: beorn7 <beorn@grafana.com>
By voting, we passed the following proposal (11 for, 1 against): “Allow Kelvin as a base unit in certain cases and update our documented recommendation and the linter code accordingly.” I therefore assume that there are no disagreements left no this PR. (Despite prompting twice, no suggestions to change the code comments were made.) |
Signed-off-by: Matt Layher mdlayher@gmail.com
I'm exporting metrics from my new Elgato Key Light and the device exposes the color temperature of its light in Kelvin, e.g. 4200K. I don't think this measurement would make any sense in Celsius and propose allowing Kelvin as a "base unit", but still recommending that "kelvins" be changed to "kelvin".