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

Add ParseHost() for parsing configured hosts in the Module #2958

Merged
merged 18 commits into from Nov 22, 2016

Conversation

Projects
None yet
3 participants
@andrewkroh
Copy link
Member

commented Nov 8, 2016

Add HostParser function that can be registered with MetricSetFactories

The ParseHost() function will combine configuration data with the raw host values to produce a value appropriate for use by the MetricSet to make a connection.

Currently the HostParser parameter is optional when registering a MetricSetFactory, but it will become required once all MetricSets are updated to have a HostParser.

updated: Monday, Nov 21

@tsg

This comment has been minimized.

Copy link
Collaborator

commented Nov 8, 2016

I wonder if we should really worry about BWC at this point. I don't think there are any external Metricbeat based Beats, and even if they were, it would be trivial to provide an empty implementation for that function.

@ruflin

This comment has been minimized.

Copy link
Collaborator

commented Nov 9, 2016

I like that it is BC compatible as currently the module file normally does not exist. I would not necessarely tie it to 6.0 to make it mandatory, but only make it mandatory when we got some more experience with it to make sure external developers only need to change it once.

For the generator we should provide also an empty method which potentially logs an error, that it is not implemented.

In the future we should offer "generic" ParseHost() methods for example for Http so these can be reused.

@andrewkroh andrewkroh force-pushed the andrewkroh:feature/mb/parse-host branch from 7371d17 to e434ec7 Nov 17, 2016

@andrewkroh

This comment has been minimized.

Copy link
Member Author

commented Nov 17, 2016

I changed my approach on this because providing the parsing functionality at the module level wasn't sufficient. Individual MetricSets within the same module can have unique host parsing requirements. For example, nginx MetricSets for stub-status and plus-status might take the same host value and append different paths.

So my approach is to allow (require in the future) registering a HostParser function with the MetricSetFactory. If all MetricSets in a Module can share the same HostParser function then you just write that inside the module package and use it in each MetricSet registration. We can provide some utilities in a helper package to cut down on the code duplication.

As for not breaking BWC, it looks like there's only one external MetricSet at the moment. So we won't break much when we require the HostParser function at registration time. We don't have to wait until 6.0 to do this. Once each of our MetricSets has a HostParser I'll make it a requirement. I'll implement HostParsers for the other MetricSets in a separate PR.

@andrewkroh andrewkroh force-pushed the andrewkroh:feature/mb/parse-host branch from e434ec7 to e24f7e0 Nov 17, 2016

@ruflin
Copy link
Collaborator

left a comment

What happens with metricsets that don't have a host like system? Will they just pass nil?

