-
Notifications
You must be signed in to change notification settings - Fork 492
Configurable hostname #2439
Configurable hostname #2439
Conversation
bd2efc4 to
96288b3
Compare
| // Data for Hosts (Hypervisors) | ||
| for _, host := range hostSystems { | ||
| name := util.Clean(host.Name) | ||
| name, err := util.GetHostManager().GetNameProcessor().FormatName(host.Name) |
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.
Is this change in behaviour intended? The previous version didn't perform a validity check on the host name. The name is only used in an OpenTSDB tag, which allows more characters than the RFC for host names: http://opentsdb.net/docs/build/html/user_guide/writing/index.html#metrics-and-tags
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.
Yes this was intended and the reason I oddly chose to tick both breaking and non-breaking change in PR message.
Prior to now the Bosun hostname always comes from a call to os.Hostname() and this should always be valid.
It has been possible to override the hostname with SCollector however I can't imagine why anyone would have wanted to specify an invalid (according to IETF rules) hostname and if an invalid name were provided then it could end up causing problems elsewhere which would be much harder to debug than if a clear error was raised if an invalid hostname was provided.
muffix
left a comment
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.
This is loooking great, thanks. 👍
Fixes a bug that was introduced with bosun-monitor#2439 which now requires the initialisation of the HostManager on startup. That was forgotten to add to the tsdbrelay command, causing it to panic on startup. Fixes: 2475
Description
Resolves #2438
Type of change
From the following, please check the options that are relevant.
See linked ticket for details on breaking change
How has this been tested?
Checklist: