-
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
Migrate packetbeat config options to namespace #1490
Conversation
if pb.PbConfig.Shipper.QueueSize != nil { | ||
queueSize = *pb.PbConfig.Shipper.QueueSize | ||
if cfg.Shipper.QueueSize != nil { | ||
queueSize = *cfg.Shipper.QueueSize |
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 means that these two options under shipper
now are under packetbeat.shipper
. Since these settings might be read by both Packetbeat and libbeat, but now they are defined in two parts. We could either leave Shipper top level in config.Config
or clean this up to not require Packetbeat reading options from the libbeat sections.
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 @urso can comment on whether these two options could be just removed?
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.
Ups, this change was not intended :-( I think I should revert the shipper changes for this PR in any case.
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.
There's also the Logging section which is not quite clear to me why it is also declared at Packetbeat level.
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.
Fixed
Did you try to make it backwards compatible like for Topbeat? Does it complicate the code too much? |
|
||
# Select the network interfaces to sniff the data. You can use the "any" | ||
# keyword to sniff on all connected interfaces. | ||
interfaces: |
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 wonder if it wouldn't be better to write these as:
packetbeat.interfaces:
device: any
This way, there's no increase in the indentation level, making it easier for people to migrate and for us to review config changes.
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.
Was thinking about the same, see: #1488 (Option 2)
Going with the above would mean doing the same for flows, protocols and and procs.
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 use the . notation.
I didn't try the BC change. I think it would be possible to implement it also with BC but makes the code a little bit more complex. My idea was to make it BC in a second step if needed to keep the change smaller. |
ports: [80, 8080] | ||
send_response: true | ||
include_body_for: ["text/html"] | ||
protocols: |
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.
@tsg Thinking if I should also do here packetbeat.protocols instead of changing the indentation. At the same time it "only" describes the protocols part. Same for all other doc parts.
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.
@dedemorton WDYT?
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 packetbeat.protocols
is better, because when people copy/paste it's more likely to get it right.
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.
Ok, will change it accordingly everywhere.
* Fix docs by adding indentation * Remove singleton config Relates to elastic#1417
"github.com/elastic/beats/packetbeat/procs" | ||
) | ||
|
||
type Config struct { | ||
Packetbeat PacketbeatConfig |
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.
alternatively this could have been written like:
type Config struct {
Interfaces InterfacesConfig `config:"packetbeat.interfaces"`
Flows *Flows `config:"packetbeat.flows"`
...
}
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.
Interesting, I wasn't aware that this would work.
This change is not backward compatible.
Relates to #1417