-
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
Allow a Beat to override monitoring-related fields #7850
Allow a Beat to override monitoring-related fields #7850
Conversation
That’s a good PR description. |
Implementation looks good to me. It feels a bit odd to have these configs inside the Info struct and I would prefer to keep it out but I understand why it's there. @urso you have some ideas on a better way on how this could be put somewhere else? |
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.
The approach looks quite hacky. The beat.Info structure is created by libbeat. It's not supposed to be used to set/configure libbeat. Let me check if we can find a more clean solution.
E.g. the cmd/instance is very minimal, with positional parameters. We could replace positional parameters with a struct providing some additional configs/constants to libbeat.
The cmd/instance is somewhat involved and requires some interaction with the beater code, working on partially initialized state only. With the introduction of features, we will try to cleanup the beat setup (rewrite it). Therefore I'd prefer to apply changes, which do not rely on any call-order between the beater and the beat setup code. Otherwise we're more likely to introduce bugs by accident or refactoring might become somewhat more complicated (depends if test code points out potential bugs). The idea would be to pass init-information from top to bottom only: Update
Circular dependencies might be a problem. Let's put the struct in Update
Note: We pass beats.Info to the monitoring reporters, so to allow us to create a local publisher pipeline and create the event meta-data. We can change the reporter factory(package 'reporter') to accept more rich settings:
The |
Thanks for the detailed feedback, @urso. Agreed that the original implementation is hacky. I will look into updating this PR with your suggestions now. |
9754381
to
449e2d2
Compare
libbeat/monitoring/report/report.go
Outdated
type Reporter interface { | ||
Stop() | ||
} | ||
|
||
type ReporterFactory func(beat.Info, *common.Config) (Reporter, error) | ||
type ReporterFactory func(Settings, *common.Config) (Reporter, 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.
exported type ReporterFactory should have comment or be unexported
@@ -30,11 +30,16 @@ type config struct { | |||
Reporter common.ConfigNamespace `config:",inline"` | |||
} | |||
|
|||
type Settings 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.
exported type Settings should have comment or be unexported
@urso I've tried to reimplement this PR as per your guidance. Please check if I did the right thing. Note that originally this PR was allowing beat authors to override the monitoring |
libbeat/cmd/instance/settings.go
Outdated
Name string | ||
IndexPrefix string | ||
Version string | ||
MonitoringDefaultUsername string |
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.
phew, that's a long/descriptive name. Is there a chance we will add more fields?
What if MonitoringDefaultUsername == ""
?
How about:
type Settings struct {
...
Monitoring MonitoringConfig
}
type MonitoringConfig struct {
Username string // default username
}
I see we already transform this parameter to report.Settings.
How about:
type Settings struct {
...
Monitoring report.Settings
}
and in code:
monSettings := settings.Monitoring // this is a copy, as Monitoring is no pointer
if monSettings.Beat == "" { // fill in more defaults if not configured
monSettings.Beat = b.Info
}
reporter, err := report.New(monSettings, ...)
config := defaultConfig | ||
overrideConfigFromBeat(&config, settings) |
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.
let's replace these with:
config := defaultConfig(settings)
with:
func defaultConfig(settings report.Settings) config {
...
}
We started with creating default configs as struct and use them as kinda templates by copying the struct. Rethinking this approach I'm in favor of functions producing the default configs.
libbeat/cmd/root.go
Outdated
// GenRootCmdWithSettingsWithRunFlags returns the root command to use for your beat. It takes the | ||
// run command, which will be called if no args are given (for backwards compatibility), | ||
// beat settings, and runFlags. runFlags parameter must the flagset used by run command | ||
func GenRootCmdWithSettingsWithRunFlags(beatCreator beat.Creator, settings instance.Settings, runFlags *pflag.FlagSet) *BeatsRootCmd { |
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.
By intruducing 'settings', I wonder if we can unify all these constructors by adding runFlags
(or some other means to compute runFlags` from settings.
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.
+1 for making this change backwards-compatible. Still, this change warrants an update in the developers changelog + explanation(upgrade info) in PR.
265bb26
to
6c2be56
Compare
@urso I made most of the changes from your latest round of review. I didn't quite understand the second half of this comment so perhaps you could take a look at the latest changes and clarify your comment w.r.t. the changed code? Thanks! |
libbeat/cmd/instance/settings.go
Outdated
// MonitoringConfig contains monitoring-specific settings | ||
type MonitoringConfig struct { | ||
DefaultUsername string | ||
} |
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.
@ycombinator the idea is to reuse reporter.Settings
here.
Like change instance.Settings
type to:
type Settings struct {
Name string
IndexPrefix string
Version string
Monitoring reporter.Settings
RunFlags *pflag.FlagSet
}
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.
Of course, duh! Thanks for holding my hand through this one 😊.
libbeat/cmd/root.go
Outdated
// GenRootCmdWithIndexPrefixWithRunFlags returns the root command to use for your beat. It takes | ||
// beat name, index prefix, version, run command, and runFlags. runFlags parameter must the flagset used by | ||
// run command | ||
// DEPRECATED! Use GenRootCmdWithSettings instead. |
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 is no valid note marker. See: https://blog.golang.org/godoc-documenting-go-code
It's some badly documented convention, but godoc can parse and filter out/display notes. The syntax for notes is: // Marker(id): description
.
In this case the comment should say:
// GenRootCmdWithIndexPrefixWithRunFlags returns the root command to use for your beat. It takes
// beat name, index prefix, version, run command, and runFlags. runFlags parameter must the flagset used by
// run command
//
// Deprecated: Use GenRootCmdWithSettings instead.
For conventions on Deprecation:
use see: golang/go#10909 (comment)
Also see this stackoverflow answer with even more links for tooling around Deprecated:
: https://stackoverflow.com/a/36360323
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.
@graphaelli Any chance you could test this branch with apm-server?
@ycombinator Please check comments from Steffen.
@ruflin said:
Yes, I've seen them. Just getting back from holiday and plan to address them today. |
@urso I've addressed the latest round of feedback in c2b867a and bc6574125de6720319f171bae94f5ccf20ee1780. My one "concern" with reusing |
Oh, didn't consider this. Yeah, you are right. 2 Options:
Option 2 might gives us some flexibility if we decide we need more |
93c84e9
to
a04b9eb
Compare
The only CI failure I'm seeing is this flaky test: #8208. So I'm ignoring it and merging this PR now. |
* Adding TODOs * Allow a beat to override monitoring-related fields * Disallowing override of system_api_version (until we need it) * Fixing datatype * Create Settings structs for monitoring-specific info * Changes from review * Make runFlags part of settings struct * Adding comments * Adding developer changelog entry * Adding default for index prefix * Fixing deprecated note markers * Reuse report.Settings * Extract beat.Info back out of Settings struct * Updating generator template * Tabs, not spaces :eyeroll: (cherry picked from commit 74cbb79)
* Adding TODOs * Allow a beat to override monitoring-related fields * Disallowing override of system_api_version (until we need it) * Fixing datatype * Create Settings structs for monitoring-specific info * Changes from review * Make runFlags part of settings struct * Adding comments * Adding developer changelog entry * Adding default for index prefix * Fixing deprecated note markers * Reuse report.Settings * Extract beat.Info back out of Settings struct * Updating generator template * Tabs, not spaces :eyeroll: (cherry picked from commit 74cbb79)
… fields (#8802) Cherry-pick of PR #7850 to 6.4 branch. Original message: By default all beats will use the `beats_system` user to talk to the Elasticsearch Monitoring bulk API. End-users can, of course, override this via the `xpack.monitoring.elasticsearch.username` configuration setting but sometimes a beat might want to specify a default other than `beats_system`. APM server is one such example wanting to use the `apm_system` user as it's default. This PR makes it so a beat can override the Monitoring default username. This override should be done when the beat exports the `cmd.RootCmd` variable like so: ```golang import ( "github.com/elastic/beats/libbeat/cmd/instance" "github.com/elastic/beats/libbeat/monitoring/report" ) var settings = instance.Settings{ Name: "apm_server", Version: "", Monitoring: report.Settings{ DefaultUsername: "apm_system", }, } var RootCmd = cmd.GenRootCmdWithSettings(beater.New, settings) ``` --- ### Deprecation notice This PR introduces a new function, `cmd.GenRootCmdWithSettings`. See the code snippet above for sample usage. This new function should be preferred over now-deprecated functions, `cmd.GenRootCmd`, `cmd.GenRootCmdWithRunFlags`, and `cmd.GenRootCmdWithIndexPrefixWithRunFlags`. See examples below on how to upgrade from the using the deprecated functions to using the new function. #### Upgrading from using `cmd.GenRootCmd` ```golang // Before var RootCmd = cmd.GenRootCmd("countbeat", "", beater.New) // After import "github.com/elastic/beats/libbeat/cmd/instance" var settings = instance.Settings{ Name: "countbeat", } var RootCmd = cmd.GenRootCmdWithSettings(beater.New, settings) ``` #### Upgrading from using `cmd.GenRootCmdWithRunFlags` ```golang // Before import "github.com/spf13/pflag" var runFlags = pflag.NewFlagSet(Name, pflag.ExitOnError) runFlags.AddGoFlag(flag.CommandLine.Lookup("I")) runFlags.AddGoFlag(flag.CommandLine.Lookup("dump")) var RootCmd = cmd.GenRootCmdWithRunFlags("countbeat", "", beater.New, runFlags) // After import ( "github.com/spf13/pflag" "github.com/elastic/beats/libbeat/cmd/instance" ) var runFlags = pflag.NewFlagSet(Name, pflag.ExitOnError) runFlags.AddGoFlag(flag.CommandLine.Lookup("I")) runFlags.AddGoFlag(flag.CommandLine.Lookup("dump")) var settings = instance.Settings{ Name: "countbeat", RunFlags: runFlags, } var RootCmd = cmd.GenRootCmdWithSettings(beater.New, settings) ``` #### Upgrading from using `cmd.GenRootCmdWithIndexPrefixWithRunFlags` ```golang // Before import "github.com/spf13/pflag" var runFlags = pflag.NewFlagSet(Name, pflag.ExitOnError) runFlags.AddGoFlag(flag.CommandLine.Lookup("I")) runFlags.AddGoFlag(flag.CommandLine.Lookup("dump")) var RootCmd = cmd.GenRootCmdWithIndexPrefixWithRunFlags("countbeat", "cb", "", beater.New, runFlags) // After import ( "github.com/spf13/pflag" "github.com/elastic/beats/libbeat/cmd/instance" ) var runFlags = pflag.NewFlagSet(Name, pflag.ExitOnError) runFlags.AddGoFlag(flag.CommandLine.Lookup("I")) runFlags.AddGoFlag(flag.CommandLine.Lookup("dump")) var settings = instance.Settings{ Name: "countbeat", IndexPrefix: "cb", RunFlags: runFlags, } var RootCmd = cmd.GenRootCmdWithSettings(beater.New, settings) ```
…related fields (elastic#8802) Cherry-pick of PR elastic#7850 to 6.4 branch. Original message: By default all beats will use the `beats_system` user to talk to the Elasticsearch Monitoring bulk API. End-users can, of course, override this via the `xpack.monitoring.elasticsearch.username` configuration setting but sometimes a beat might want to specify a default other than `beats_system`. APM server is one such example wanting to use the `apm_system` user as it's default. This PR makes it so a beat can override the Monitoring default username. This override should be done when the beat exports the `cmd.RootCmd` variable like so: ```golang import ( "github.com/elastic/beats/libbeat/cmd/instance" "github.com/elastic/beats/libbeat/monitoring/report" ) var settings = instance.Settings{ Name: "apm_server", Version: "", Monitoring: report.Settings{ DefaultUsername: "apm_system", }, } var RootCmd = cmd.GenRootCmdWithSettings(beater.New, settings) ``` --- ### Deprecation notice This PR introduces a new function, `cmd.GenRootCmdWithSettings`. See the code snippet above for sample usage. This new function should be preferred over now-deprecated functions, `cmd.GenRootCmd`, `cmd.GenRootCmdWithRunFlags`, and `cmd.GenRootCmdWithIndexPrefixWithRunFlags`. See examples below on how to upgrade from the using the deprecated functions to using the new function. #### Upgrading from using `cmd.GenRootCmd` ```golang // Before var RootCmd = cmd.GenRootCmd("countbeat", "", beater.New) // After import "github.com/elastic/beats/libbeat/cmd/instance" var settings = instance.Settings{ Name: "countbeat", } var RootCmd = cmd.GenRootCmdWithSettings(beater.New, settings) ``` #### Upgrading from using `cmd.GenRootCmdWithRunFlags` ```golang // Before import "github.com/spf13/pflag" var runFlags = pflag.NewFlagSet(Name, pflag.ExitOnError) runFlags.AddGoFlag(flag.CommandLine.Lookup("I")) runFlags.AddGoFlag(flag.CommandLine.Lookup("dump")) var RootCmd = cmd.GenRootCmdWithRunFlags("countbeat", "", beater.New, runFlags) // After import ( "github.com/spf13/pflag" "github.com/elastic/beats/libbeat/cmd/instance" ) var runFlags = pflag.NewFlagSet(Name, pflag.ExitOnError) runFlags.AddGoFlag(flag.CommandLine.Lookup("I")) runFlags.AddGoFlag(flag.CommandLine.Lookup("dump")) var settings = instance.Settings{ Name: "countbeat", RunFlags: runFlags, } var RootCmd = cmd.GenRootCmdWithSettings(beater.New, settings) ``` #### Upgrading from using `cmd.GenRootCmdWithIndexPrefixWithRunFlags` ```golang // Before import "github.com/spf13/pflag" var runFlags = pflag.NewFlagSet(Name, pflag.ExitOnError) runFlags.AddGoFlag(flag.CommandLine.Lookup("I")) runFlags.AddGoFlag(flag.CommandLine.Lookup("dump")) var RootCmd = cmd.GenRootCmdWithIndexPrefixWithRunFlags("countbeat", "cb", "", beater.New, runFlags) // After import ( "github.com/spf13/pflag" "github.com/elastic/beats/libbeat/cmd/instance" ) var runFlags = pflag.NewFlagSet(Name, pflag.ExitOnError) runFlags.AddGoFlag(flag.CommandLine.Lookup("I")) runFlags.AddGoFlag(flag.CommandLine.Lookup("dump")) var settings = instance.Settings{ Name: "countbeat", IndexPrefix: "cb", RunFlags: runFlags, } var RootCmd = cmd.GenRootCmdWithSettings(beater.New, settings) ```
By default all beats will use the
beats_system
user to talk to the Elasticsearch Monitoring bulk API. End-users can, of course, override this via thexpack.monitoring.elasticsearch.username
configuration setting but sometimes a beat might want to specify a default other thanbeats_system
. APM server is one such example wanting to use theapm_system
user as it's default.This PR makes it so a beat can override the Monitoring default username. This override should be done when the beat exports the
cmd.RootCmd
variable like so:Deprecation notice
This PR introduces a new function,
cmd.GenRootCmdWithSettings
. See the code snippet above for sample usage. This new function should be preferred over now-deprecated functions,cmd.GenRootCmd
,cmd.GenRootCmdWithRunFlags
, andcmd.GenRootCmdWithIndexPrefixWithRunFlags
. See examples below on how to upgrade from the using the deprecated functions to using the new function.Upgrading from using
cmd.GenRootCmd
Upgrading from using
cmd.GenRootCmdWithRunFlags
Upgrading from using
cmd.GenRootCmdWithIndexPrefixWithRunFlags