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

internal/util: Read(U)intFromFile: Return file name in errors #326

Closed
wants to merge 1 commit into from

Conversation

hoffie
Copy link

@hoffie hoffie commented Sep 13, 2020

This PR improves error messages by adding the affected file name. This helps downstream users to locate the source of errors which is useful for e.g. prometheus/node_exporter#1710.

The original error is wrapped as some consumers perform checks on it. The consumers within procfs have been updated to Unwrap the error if necessary.

@pgier @discordianfish What do you think?
cc @SuperQ who was involved with the referenced node_exporter/cpufreq issue.

This improves error messages by adding the affected file name.
This helps downstream users to locate the source of errors which
is useful for e.g. prometheus/node_exporter#1710.

The original error is wrapped as some consumers perform checks on it.
The consumers within procfs have been updated to Unwrap the error if
necessary.

Signed-off-by: Christian Hoffmann <[email protected]>
@discordianfish
Copy link
Member

Hrm not sure about the best practices around this but I would assume that if the caller has the filename, there is no need to add it in the error, no?

@hoffie
Copy link
Author

hoffie commented Sep 26, 2020

Hrm not sure about the best practices around this but I would assume that if the caller has the filename, there is no need to add it in the error, no?

Not sure about best practices either, but it was deemed useful to have the file name somewhere in the user-visible error message in the mentioned ticket regarding the cpufreq collector.

We could also add the information in the caller (e.g. https://github.com/prometheus/procfs/blob/master/sysfs/system_cpu.go#L197), but a quick grep shows that would mean that potentially more places would have to be modified with duplicated code (~14).

If you see benefits in doing that instead, I can modify the PR accordingly.

@discordianfish
Copy link
Member

@hoffie Yeah I think we should wrap[1] the error in the caller and add the filename there, even if it's duplication.
But let see what @SuperQ and @pgier thinks?

[1] https://blog.golang.org/go1.13-errors

@discordianfish
Copy link
Member

I think #518 superseeds this, so closing

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.

2 participants