-
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 support to configure monitoring output #3517
Conversation
0455316
to
9c25940
Compare
libbeat/beat/beat.go
Outdated
Name string // Beat name. | ||
Version string // Beat version number. Defaults to the libbeat version when an implementation does not set a version. | ||
UUID uuid.UUID // ID assigned to a Beat instance. | ||
Info publisher.BeatInfo // beat 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.
I can also make use of this in #3527 Perhaps we should put it into a separate package to prevent circular imports.
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.
moved to common.BeatInfo
@@ -4,6 +4,7 @@ import "errors" | |||
|
|||
type Mode uint8 | |||
|
|||
//go:generate stringer -type=Mode |
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.
We should make sure that make update
also executes the generate commands.
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.
oh, it's a leftover from debugging. There are 2 problems with stringer though:
- you either need to have it installed or we have to vendor it and use
go run
- package loading in stringer requires you to have done
go install
on dependent packages (unless you have build a binary already). Otherwisego generate
will fail (See this ticket).
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.
Conclusion on this is that we should not add it to make update
, correct?
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.
yeah, I wouldn't add it to make update
. I have had the build broken when just doing unit tests without having build the full project before go generate
. Maybe introduce separate make go-generate
, but this one might fail and developer has to 'fix' it locally. generated files should be committed.
Reporter common.ConfigNamespace `config:",inline"` | ||
} | ||
|
||
type Reporter interface { |
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.
Seems like we start to have this Runner
interface in lots of different places. We should probably start to use a shared interface.
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.
nice thing about interfaces is, if they are assignable (same definition or sub-type relation), they can be used interchangeable. I'm aware of us having a mix of this interface.
Besides similar definition, giving it a name fitting the packages usage is sometimes nice to have. Plus, the Reporter interface might change. I'm not too much a fan of the 'Start(), Stop()` interfaces, as this clearly mandates/leaks usage of go-routines and very often channels for communication... I think in 'Reporter' we can get rid of 'Start' :)
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.
just realized I don't need Start()
;)
libbeat/publisher/publish.go
Outdated
@@ -87,6 +87,14 @@ type BeatPublisher struct { | |||
numClients uint32 | |||
} | |||
|
|||
type BeatInfo struct { |
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.
Should we put it under common
?
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'd like it to be beat.Info
, not common.BeatInfo
. With beats being kind of a framework, it would be nice if the beat
(or libbeat
) namespace would be the only packages libbeat and the actual beats would meat. Like: beat.Client
being the instance used to publish a beat.Event
. Only problem is initialization and circular dependencies due to having the main in beat
package as well.
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.
Will move it to common
for now. Seems to be the place to prevent potential circular dependencies for now.
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
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.
beat.Info
would be nice, but lets keep such a more "radical" move for later.
ee49df7
to
2d90593
Compare
@urso Seems like you broke the beat generator. Also there you have to pass BeatInfo. |
Info: common.BeatInfo{ | ||
Beat: name, | ||
Version: version, | ||
Name: hostname, |
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 here you want Name: name
, that it's equal to the name
configuration option.
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.
name is the name configured via config file and defaults to hostname. beat.Beat is the beat name like filebeat
, packetbeat
. We set Name
later after having parsed the config file.
|
||
event := outputs.Data{Event: common.MapStr{ | ||
"timestamp": common.Time(ts), | ||
"beat": r.beatMeta, |
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.
Instead of exporting beat.beat
, wouldn't be nicer beat.type
?
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
}, nil | ||
} | ||
|
||
func parseProxyURL(raw string) (*url.URL, error) { |
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.
maybe we should have this in common
to be used later by others?
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.
added todo item
libbeat/outputs/elasticsearch/url.go
Outdated
@@ -114,3 +114,18 @@ func makePath(index string, docType string, id string) (string, error) { | |||
} | |||
return path, nil | |||
} | |||
|
|||
func parseProxyURL(raw string) (*url.URL, error) { |
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.
multiple times defined?
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.
added todo item. The HTTP connection/configurations should be split and generalized for all beats. e.g. heartbeat and metricbeat also have http clients with similar settings.
- Add support to configure `monitoring.elasticsearch`. If `monitoring.elasticsearch` is missing, configuration from `output.elasticsearch` will be used. If both are present, `monitoring.elasticsearch` re-uses missing options from `outputs.elasticsearch`. E.g. this way timeouts and user/password can be configured in `monitoring.elasticsearch`, while re-using `hosts` and other connection settings. - checks if xpack.monitoring.elasticsearch.hosts not configured if output.elasticsearch.hosts is configured - send and publisher loop disconnected from event publisher to publish metric snapshots collected via `libbeat/monitoring` - Introduce common.BeatInfo structure being initialized on startup with common beat meta-data to be used by outputs and monitoring reporter: - beat (e.g. filebeat, metricbeat, ...) - version - name (configure name, otherwise hostname) - hostname - uuid
d68da22
to
57f87b6
Compare
xpack.monitoring.elasticsearch
. Ifxpack.monitoring.elasticsearch
is missing, configuration fromoutput.elasticsearch
will be used. If both are present,monitoring.elasticsearch
re-uses missing options fromoutputs.elasticsearch
. E.g. this way timeouts and user/password can be configured inmonitoring.elasticsearch
, while re-usinghosts
and other connection settings.libbeat/monitoring
will do doc update in followup PR, to not block review for this PR for too long.
Note:
would be nice to have
beat.Info
orbeat.Meta
instead ofpublisher.BeatInfo
, but due to potential circular imports I have had to putthe common structure into the publisher package (already contains shipper
config/meta).