Skip to content
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

Merged
merged 1 commit into from Jun 2, 2020

Conversation

mdlayher
Copy link
Contributor

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".

@mdlayher mdlayher requested a review from beorn7 May 28, 2020 15:06
Copy link
Member

@beorn7 beorn7 left a 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.

prometheus/testutil/promlint/promlint.go Outdated Show resolved Hide resolved
prometheus/testutil/promlint/promlint.go Outdated Show resolved Hide resolved
Signed-off-by: Matt Layher <mdlayher@gmail.com>
@mdlayher
Copy link
Contributor Author

Done and done, thanks!

@brian-brazil
Copy link
Contributor

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.

@beorn7
Copy link
Member

beorn7 commented May 28, 2020

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.

@brian-brazil
Copy link
Contributor

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.

@beorn7
Copy link
Member

beorn7 commented May 28, 2020

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.

I completely disagree with this statement, but I don't want to sink an enormous amount of time into this fairly low-significance discussion.

Both the Prometheus docs and the Openmetrics draft only allow for Celsius, and both also happen to explicitly mention Kelvin.

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.

@brian-brazil
Copy link
Contributor

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.

beorn7 added a commit to prometheus/docs that referenced this pull request May 28, 2020
Triggered by prometheus/client_golang#761

Signed-off-by: beorn7 <beorn@grafana.com>
@beorn7
Copy link
Member

beorn7 commented May 28, 2020

for the same reasons we complain about minutes, bits, and Fahrenheit.

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.

beorn7 added a commit to prometheus/docs that referenced this pull request Jun 2, 2020
Triggered by prometheus/client_golang#761

Signed-off-by: beorn7 <beorn@grafana.com>
@beorn7
Copy link
Member

beorn7 commented Jun 2, 2020

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.)

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.

None yet

4 participants