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 one level up the sections in the configuration file #1544

Merged
merged 6 commits into from
May 12, 2016

Conversation

monicasarbu
Copy link
Contributor

@monicasarbu monicasarbu commented May 2, 2016

Based on the changed done in #1490, I am proposing the following structure of the configuration file where I move all the sections one level up all.

Implements part of #1417.

The changes should affect the following configuration files:

  • libbeat.yml
  • packetbeat.yml
  • topbeat.yml
  • filebeat.yml
  • winlogbeat.yml

@monicasarbu monicasarbu added in progress Pull request is currently in progress. :Testing and removed in progress Pull request is currently in progress. :Testing labels May 2, 2016
# Configure the ports where to listen for NFS traffic. You can disable
# the NFS protocol by commenting out the list of ports.
ports: [2049]
packetbeat.protocols.icmp:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't the "move one level up" already done with packetbeat.protocols?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved them one more level up. I think it's easier to read the configuration file like this. I think it's similar with what ES has now.

@ruflin
Copy link
Contributor

ruflin commented May 2, 2016

@monicasarbu I think that is in general a good change. What I'm not sure about is our "logic"? When do we exactly use the dot nation, and when exactly sub points? Would be good to have a common logic that we can always apply.

@monicasarbu monicasarbu added the discuss Issue needs further discussion. label May 2, 2016
@monicasarbu monicasarbu force-pushed the move_one_level_up_config branch 2 times, most recently from e7c352e to e5b2cd0 Compare May 3, 2016 09:24
@monicasarbu
Copy link
Contributor Author

@ruflin @tsg @urso @andrewkroh I finished changing all the config files. Please let me know what you think about the new structure.

@monicasarbu monicasarbu added review and removed in progress Pull request is currently in progress. labels May 3, 2016
cpu_per_core: false

# per system statistics, by default is true
stats.system: true
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for topbeat I think it makes sense to have a fully flat config:

topbeat.period: ...

topbeat.procs: 

topbeat.stats.system: ...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or the follwoing would als be fine from my perpsective:

topbeat.period
topbeat.procs
topbeat.stats:
  system: ...
  process: ...

@urso
Copy link

urso commented May 3, 2016

I like the new structure, but change looks very minimal. I don't think this prevents people from messing up filebeat prospector config.

packetbeat, filebeat look good. For topbeat/winlogbeat i'd rather flatten more.


######################### Filebeat general configuration #############################

filebeat:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I quite like the idea of separating general config and prospectors. Should we even comment out "filebeat" here as all sub config options are commented out?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, good spot

@ruflin
Copy link
Contributor

ruflin commented May 4, 2016

Changes look good to me. See comments above. In a second step we should verify that all config options which actually exist are also in the config file. So in a third step we can potentially create a minimized option.

@ruflin
Copy link
Contributor

ruflin commented May 4, 2016

BTW: One idea to create the minimized option of the config: Remove all fields which are commented out in the config could work.

@ruflin
Copy link
Contributor

ruflin commented May 4, 2016

@monicasarbu It would be good to also update the config files we use for system tests with the above changes to make sure all the changes work as expected.

@monicasarbu
Copy link
Contributor Author

@ruflin @tsg @urso @andrewkroh I have updated the config files with your comments.

@ruflin I agree with you that we need to update the system tests and also create a minimized version as we discussed. I have updated the meta ticket with these two new requests: #1417

@ruflin
Copy link
Contributor

ruflin commented May 6, 2016

@monicasarbu One of the configtest system tests seems to fail ... Ping when it's fixed, happy to merge and go from there.

@monicasarbu monicasarbu force-pushed the move_one_level_up_config branch from 0b9992e to a3fe239 Compare May 12, 2016 07:33
@tsg tsg merged commit c49abfe into elastic:master May 12, 2016
tsg pushed a commit to tsg/beats that referenced this pull request May 12, 2016
Little changes content wise, but some style changes:

* Use ###, ===, and --- headers to indicate hierarchy. This makes it
  easier to skim the config file. Idea stolen from elasticsearch.yml
* Unindent filebeat prospectors and metricbeat modules by one level.
  This is still valid yaml, and fewer spaces make it harder to mess up the
  white spaces.
* Reorganized a bit the logging config
* Moved the "general" libbeat section before the outputs
* Other fairly minor changes to the beats yaml files.

This is a follow up of elastic#1544 and part of elastic#1417.
ruflin pushed a commit that referenced this pull request May 12, 2016
Little changes content wise, but some style changes:

* Use ###, ===, and --- headers to indicate hierarchy. This makes it
  easier to skim the config file. Idea stolen from elasticsearch.yml
* Unindent filebeat prospectors and metricbeat modules by one level.
  This is still valid yaml, and fewer spaces make it harder to mess up the
  white spaces.
* Reorganized a bit the logging config
* Moved the "general" libbeat section before the outputs
* Other fairly minor changes to the beats yaml files.

This is a follow up of #1544 and part of #1417.

* Added a new line in front of the libbeat options
* Make the styling changes for Metricbeat in automation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss Issue needs further discussion. review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants