allow user to manually set desired hostname #260
Conversation
|
I'm not fully sure - but for "hostname" there is no concept of "short" vs "full". Hostname just returns the machines name. If that name happens to be set to the short name then that will be returned - because that's the machine's name. Maybe you just need to fix the docker containers hostname in /etc/sysconfig/network or wherever it is set? I remember there being a debate on whether the machine name should be the short or full name. In general I tend to set it to the full name. Found a snippet here but it seems to be completely down to preference, and different vendors recommend different things. |
|
well, in the end this is about consistency and simplicity... what i think would be valuable, for the docker case, is all the times one sets a conatiner name to be (via (docker run's) -h) foo.bar then have logstash-forwarder to relay its logs having 'foo.bar' as their source, regardless of the inner container distro being RH or debian derivative. (and yes, not having to mess anything inside the container at runtime would be an huge plus, imho) as nowadays that does not happen. IMHO that can be seen as a bug as i (lack of imagination, certainly) can't see too many use cases where (in the concrete case of relaying logs) one wouldn't want to have the fqdn passed to the upper level. just my .0002€ :) |
|
Hi @AntonioMeireles, (Just fyi & granted we have the same pattern in current code base :) but using _ for the error return is strictly not acceptable going forward. If there is an error, we want to know about it. |
AntonioMeireles
force-pushed
the
master
branch
from
5e341a0 to
fcf431c
Compare
|
@alphazero: point taken :-). patch(es) updated. |
|
for further context this would be an workaround for docker's #7851. |
|
Would it be right to "workaround" things in a product though? I don't know. Might confuse an admin if other utilities don't workaround it, potentially pointing them in wrong direction. Just a thought. |
|
Summary of my two cents: Maybe an option though to enable it would be useful? That's be best of both :) |
|
Hmm.. Thinking about this, if some users can't trust os.Hostname(), then maybe we can provide a way for those users to specify a value to use explicitly; a flag or a config setting. Thoughts? I think a config setting would be preferable to "trying harder with code" because ultimately there will still be edge cases and allowing users to specify 'this is what the hostname should be reported as' would let humans correct for computers doing things incorrectly. |
|
+1 on that option! |
…ing conversation
|
yes, having an option to just set the hostname would do too :-) . above, for comments, is a very quick patch that would allow just that. |
AntonioMeireles
changed the title
| @@ -64,7 +66,8 @@ var infolog *log.Logger | |||
|
|
|||
| func init() { | |||
| flag.StringVar(&options.configFile, "config", options.configFile, "path to logstash-forwarder configuration file") | |||
|
|
|||
| flag.StringVar(&options.hostname, "hostname", options.hostname, "hostname we want to advertise as") | |||
| flag.StringVar(&options.hostname, "h", options.hostname, "hostname we want to advertise as") | |||
There was a problem hiding this comment.
just -hostname is OK with me, the '-h' flag would override the current '-h' behavior which is to get the usage/help output.
|
reviewed, LGTM once the comments I made are resolved. :) Thanks for doing this! |
|
@AntonioMeireles Can you do step 2 of the contributing guide? https://github.com/elasticsearch/logstash/blob/master/CONTRIBUTING.md#contribution-steps |
|
Is it better in the config file? Problem here is people may need to change init scripts if they already packaged etc |
|
I've been trending towards putting all things in config lately; splitting configuration across two interfaces (some as cli flags, some as config file) is confusing and possibly irritating. |
|
If it counts - I vote for having it in config file |
|
@jordansissel regarding step 2 of the CLA i've already done it, at Sep 1st. |
|
@jordansissel et all, code tweaked per your comments. regarding the comments about the config file all i can say is that things are not mutually exclusive. unlike my 1st approach this one is fully backwards compatible, i.e. if one does not set -hostname app behaviour is exactly as ever been, for good or worst, (so for people that was never hit by this there is not any need to change anything, anywhere). I can see use cases where it makes sense having the hostname set in the config file too (imagining a volume within a logstash-forwarder container to where multiple containers write their logs (e.g. instead of having logstash-forwarder installed in every container), but IMHO the general case is that the hostname is a global immutable var and in that sense set unconditionally globally by default. (anyway, open to code that feature, either in addition or instead of present patch suggestion :-) ) |
|
Edited to clarify I know there are a few cmd line options already that should really be in config file. IMO all configurable options should be in that file, and command line only contain things required before the config file is available (such as the config file path itself) or things that change the action of the daemon (asking for version, asking for what options available, running to background etc.) Since a cleanup would be needed anyway to move the things like spool size to the config file - there's nothing blocking this getting merged and then it can be shifted to the config later. But if you could shift it to the config that might save some future work. All down to opinion I guess on what should be where - just adding my thoughts :) Jason |
|
@driskell, matter of trade-offs, i'd guess. personally i'd vote to get this merged now, and after go for a general cleanup - dropping dead code, normalizing a bit internal code conventions (specially error handling (done on too many != ways, etc)) and by then i'd do the more structural and invasive mods (like changing config files conventions/features) and after eventually signalling it with a sound version bump. but that would be me, it's not my call :-) |
|
Unfortunately I've been relegated to using beaver for this functionality. I would oh so love to see it included in logstash-forwarder since beaver has a slew of other issues. Having the ability to specify a hostname via command-line AND via config file would be ideal since we sometimes test/develop in a single vm/instance via command-line but we are also puppetizing the configuration. 👍 |
|
+1 This is really needed, right now my servers are named just as the first part of the hostname And yes getting this in the config file instead of a config line would really improove startup script etc. |
|
@kiranos 👍 My thoughts exactly. |
|
I tried this patch with the current master and got _grokparsefailure from exported apache2 logs. I reverted it. |
|
I've done some more testing now, but cant get it to work: For startes here is my hostname (not fqdn) but the name in /etc/hostname Here is it running with modified patch applied But I still get host=go-buildhm in my stdout debug: |
|
@kiranos what's your logstash config? Maybe it's overriding the host field. |
|
it is: |
|
Can you check again? The received date of the event you gave us before the logs for restarting forwarder. Maybe it's an old event. Can you confirm with new events since the change? |
|
Yes on my remote host running logstash-forwarder: And the modified file with --help: I did a new post and its the same: Using the same logstash conf as above this time aswell. |
|
How did you apply the patch? Could you build https://github.com/AntonioMeireles/logstash-forwarder directly if you didn't already? Just to rule out something not patching properly. |
|
I applied it myself, I can try above repo tomorrow, I'll update after I test it, thanks! |
|
I just compiled the fork and its exacly the same: From my understanding, the logstash server should never see anythin with go-buildhm in it as I have switched hostname, so logstash-forwarder must send out the wrong info. @AntonioMeireles I can send out a root password for a testbox for you to see it first hand if you like? if so just let me know. |
|
@kiranos i'll find a way to slot quality time to dig into this before the end of this week. no need (yet, hopefully) for access to a test box. thanks for your patience. |
|
@AntonioMeireles great thanks! I'll test it straight away, I have a test setup ready. Would you consider, adding support for placing it in a config file aswell? |
|
@AntonioMeireles Did you have time to look at this, I've saved my testbox so just let me know if you need it. |
|
@AntonioMeireles would be nice to get this merged before next version (which seems to be around the corner) do you need anything from me? |
|
So many merge commits. I'll probably merge this by hand to avoid those. Git is so terrible :\ |
|
I'll see about merging this soon, but for now setting "host" field per file should be a functional workaround. |
|
I can do a git rebase against your current master tomorrow if that helps sent from my mobile
|
Signed-off-by: António Meireles <antonio.meireles@reformi.st>
|
Following. +1 Wanting to make logstash-forwarder show a specific hostname rather than the FQDM or Before: After: |
|
This feature is very convenient, either command line or config setting, or both. |
|
Is there any way to get this feature merged? There is some real cases where short name is simply not enough, especially if you collect logs from multiple environments where same names can appear (e.g. webapp01.smallcompany.com, webapp01.bigcompany.com), but there is a reason why the hostname should not be FQDN (usually, it's a bad idea, since all program that use FQDN names can also parse this info by gethostbyaddr(3) system call (this usually returns with FQDN, same as |
|
+1. We are using multiple clusters with servers called web1,2,3 and without FQDN they all look the same in Logstash. Please merge this soon, otherwise we will have to move to log courier. |
|
@jordansissel filebeat is allows us to explicitly set the hostname (instead of some magic that try to figure it out - wrongly?) |
Hi, attached patch attempts to fix an issue (specially inside docker containers) where too often logstash-forwarder relays data with the short hostname as source (an alternative approach would be to just actually call hostname -f but here i kept using exclusively golang machinery in order to keep platform portability).