-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Add host metadata processor #5968
Conversation
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.
+1 to this, would it make sense to send this info always by default instead of adding a processor?
|
||
func (p addHostMetadata) Run(event *beat.Event) (*beat.Event, error) { | ||
|
||
info, err := host.Info() |
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.
Do you have any benchmark on this? it would make sense to move it to the constructor and cache the result, we just need to document that a hostname change won't get reflected here
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.
I changed it to be instantiated in the constructor. I'm pretty sure later a request for expiring the info is coming up :-)
I was kind of working on something related over the break. It can (or will be able to) fetch some of the information that you want to add with this processor. I am working on this because I wanted to log (#5946) some of this information and also make this data available for xpack monitoring. |
Big +1 to this. I think a processor makes sense to mirror |
@andrewkroh Very nice. Did not look into your code yet. Did you use a third party lib or implement it yourself? |
+1 on adding |
This will close #3480 |
42d3931
to
485145d
Compare
"hostname": p.info.Hostname, | ||
"id": p.info.UniqueID, | ||
"os": common.MapStr{ | ||
"architecture": p.info.Architecture, |
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.
@andrewkroh Seeing that you have Architecture
outside of info
I have second thoughts on puttig it under os
as I agree it more belongs to the host
then the os itself.
Also thinking about putting os
info on the top level.
package add_host_metadata | ||
|
||
import ( | ||
sysinfo "github.com/elastic/go-sysinfo" |
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.
@andrewkroh Did you have to add a -
to the lib name? :-D
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.
With Go you don't need to add a package alias, just import plain old "github.com/elastic/go-sysinfo" and use sysinfo.XYZ
in the code.
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.
It was mainly my IDE that complained :-)
82df182
to
9da8b6a
Compare
@@ -0,0 +1,26 @@ | |||
package add_host_metadata |
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.
don't use an underscore in package name
I'm glad you are trying out go-sysinfo. I'll take a closer look at this next week. I've been sitting on a bunch of updates to go-sysinfo, that I haven't had time to cleanup. I plan on getting the lib into a better place so that this can make 6.3. |
@andrewkroh Good timing. I plan to put some work into this next week again. But also plan to start with a very basic version first which we then can expand on. The current fields are probably already enough for a first version. |
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 looks like it's just about ready. 👍
} | ||
|
||
func (p addHostMetadata) String() string { | ||
return "add_host_metadata=" |
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.
How about add_host_metadata=[]
to indicate an empty config?
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.
done
return nil, err | ||
} | ||
return &addHostMetadata{ | ||
info: h.Info(), |
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.
I think we will want to refresh this data every couple of minutes (image that the user installs an OS update). And given that we are caching this data we might was well cache the common.MapStr
containing it.
I'd probably add a time.Time
value to track when the value was collected. Then refresh the value from Run
after it becomes stale.
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.
I cached the mapstr and set the cache expiry to 5 minutes. We can still make it a config later if needed.
"os": common.MapStr{ | ||
"platform": p.info.OS.Platform, | ||
"version": p.info.OS.Version, | ||
"family": p.info.OS.Family, |
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.
Did you want codename
and build
?
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.
I added it.
Timestamp: time.Now(), | ||
} | ||
p, err := newHostMetadataProcessor(nil) | ||
assert.NoError(t, err) |
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 should expect to receive sysinfo/types.ErrNotImplemented
for any OS other than windows, linux, and darwin.
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.
done
data := common.MapStr{ | ||
"host": common.MapStr{ | ||
"hostname": p.info.Hostname, | ||
"id": p.info.UniqueID, |
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.
info.UniqueID
is optional (as signified by ,omitempty
in the json tag). You should add handling for this. Older versions of Linux don't have /etc/machine-id
.
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.
done
9da8b6a
to
ec4e196
Compare
@@ -0,0 +1,80 @@ | |||
package add_host_metadata |
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.
don't use an underscore in package name
I think we no longer need to keep |
|
b3a7396
to
7fbe357
Compare
Here is a separate PR that changes the dependency. We can merge the other one first and rebase on top of it: #6548 |
7fbe357
to
08123ea
Compare
This adds a processor to add some information about the host machine to each event very similar to what we have for the cloud metadata processor. The idea is to make it possible to allow better filtering based on additional information about the host if needed. This information partially overlaps with what we have in `beat.hostname` but adds additional fields. Other fields like the IP address(es) could be also added here.
08123ea
to
107709c
Compare
This PR currently fails because of a crosscompile issue with darwin 32 bit: elastic/go-sysinfo#3 |
I believe elastic/go-sysinfo#4 should fix the cross-compile issue seen here. The sysinfo code was lacking cgo tags to prevent the darwin provider from being included when cross-compiling on Linux (without a C x-compiler). |
Is the host's FQDN included in the output (eg "hostname -f" output)? If not should/can it be? |
So far the code use to create the hostname is @andrewkroh WDYT on where this belongs? Should we have this in go-sysinfo? |
@ruflin How do I "add a feature request" please? |
@TimWardOrigami Just open a new Github issue in this repo, explain quickly on what it is a about and what it is needed for. I will label it then accordingly. |
#6741
Tim Ward
From: Nicolas Ruflin <notifications@github.com>
Sent: 03 April 2018 13:56
To: elastic/beats <beats@noreply.github.com>
Cc: Tim Ward <tim.ward@origamienergy.com>; Mention <mention@noreply.github.com>
Subject: Re: [elastic/beats] Add host metadata processor (#5968)
@TimWardOrigami<https://github.com/TimWardOrigami> Just open a new Github issue in this repo, explain quickly on what it is a about and what it is needed for. I will label it then accordingly.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#5968 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AkHS-OuiKQPjRRIHDG_5JmkH3flZSpIRks5tk3FvgaJpZM4RO2cH>.
The contents of this email and any attachment are confidential to the intended recipient(s). If you are not an intended recipient: (i) do not use, disclose, distribute, copy or publish this email or its contents; (ii) please contact the sender immediately; and (iii) delete this email. Origami Energy Limited (company number 8619644); Origami Storage Limited (company number 10436515) and OSSPV001 Limited (company number 10933403), each registered in England and each with a registered office at: Ashcombe Court, Woolsack Way, Godalming, GU7 1LQ.
|
This adds a processor to add some information about the host machine to each event very similar to what we have for the cloud metadata processor. The idea is to make it possible to allow better filtering based on additional information about the host if needed. This information partially overlaps with what we have in
beat.hostname
but adds additional fields. Other fields like the IP address(es) could be also added here.