Combine libbeat/beat and libbeat/publisher/beat package + move all beat instance handling to libbeat/cmd package by urso · Pull Request #4821 · elastic/beats

urso · 2017-08-02T21:05:57Z

This PR moves some types and functionality to libbeat/beat and libbeat/cmd. The libbeat/beat package only defines some common/important types and interfaces to be used by actual beats, without providing real implementations. With these changes, the publisher/libbeat/beat finally could be fully merged into libbeat/beat For example beat.Client, beat.Pipeline, beat.Info, .... Setting up a beats instance is moved to libbeat/cmd/instance package (due to lack of a better name), as the different commands implemented by libbeat/cmd do rely on a shared view of an actual configured instance. The move was also required to resolve quite a number of circular imports, as the instance package basically needs to import all other libbeat packages (directly/indirectly) in order to setup a working beat.
Setting up and running a beat instance is still somewhat complicated. Did keep me quite busy in hunting chicken and eggs. Let's see if we can refine/cleanup this somewhat more in later PRs.

Move actual beat instance with run loop and configurations to libbeat/cmd/instance
Move all definitions from libbeat/publisher/beat to libbeat/beat
As libbeat/beat defines shared type/interfaces between libbeat and the actual beat => reduce public interface (exports) in libbeat/beat to fields/types really being used/shared
Remove packetbeat -device in favor of packetbeat device. Removes some clunky hack we used to have for supporting the -device flag.
Move common.BeatInfo to beat.Info

- only have commonly shared structs/interfaces in libbeat/beat - move libbeat/beat functionality to libbeat/cmd/instance package - update imports/dependencies - start hiding some exported symbols

- move beat.Client, beat.Event, beat.Pipeline to libbeat/beat - move some shared config objects from beat.BeatConfig to private cmd/instance package where easily possible (fix import cycles)

ruflin · 2017-08-03T07:19:10Z

filebeat/prospector/udp/harvester.go

 	"github.com/elastic/beats/libbeat/logp"
-	"github.com/elastic/beats/libbeat/publisher/beat"
+
+	"github.com/elastic/beats/filebeat/harvester"


do you started to separate filebeat from libbeat imports on purpose?

yes :)

I did find it weird for filebeat having import order filebeat>libbeat and for packetbeat it being the other way around.

@andrewkroh WDYT? Can we automate that? :-D

Yes, but not with the standard goimports. It would take a customization to goimports, which isn't a bad idea, it would just take a little time.

ruflin · 2017-08-03T07:21:10Z

libbeat/cmd/instance/beat_test.go

 // +build !integration

-package beat
+package instance


If we have an instance package, it would kind of make sense to have this under the beat package?

I put it in libbeat/cmd, as the instance package is used by almost all commands defined in libbeat/cmd. This way all code required or requiring a beat instance is combined in libbeat/cmd.

Some more cleanup on instance or cmd package would be nice. But this was nothing I wanted to touch right now.

Yeah, also see some other cleanups coming but this is big enough already :-)

ruflin · 2017-08-03T07:23:01Z

packetbeat/beater/devices.go

@@ -1,39 +0,0 @@
-package beater


I like this change but not sure how it is related to the rest of the changes?

it's mostly for sanity, as I wanted the libbeat/cmd/instance module to be used only internally. The -devices flag always has been a pretty ugly hack, generating some dependency between the beat and some weird global access to libbeat internals.

ruflin · 2017-08-03T07:27:06Z

In general I like the changes you made. I'm not a big fan of the cmd package but I don't have a better suggestion at the moment.

It seems some other cleanups / improvements sneaked into this PR like Winlogbeat Settings or packetbeat device removal. As this is a major change in how beats are setup and will break all community beats I would like to see the unrelated changes in other PR's if possible. This will make it much easier later to follow which things belonged together. For example for the device removal the changelog will link to the PR that only did this.

andrewkroh

LGTM. I left some comments about the pre-existing godocs.

andrewkroh · 2017-08-03T14:00:57Z

libbeat/beat/beat.go

 )

+// Creator initializes and configures a new Beater instance used to execute
+// the beat its run-loop.


s/beat its/beat's/

andrewkroh · 2017-08-03T14:03:09Z

libbeat/beat/beat.go

 	SetupMLCallback SetupMLCallback // setup callback for ML job configs
 	InSetupCmd      bool            // this is set to true when the `setup` command is called
+
+	// XXX: remove Config from public interface


It would be nice to include in the comment what's blocking this from happening or when it can be done.

andrewkroh · 2017-08-03T14:03:38Z

libbeat/beat/beat.go

+	// XXX: remove Config from public interface
+	Config *BeatConfig // Common Beat configuration data.
+
+	BeatConfig *common.Config // The beats it's own configuration section


