Make hostname a stored variable so it is not retrieved for each metri… #1215
Conversation
| "error": err.Error(), | ||
| }).Error("Unable to determine hostname") | ||
| } | ||
| hostnameReader = &hostnameReaderType{hostname: h} |
There was a problem hiding this comment.
I think this is going to result in only calling once per run of snapd since init will only be run at that time, instead of the intended once per collect call.
There was a problem hiding this comment.
What is the argument against using a simple hostname global set once when snapd starts?
There was a problem hiding this comment.
Just that it was the intention of the PR. Is there a case where the hostname would change more regularly than snapd would be restarting?
There was a problem hiding this comment.
Not that I am aware of.
@jtlisi I would recommend simplifying this down to a global var for hostname. No need for the hostnamer interface IMO.
There was a problem hiding this comment.
Gotcha, I'll change it accordingly
There was a problem hiding this comment.
Does this means a hostname change requires a restart of snapd?
There was a problem hiding this comment.
@lynxbat I believe so, I can move the hostname out into the metrics.go file and have it run every collection. This would also have to be done at control/plugin_manager.go
There was a problem hiding this comment.
I think requiring a restart is nasty and we should avoid that one.
Maybe having a TTL that is pretty long (60min)? And have it invalidate and refresh past that. You can just have the accessor function hide this behavior. This way we prevent too much churn (imagine a 10ms interval) but prevent hard locked restart.
We would just need to add comments to the README to call out the 60min refresh. We could postpone exposing that as config until someone asks for it.
| } | ||
|
|
||
| type hostnamer interface { | ||
| Hostname() (name string, err error) | ||
| Hostname() (name string) |
There was a problem hiding this comment.
Do we still need the hostnamer interface?
There was a problem hiding this comment.
I believe not, I'll take that out as well.
|
Hey does anyone know why the legacy build is failing on 1.5. I didn't touch the segment of code that seems to be failing. Edit. Nevermind seems all the checks have passed |
|
@jtlisi: It was failing because there is a flakey tests that hasn't been tracked down yet. Was definitely unrelated to your change. I restarted the test when I saw your comment. Sorry for not commenting at the time. |
|
@jtlisi: I think this looks good. Can you rebase it down to 1 commit that starts the message off as "Fixes #1214:...." so that github will close that issue when this is merged? The only thing I think could be changed is what happens when there is an error with the hostname. In addition to logging it, would it make sense to change the returned hostname to something like "Unknown"? |
…ing tags, unknown hosts will be labeled not_found
|
LGTM |
Fixes #
Summary of changes:
Testing done:
@intelsdi-x/snap-maintainers
…c when tags are applied