util.GetHostname should not panic, instead it should return an error#294
util.GetHostname should not panic, instead it should return an error#294
Conversation
olivielpeau
left a comment
olivielpeau
left a comment
There was a problem hiding this comment.
panicking only in StartAgent makes sense to me
| @@ -99,7 +99,8 @@ func start(cmd *cobra.Command, args []string) error { | |||
| f.Start() | |||
|
|
|||
| // FIXME: the aggregator should probably be initialized with the resolved hostname instead | |||
There was a problem hiding this comment.
unrelated: can you remove this FIXME (it's been fixed for a long time)
masci
left a comment
masci
left a comment
There was a problem hiding this comment.
Much better like this, 👍 overall but I think we should be explicit in handling the error now that we have one.
| func getHostname(w http.ResponseWriter, r *http.Request) { | ||
| w.Header().Set("Content-Type", "application/json") | ||
| hname := util.GetHostname() | ||
| hname, _ := util.GetHostname() |
There was a problem hiding this comment.
We should not rely on the value returned by GetHostname at this point, better catching the error and set a default value for hname, like:
hname, err := util.GetHostname()
if err != nil {
log.warning(err) // or something like this
hname = ""
}
| hostname := util.GetHostname() | ||
| hostname, err := util.GetHostname() | ||
| if err != nil { | ||
| panic(err) |
| // FIXME: the aggregator should probably be initialized with the resolved hostname instead | ||
| aggregatorInstance := aggregator.InitAggregator(f, util.GetHostname()) | ||
| hname, _ := util.GetHostname() | ||
| aggregatorInstance := aggregator.InitAggregator(f, hname) |
There was a problem hiding this comment.
same as above, in case of error it's better to explicitly set hname before passing it to InitAggregator
There was a problem hiding this comment.
it could make sense to panic here if no hostname is found (i.e. have the same behavior for dogstatsd as the full agent)
| //export GetHostname | ||
| func GetHostname(self *C.PyObject, args *C.PyObject) *C.PyObject { | ||
| hostname := util.GetHostname() | ||
| hostname, _ := util.GetHostname() |
There was a problem hiding this comment.
same as above, let's explicitly set an empty string
|
@masci I explicitly set the empty string in all of those, that's better than what I was doing (assuming that it would exist if it made it past the initial panic gate |
|
let's 🚢 this! |
Bumps [github.com/aws/aws-sdk-go-v2/config](https://github.com/aws/aws-sdk-go-v2) from 1.32.5 to 1.32.6. - [Release notes](https://github.com/aws/aws-sdk-go-v2/releases) - [Changelog](https://github.com/aws/aws-sdk-go-v2/blob/main/changelog-template.json) - [Commits](aws/aws-sdk-go-v2@v1.32.5...v1.32.6) --- updated-dependencies: - dependency-name: github.com/aws/aws-sdk-go-v2/config dependency-version: 1.32.6 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
3 participants
What does this PR do?
util.GetHostname panics right now if it cannot get the hostname. Instead it should return an error that is handled as is appropriate.
Motivation
We need a hostname for the Flare command. The Flare command should work even if it is not possible to get a hostname. This will ensure that the flare command does not break when it attempts to get the hostname.
Additional Notes
I am not certain if I handled it properly everywhere. For example, in the aggregator I ignored the hostname failure, as I thought that was the appropriate course of action. If there had been a failure to grab it, it would have been handled much earlier.