@@ -57,6 +61,7 @@ type MetricSet interface {
Host() string // Host returns a hostname or other module specific value
// that identifies a specific host or service instance from which to collect
// metrics.
HostInfo() HostInfo // HostInfo returns the parsed host data.

This comment has been minimized.

Copy link
@ruflin

ruflin Nov 18, 2016

Collaborator

Should we call this HostData()?

This comment has been minimized.

Copy link
@andrewkroh

andrewkroh Nov 21, 2016

Author Member

Changed to HostData.

// HostInfo contains values parsed from the 'host' configuration. Other
// configuration data like protocols, usernames, and passwords may also be
// used to construct this HostInfo data.
type HostInfo struct {

This comment has been minimized.

Copy link
@ruflin

ruflin Nov 18, 2016

Collaborator

HostData ?

@@ -0,0 +1,91 @@
package helper

This comment has been minimized.

Copy link
@ruflin

ruflin Nov 18, 2016

Collaborator

should we move the helper package to the top level and not under mb?

This comment has been minimized.

Copy link
@andrewkroh

andrewkroh Nov 21, 2016

Author Member

I left the package inside of mb because it contains functions that apply specifically to the things in the mb. I did rename it from the generic helpers to parse which is more specific and less likely to collide with a future helpers package that may exist someday.

// ParseServerStatusURL takes the configuration data and host value and
// constructs a HostInfo object containing the URI to the Apache server status
// endpoint.
func ParseServerStatusURL(mod mb.Module, host string) (mb.HostInfo, error) {
config := struct {

This comment has been minimized.

Copy link
@ruflin

ruflin Nov 18, 2016

Collaborator

I kind of liked that config part was in New. But I see that having it here will make it easier reusable for the different modules.

@andrewkroh andrewkroh force-pushed the andrewkroh:feature/mb/parse-host branch 2 times, most recently from 5f425e4 to 9986930 Nov 21, 2016

@andrewkroh andrewkroh added review and removed in progress labels Nov 21, 2016

@andrewkroh

This comment has been minimized.

Copy link
Member Author

commented Nov 21, 2016

jenkins, test it

andrewkroh added some commits Nov 21, 2016

Add HostParser function that can be registered with MetricSetFactories
The ParseHost() function will combine configuration data with the raw host values to produce a value appropriate for use by the MetricSet to make a connection.

Currently the HostParser parameter is optional when registering a MetricSetFactory, but it will become required once all MetricSets are updated to have a HostParser.
Implement HostParser for docker module
- Changed docker configuration options to be consistent with other modules. The `socket` should now be specified using `hosts`. The TLS options are now grouped under `ssl` and named consistently with the options in other parts of Beats (with the exception of certificate_authority which is singular because docker only accepts a single file).
- Cleanup some variable naming in the docker module.

@andrewkroh andrewkroh force-pushed the andrewkroh:feature/mb/parse-host branch from 9986930 to e56afc2 Nov 21, 2016

andrewkroh added some commits Nov 21, 2016

# Host DSN should be defined as "user:pass@tcp(127.0.0.1:3306)/"
# The username and password can either be set in the DSN or using the username
# and password config options. Those specified in the DSN take precedence.
hosts: ["root:secret@tcp(127.0.0.1:3306)/"]

This comment has been minimized.

Copy link
@ruflin

ruflin Nov 21, 2016

Collaborator

This should probably be commented out?

@ruflin

ruflin approved these changes Nov 22, 2016

Copy link
Collaborator

left a comment

Create addition and thanks a lot for all the cleanup / linting. I left a few minor comments, all of which can be discussed later. Will merge this PR.

SanitizedURI string // A sanitized version of the URI without credentials.

// Parts of the URI.
Host string // The host and possibly port.

This comment has been minimized.

Copy link
@ruflin

ruflin Nov 22, 2016

Collaborator

In the future we can perhaps extract also the port in case this could become useful.

}

func GetDefaultConf() Config {
return Config{
Socket: "unix:///var/run/docker.sock",

This comment has been minimized.

Copy link
@ruflin

ruflin Nov 22, 2016

Collaborator

No need for the default config anymore?

)

func init() {
// Register the ModuleFactory function for the "mysql" module.

This comment has been minimized.

Copy link
@ruflin

ruflin Nov 22, 2016

Collaborator

This should say mongodb not mysql


func NewModule(base mb.BaseModule) (mb.Module, error) {
// Validate that at least one host has been specified.
config := struct {

This comment has been minimized.

Copy link
@ruflin

ruflin Nov 22, 2016

Collaborator

we could turn this around and make hosts required by default and only system module overwrites it to not be required.

This comment has been minimized.

Copy link
@andrewkroh

andrewkroh Nov 22, 2016

Author Member

You're right, the system module is more the exception than the rule w.r.t. not requiring or accepting a host.

While making these changes I had an idea to accept a vararg of options when registering metricsets, like AddMetricSet("system", "process", New, OptHostsNotAllowed, OptPushesData, OptSomeOtherAttribute).

This comment has been minimized.

Copy link
@ruflin

ruflin Nov 23, 2016

Collaborator

+1 on making it a list of params instead of a single one, as I also think we will add more int he future.

@ruflin ruflin merged commit 27478e0 into elastic:master Nov 22, 2016

4 checks passed

CLA Commit author has signed the CLA
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
default Build finished.
Details

suraj-soni added a commit to suraj-soni/beats that referenced this pull request Dec 15, 2016

Add ParseHost() for parsing configured hosts in the Module (elastic#2958
)

Add HostParser function that can be registered with MetricSetFactories

The ParseHost() function will combine configuration data with the raw host values to produce a value appropriate for use by the MetricSet to make a connection.

Currently the HostParser parameter is optional when registering a MetricSetFactory, but it will become required once all MetricSets are updated to have a HostParser.

* Implement HostParser for apache module
* Implement HostParser for docker module
* Changed docker configuration options to be consistent with other modules. The `socket` should now be specified using `hosts`. The TLS options are now grouped under `ssl` and named consistently with the options in other parts of Beats (with the exception of certificate_authority which is singular because docker only accepts a single file).
* Cleanup some variable naming in the docker module.
* Implement HostParser for haproxy module
* Implement HostParser for kafka module
* Implement HostParser for mongodb module
* Implement HostParser for mysql module
* Implement HostParser for nginx module
* Implement HostParser for postgresql module
* Implement HostParser for redis module
* Implement HostParser for system module
* Implement HostParser for zookeeper module
* Update generated files
* Add docker to documentation
* Update postgresql docs and config
* Rebuild docs
* Update generated data.json files for metricbeat modules
* Comment out mysql hosts in config file

@andrewkroh andrewkroh deleted the andrewkroh:feature/mb/parse-host branch Mar 31, 2017

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