Skip to content
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

Move "beat" (ex-"shipper") config settings to be top level #1570

Merged
merged 2 commits into from May 9, 2016

Conversation

@tsg
Copy link
Collaborator

commented May 4, 2016

Part of #1417. Done in a non-BWC way, because I think it's not worth
complicating the code. I'd rather have a script that migrates the
configuration.

@tsg tsg referenced this pull request May 4, 2016
14 of 14 tasks complete
# The tags of the shipper are included in their own field with each
# transaction published. Tags make it easy to group servers by different
# logical properties.
#tags: ["service-X", "web-tier"]

This comment has been minimized.

Copy link
@ruflin

ruflin May 4, 2016

Collaborator

@urso @andrewkroh I remember we discussed once if we should perhaps move these unter a "meta" namespace. Now that we break BC anyways? But not 100% convinced we should do it.

This comment has been minimized.

Copy link
@tsg

tsg May 4, 2016

Author Collaborator

I think we introduced them in 5.0-alpha1, so it wouldn't even be a real BWC change.

This comment has been minimized.

Copy link
@ruflin

ruflin May 4, 2016

Collaborator

In case we also use filebeat.prospector.0.meta.fields it would be. But it makes path even longer.

Side note: How nice is the above that I can write a config example in one line :-)

This comment has been minimized.

Copy link
@tsg

tsg May 4, 2016

Author Collaborator

I see, if we do it everywhere, yes. To me, it's not worth making these config paths longer, especially since we just add "meta", which is everyone's guess what it means :)

This comment has been minimized.

Copy link
@ruflin

ruflin May 4, 2016

Collaborator

Lets just keep it as it is.

# Uncomment the following if you want to ignore transactions created
# by the server on which the shipper is installed. This option is useful
# to remove duplicates if shippers are installed on multiple servers.
#ignore_outgoing: true

This comment has been minimized.

Copy link
@ruflin

ruflin May 4, 2016

Collaborator

We should make sure to remove / refactor this one and the next 2 before beta 1.

This comment has been minimized.

Copy link
@tsg

tsg May 4, 2016

Author Collaborator

Or we could just remove it from the default configuration file? I wouldn't consider it that critical.

This comment has been minimized.

Copy link
@ruflin

ruflin May 4, 2016

Collaborator

+1 on that, but still open an issue to clean it up in the long run.

This comment has been minimized.

Copy link
@tsg

tsg May 4, 2016

Author Collaborator

I'd say it is part of this one: #946

This comment has been minimized.

Copy link
@ruflin

ruflin May 4, 2016

Collaborator

There we go :-)


# Configure local GeoIP database support.
# If no paths are not configured geoip is disabled.
#geoip:

This comment has been minimized.

Copy link
@ruflin

ruflin May 4, 2016

Collaborator

THis should also be refactored to packetbeat.

This comment has been minimized.

Copy link
@tsg

tsg May 4, 2016

Author Collaborator

Since Ingest node can do Geoip, I think easiest would be to deprecate it. I added it to our team meeting agenda, for now.

This comment has been minimized.

Copy link
@urso

urso May 4, 2016

Collaborator

+1 for deprecating it

This comment has been minimized.

Copy link
@ruflin

ruflin May 9, 2016

Collaborator

@tsg That will be done in an other PR?

@@ -113,7 +113,7 @@ type Beat struct {
type BeatConfig struct {
Output map[string]*common.Config
Logging logp.Logging
Shipper publisher.ShipperConfig
Shipper publisher.ShipperConfig `config:",inline"`

This comment has been minimized.

Copy link
@ruflin

ruflin May 4, 2016

Collaborator

Nice solution. Long term we should have all the variables in here to make it more readable.

This comment has been minimized.

Copy link
@urso

urso May 4, 2016

Collaborator

whenever I update some config struct I try to add config tags for naming (even if it feels redundant). Can we add config tags with names here?

This comment has been minimized.

Copy link
@urso

urso May 4, 2016

Collaborator

Another style one: I like to put inlined configs first/last in struct to have them 'stick' out a little.

@@ -5,29 +5,28 @@ mockbeat:


############################# Shipper ############################################

This comment has been minimized.

Copy link
@ruflin

ruflin May 4, 2016

Collaborator

We should rename that to General or Beat

@@ -287,29 +287,32 @@
"system-filesystem": {
"properties": {
"avail": {
"type": "integer"
"type": "long"

This comment has been minimized.

Copy link
@ruflin

ruflin May 4, 2016

Collaborator

How did that get into this PR?

This comment has been minimized.

Copy link
@tsg

tsg May 4, 2016

Author Collaborator

I ran make update.

This comment has been minimized.

Copy link
@ruflin

ruflin May 4, 2016

Collaborator

Probably going to conflict with this one (outcome should be mostly the same): #1572

@ruflin

This comment has been minimized.

Copy link
Collaborator

commented May 4, 2016

LGTM. See comments.

@tsg tsg force-pushed the tsg:config_move_beat_to_toplevel branch from 5aa78bb to 8f74744 May 4, 2016

@tsg

This comment has been minimized.

Copy link
Collaborator Author

commented May 4, 2016

@ruflin i rebased on top of master. There's still a change in the topbeat template, but not mertricbeat.

@urso

This comment has been minimized.

Copy link
Collaborator

commented May 4, 2016

LGTM

@ruflin

This comment has been minimized.

Copy link
Collaborator

commented May 6, 2016

@tsg I suggest to remove geoip as part of a second PR because then we have a specific commit / pr we can reference to for the BC break.

I restarted the appveyor build but the failing could be related to this change ...

Move "beat" (ex-shipper) config settings to be top level
Part of #1417. Done in a non-BWC way, because I think it's not worth
complicating the code. I'd rather have a script that migrates the
configuration.

@tsg tsg force-pushed the tsg:config_move_beat_to_toplevel branch 2 times, most recently from 6aeb2b5 to d1274a2 May 9, 2016

Make winlogbeat accept the new top-level settings
The list has grown quite long, switching to ucfg validation for this would be nice.
Also added explicit key names to config.

@tsg tsg force-pushed the tsg:config_move_beat_to_toplevel branch from d1274a2 to 845cd14 May 9, 2016

@tsg

This comment has been minimized.

Copy link
Collaborator Author

commented May 9, 2016

This should be ready for merging.

@ruflin ruflin merged commit d51884b into elastic:master May 9, 2016

4 checks passed

CLA Commit author is a member of Elasticsearch
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
default Build finished.
Details
medcl added a commit to medcl/beats that referenced this pull request Aug 17, 2016
Move "beat" (ex-"shipper") config settings to be top level (elastic#1570
)

* Move "beat" (ex-shipper) config settings to be top level

Part of elastic#1417. Done in a non-BWC way, because I think it's not worth
complicating the code. I'd rather have a script that migrates the
configuration.

* Make winlogbeat accept the new top-level settings

The list has grown quite long, switching to ucfg validation for this would be nice.
Also added explicit key names to config.

@tsg tsg deleted the tsg:config_move_beat_to_toplevel branch Aug 25, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.