s/beats it's/beat's/

andrewkroh · 2017-08-03T14:04:54Z

libbeat/beat/info.go

 // BeatInfo stores a beats instance meta data.
-type BeatInfo struct {
+type Info struct {
 	Beat     string    // The actual beat its name


s/beat its/beat's/

andrewkroh · 2017-08-03T14:11:50Z

winlogbeat/beater/winlogbeat.go

@@ -49,21 +48,19 @@ func New(b *beat.Beat, _ *common.Config) (beat.Beater, error) {
 	// XXX: winlogbeat validates top-level config -> ignore beater config and


This comment can be removed now that I removed the validation code.

andrewkroh · 2017-08-03T14:14:15Z

winlogbeat/config/config.go

@@ -23,26 +23,12 @@ type Validator interface {
 	Validate() error


Can this interface be removed too? I think it predates ucfg.

ruflin · 2017-08-08T20:24:11Z

winlogbeat/eventlog/eventlogging.go

-	IgnoreOlder      time.Duration          `config:"ignore_older"`
-	ReadBufferSize   uint                   `config:"read_buffer_size"   validate:"min=1"`
-	FormatBufferSize uint                   `config:"format_buffer_size" validate:"min=1"`
-	Raw              map[string]interface{} `config:",inline"`


Not sure what raw was doing but want to make sure it didn't disappear by accident.

I guess it used to be for additional 'validation' on allowed settings. But it's not used anymore in any place, so I removed it.

urso added the in progress Pull request is currently in progress. label Aug 2, 2017

urso added 7 commits August 2, 2017 23:13

Move most functionality from libbeat/beat to libbeat/cmd package

5e60bba

- only have commonly shared structs/interfaces in libbeat/beat - move libbeat/beat functionality to libbeat/cmd/instance package - update imports/dependencies - start hiding some exported symbols

Add devices command to packetbeat

754f586

removes devices flag from packetbeat

4a7e110

Remove FlagHandler support

dc55186

Move definitions from libbeat/publisher/beat to libbeat/beat

bea1e8c

- move beat.Client, beat.Event, beat.Pipeline to libbeat/beat - move some shared config objects from beat.BeatConfig to private cmd/instance package where easily possible (fix import cycles)

move some more config fields to cmd/instance

66a065d

Move common.BeatInfo to beat.Info

ebcf8dd

urso force-pushed the pipeline/cleanup-pkg branch from c32b0e7 to ebcf8dd Compare August 2, 2017 21:13

urso added the discuss Issue needs further discussion. label Aug 2, 2017

Fix conflict after rebase

13709c4

ruflin reviewed Aug 3, 2017

View reviewed changes

andrewkroh approved these changes Aug 3, 2017

View reviewed changes

urso added 2 commits August 8, 2017 16:38

Remove packetbeat -devices flag in cobra setup

88a29be

review fixes

ce1229e

urso added refactoring review and removed in progress Pull request is currently in progress. labels Aug 8, 2017

urso changed the title ~~[WIP] Combine libbeat/beat and libbeat/publisher/beat package + move all beat instance handling to libbeat/cmd package~~ Combine libbeat/beat and libbeat/publisher/beat package + move all beat instance handling to libbeat/cmd package Aug 8, 2017

urso added 3 commits August 8, 2017 17:45

Update generator

d790593

metricbeat generator

ee27656

fix panic on AddGoFlag

11c9e7d

ruflin reviewed Aug 8, 2017

View reviewed changes

ruflin approved these changes Aug 8, 2017

View reviewed changes

kvch merged commit 0cacffe into elastic:master Aug 9, 2017

urso mentioned this pull request Aug 20, 2017

Publisher Pipeline #4598

Closed

22 tasks

urso added the needs_backport PR is waiting to be backported to other branches. label Aug 24, 2017

urso mentioned this pull request Aug 24, 2017

Cherry-pick #4821 to 6.0: Combine libbeat/beat and libbeat/publisher/beat package + move all beat instance handling to libbeat/cmd package #5004

Merged

urso removed the needs_backport PR is waiting to be backported to other branches. label Aug 24, 2017

urso deleted the pipeline/cleanup-pkg branch February 19, 2019 18:34

		@@ -49,21 +48,19 @@ func New(b beat.Beat, _ common.Config) (beat.Beater, error) {
		// XXX: winlogbeat validates top-level config -> ignore beater config and

		@@ -23,26 +23,12 @@ type Validator interface {
		Validate() error

Conversation

urso commented Aug 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ruflin commented Aug 3, 2017

Uh oh!

andrewkroh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewkroh Aug 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

urso commented Aug 2, 2017 •

edited

Loading

andrewkroh Aug 3, 2017 •

edited

Loading