Add appender support to autodiscover #6469

vjsamuel · 2018-02-26T03:15:52Z

Appenders have the ability to append additional info into the configuration. There are two appenders that are part of this PR:

config appender: This appender can append a templatized config into all of the generated configs by either templates or builders and can be used by any Beat.

Sample config:

appenders:
  - type: config
     - condition.equals:
          kubernetes.labels.app: "prometheus"
        config: 
           fields:
             test: success

kubernetes.token appender: There are endpoints in kubernetes that require bearer token authentication. This appender can add headers.Authorization to a metricset configuration. This can be used to collect metrics from Kube API server.

appenders:
  - type: "kubernetes.token"

elasticmachine · 2018-02-26T03:15:53Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

houndci-bot · 2018-02-26T03:16:02Z

metricbeat/autodiscover/appender/kubernetes/token/token.go

+	Condition *processors.Condition
+}
+
+func NewTokenAppender(cfg *common.Config) (autodiscover.Appender, error) {


exported function NewTokenAppender should have comment or be unexported

houndci-bot · 2018-02-26T03:16:03Z

libbeat/autodiscover/appenders/config/config.go

+	configMaps []configMap
+}
+
+func NewConfigAppender(cfg *common.Config) (autodiscover.Appender, error) {


exported function NewConfigAppender should have comment or be unexported

ruflin · 2018-02-27T05:47:17Z

jenkins, test it

exekias · 2018-02-27T19:20:45Z

I'm wondering if this construct should be moved to the module level instead, for a wider usage. As you could need this exact functionality outside autodiscover.

They could add in the same way as processors, where you could define global appenders, scoped to all modules/prospectors, or module specific ones.

WDYT?

vjsamuel · 2018-02-27T19:31:50Z

I thought of the same as well. Any specific thoughts on how it should look?

ruflin · 2018-02-28T08:47:03Z

I definitively think the features we brought to k8s so far can also be useful in a more generic way like mentioned above. I'm wondering if we could go forward with the above implementation for now, mark it as beta, learn from it and then extract it to be also reused in other parts?

exekias · 2018-02-28T17:37:12Z

The thing is this specific appender would be cool also for some commonly static Metricbeat modules, like kubernetes, prometheus or even http, as it gives them auth inside the cluster.

I could also implement that at the http helper level, and expose the settings only on these modules, but appender interface sounds like the right solution to the problem

ruflin · 2018-03-04T05:14:47Z

@exekias WDYT about moving forward. Getting it in as is and abstract it later or you think that will be tricky?

vjsamuel · 2018-03-04T05:22:28Z

@ruflin i have added experimental tag to all the builders/providers and appenders.

exekias · 2018-03-04T14:56:45Z

libbeat/autodiscover/appenders/config/config.go

+			// Merge the template with all the configs
+			for _, cfg := range cfgs {
+				cf := common.MapStr{}
+				err := cfg.Unpack(&cf)


This error is not being checked

exekias · 2018-03-04T15:00:39Z

Ok, let's get this in, we can make it general later

I left just a minor comment

vjsamuel · 2018-03-05T01:12:09Z

@exekias @ruflin done with all review comments.

andrewkroh

This looks well done. We need to have some level of documentation for this that explains what it does, when/why to use it, and how to configure it.

andrewkroh · 2018-03-08T00:55:15Z

libbeat/autodiscover/appender.go

+
+	appender := r.GetAppender(config.Type)
+	if appender == nil {
+		return nil, fmt.Errorf("Unknown autodiscover appender %s", config.Type)


Don't capitalize error strings. https://github.com/golang/go/wiki/CodeReviewComments#error-strings

andrewkroh · 2018-03-08T00:58:03Z

libbeat/autodiscover/appenders/config/config.go

+	configMaps []configMap
+}
+
+// NewConfigAppender creates a configAppender that can append tempatized configs into built configs


s/tempatized/templatized/

andrewkroh · 2018-03-08T01:00:27Z

libbeat/autodiscover/appenders/config/config.go

+				cf := common.MapStr{}
+				err := cfg.Unpack(&cf)
+				if err != nil {
+					logp.Debug("config", "unable to unpack config due to error: %v", err)


Should these have a higher visibility than Debug? You would know better than me.

this can be left as is.

andrewkroh · 2018-03-08T03:36:23Z

libbeat/autodiscover/providers/docker/docker.go

+	var appenders autodiscover.Appenders
+	for _, acfg := range config.Builders {
+		if appender, err := autodiscover.Registry.BuildAppender(acfg); err != nil {
+			logp.Debug("docker", "failed to construct autodiscover appender due to error: %v", err)


Likewise, should this be at the Info or Warn levels?

vjsamuel · 2018-03-08T07:33:22Z

@andrewkroh i have incorporated review comments.

andrewkroh · 2018-03-08T07:41:45Z

Thanks.

Looks like we still need documentation on Appenders.

And since there is config associated with appenders we should have it in the reference config. @exekias How come add_kubernetes_metadata isn't in the reference config?

vjsamuel · 2018-03-08T17:47:29Z

@andrewkroh we talked about the documentation and some consensus needs to be arrived at for the structuring of autodiscover documentation as there are providers, templates, builders and appenders that needs documentation at the moment. i would reserve that for a separate PR.

andrewkroh · 2018-03-08T20:05:21Z

I think we could at least document the basics of what it does, when/why to use it, and how to configure it without knowing what the exact final structure will be by adding a new "Appenders" section to the autodiscover topic. If we produce the content our talented technical writers can later shape it and give it more structure.

Basically I dislike adding new features without docs. Sometimes writing the docs even leads you to re-think how something you wrote should work or be configured.

vjsamuel · 2018-03-08T20:21:32Z

Sure thing. Let me add a documentation commit.

andrewkroh · 2018-03-12T21:53:23Z

jenkins, test it

houndci-bot reviewed Feb 26, 2018

View reviewed changes

vjsamuel force-pushed the add_autodiscover_appenders branch from db02db7 to 7f36b15 Compare February 26, 2018 03:17

ruflin added review libbeat labels Feb 26, 2018

exekias reviewed Mar 4, 2018

View reviewed changes

andrewkroh reviewed Mar 8, 2018

View reviewed changes

vjsamuel force-pushed the add_autodiscover_appenders branch from 94845c2 to 3df6aae Compare March 8, 2018 07:33

vjsamuel force-pushed the add_autodiscover_appenders branch 3 times, most recently from f315f95 to 46a670f Compare March 12, 2018 05:01

andrewkroh approved these changes Mar 12, 2018

View reviewed changes

vjsamuel added 3 commits March 13, 2018 10:00

Add appender support to autodiscover

c4b3f93

Make hound happy

260fc6d

Add experimental to all autodiscover modules

98b1858

vjsamuel added 3 commits March 13, 2018 10:00

Add missing error check

c86e61c

Incorporate review comments

32ea535

Add docs

65383a8

vjsamuel force-pushed the add_autodiscover_appenders branch from 46a670f to 65383a8 Compare March 13, 2018 17:00

exekias merged commit 3be3556 into elastic:master Mar 14, 2018

ph mentioned this pull request Mar 30, 2018

Fixes autodiscover's appender configuration example #6720

Merged

vjsamuel deleted the add_autodiscover_appenders branch July 25, 2018 06:10

tetianakravchenko mentioned this pull request Jun 15, 2022

Fix appenders documentation #31932

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add appender support to autodiscover #6469

Add appender support to autodiscover #6469

vjsamuel commented Feb 26, 2018 •

edited

Loading

elasticmachine commented Feb 26, 2018

houndci-bot Feb 26, 2018

houndci-bot Feb 26, 2018

ruflin commented Feb 27, 2018

exekias commented Feb 27, 2018 •

edited

Loading

vjsamuel commented Feb 27, 2018

ruflin commented Feb 28, 2018

exekias commented Feb 28, 2018

ruflin commented Mar 4, 2018

vjsamuel commented Mar 4, 2018

exekias Mar 4, 2018

exekias commented Mar 4, 2018

vjsamuel commented Mar 5, 2018

andrewkroh left a comment

andrewkroh Mar 8, 2018

andrewkroh Mar 8, 2018

andrewkroh Mar 8, 2018

vjsamuel Mar 8, 2018

andrewkroh Mar 8, 2018

vjsamuel commented Mar 8, 2018

andrewkroh commented Mar 8, 2018

vjsamuel commented Mar 8, 2018

andrewkroh commented Mar 8, 2018 •

edited

Loading

vjsamuel commented Mar 8, 2018

andrewkroh commented Mar 12, 2018

Add appender support to autodiscover #6469

Add appender support to autodiscover #6469

Conversation

vjsamuel commented Feb 26, 2018 • edited Loading

elasticmachine commented Feb 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruflin commented Feb 27, 2018

exekias commented Feb 27, 2018 • edited Loading

vjsamuel commented Feb 27, 2018

ruflin commented Feb 28, 2018

exekias commented Feb 28, 2018

ruflin commented Mar 4, 2018

vjsamuel commented Mar 4, 2018

Choose a reason for hiding this comment

exekias commented Mar 4, 2018

vjsamuel commented Mar 5, 2018

andrewkroh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vjsamuel commented Mar 8, 2018

andrewkroh commented Mar 8, 2018

vjsamuel commented Mar 8, 2018

andrewkroh commented Mar 8, 2018 • edited Loading

vjsamuel commented Mar 8, 2018

andrewkroh commented Mar 12, 2018

vjsamuel commented Feb 26, 2018 •

edited

Loading

exekias commented Feb 27, 2018 •

edited

Loading

andrewkroh commented Mar 8, 2018 •

edited

Loading