Add builder support for autodiscover and annotations builder #6408

vjsamuel · 2018-02-19T07:44:51Z

This PR adds builder support for Beats autodiscover along with an annotations builder for filebeat and metricbeat.

Prefix for logs would be co.elastic.logs/ and co.elastic.metrics/ for metrics. Sample config:

filebeat.autodiscover:
  providers:
    - type: kubernetes
      in_cluster: false
      kube_config: config
      builders:
        - type: log.annotations

the log.annotations takes a list of templatized configurations that can be used to build input configs. The default is a simple docker input.

Annotations that are accepted for logs are /disable which would disable the input creation and hence log tailing, /include_lines, /exclude_lines, /multiline.* where * would be pattern, negate, match etc.

If co.elastic.logs/* is used then the annotation is honored at a pod level. If `co.elastic.logs./* is used then it is honored at the container level. The latter always takes precedence.

The metric.annotations builder can be used to auto discover metric endpoints. Sample annotation set:

      co.elastic.metrics/hosts: ${data.host}:9090
      co.elastic.metrics/module: prometheus
      co.elastic.metrics/namespace: test

Sample config:

metricbeat.autodiscover:
  providers:
    - type: kubernetes
      in_cluster: false
      kube_config: config
      builders:
        - type: metrics.annotations

This would spin up a prometheus collector metricset. Other supported annotations are:

/timeout
/period
/ssl.* #to set things like insecure skip verify

co.elastic.metrics.<container>/* can be used to configure entries at a container level.

The annotations builder are provider agnostic hence both docker and kubernetes can leverage the same. Both providers will pass to the builder a provider agnostic bus.Event which has:

host
port
annotations #kubernetes annotations/docker labels
container #container information like id and name

Test cases are still WIP. Kindly review the overall structure as it is a big change. The above said will be added to the same PR to complete the builder flow for polling use-cases.

elasticmachine · 2018-02-19T07:44: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-19T07:45:00Z

libbeat/autodiscover/template/config.go

+	return result
+}
+
+func ApplyConfigTemplate(event bus.Event, configs []*common.Config) []*common.Config {


exported function ApplyConfigTemplate should have comment or be unexported

houndci-bot · 2018-02-19T07:45:00Z

libbeat/autodiscover/registry.go

+var Registry = NewRegistry()
+
+// NewRegistry creates and returns a new Registry
+func NewRegistry() *registry {


exported func NewRegistry returns unexported type *autodiscover.registry, which can be annoying to use

im not sure what to do about this :)

This could return an interface implemented by *register, wdyt?

register doesnt implement any interfaces at the moment. i think even mb registry and few others will have the same hound violation.

works for me 😄

houndci-bot · 2018-02-19T07:45:01Z

libbeat/autodiscover/builder/helper.go

+	return result
+}
+
+func GetContainerAnnotationsWithPrefix(annotations map[string]string, prefix, container, key string) map[string]string {


exported function GetContainerAnnotationsWithPrefix should have comment or be unexported

houndci-bot · 2018-02-19T07:45:01Z

libbeat/autodiscover/builder/helper.go

+	return IsNoOp(annotations, fmt.Sprintf("%s.%s", prefix, container))
+}
+
+func GetAnnotationsWithPrefix(annotations map[string]string, prefix, key string) map[string]string {


exported function GetAnnotationsWithPrefix should have comment or be unexported

houndci-bot · 2018-02-19T07:45:01Z

libbeat/autodiscover/builder/helper.go

+	return noop
+}
+
+func IsContainerNoOp(annotations map[string]string, prefix, container string) bool {


exported function IsContainerNoOp should have comment or be unexported

houndci-bot · 2018-02-19T07:45:02Z

libbeat/autodiscover/builder.go

+	return builder(c)
+}
+
+func (b Builders) GetConfig(event bus.Event) []*common.Config {


exported method Builders.GetConfig should have comment or be unexported

houndci-bot · 2018-02-19T07:45:02Z

libbeat/autodiscover/builder.go

+
+type Builders []Builder
+
+type BuilderConstructor func(*common.Config) (Builder, error)


exported type BuilderConstructor should have comment or be unexported

houndci-bot · 2018-02-19T07:45:02Z

libbeat/autodiscover/builder.go

+	CreateConfig(event bus.Event) []*common.Config
+}
+
+type Builders []Builder


exported type Builders should have comment or be unexported

houndci-bot · 2018-02-19T07:45:02Z

libbeat/autodiscover/builder.go

+	"github.com/elastic/beats/libbeat/logp"
+)
+
+type Builder interface {


exported type Builder should have comment or be unexported

houndci-bot · 2018-02-19T07:45:02Z

filebeat/autodiscover/include.go

+package autodiscover
+
+import (
+	_ "github.com/elastic/beats/filebeat/autodiscover/builder/log_annotations"


a blank import should be only in a main or test package, or have a comment justifying it

houndci-bot · 2018-02-19T23:13:37Z

metricbeat/autodiscover/include.go

+package autodiscover
+
+import (
+	_ "github.com/elastic/beats/metricbeat/autodiscover/builder/metric_annotations"


a blank import should be only in a main or test package, or have a comment justifying it

houndci-bot · 2018-02-19T23:13:37Z

metricbeat/autodiscover/builder/metric_annotations/metric_annotations.go

+func (m *metricAnnotations) getTimeout(annotations map[string]string, container string) string {
+	if tout := builder.GetContainerAnnotationAsString(annotations, m.Prefix, container, timeout); tout != "" {
+		return tout
+	} else {


if block ends with a return statement, so drop this else and outdent its block (move short variable declaration to its own line if necessary)

houndci-bot · 2018-02-19T23:13:37Z

metricbeat/autodiscover/builder/metric_annotations/metric_annotations.go

+func (m *metricAnnotations) getPeriod(annotations map[string]string, container string) string {
+	if ival := builder.GetContainerAnnotationAsString(annotations, m.Prefix, container, period); ival != "" {
+		return ival
+	} else {


if block ends with a return statement, so drop this else and outdent its block (move short variable declaration to its own line if necessary)

houndci-bot · 2018-02-19T23:13:38Z

metricbeat/autodiscover/builder/metric_annotations/metric_annotations.go

+		// Prometheus stats can be explicitly configured if need be.
+		if module == "prometheus" {
+			return []string{"collector"}
+		} else {


if block ends with a return statement, so drop this else and outdent its block

houndci-bot · 2018-02-19T23:13:38Z

metricbeat/autodiscover/builder/metric_annotations/metric_annotations.go

+	Prefix string
+}
+
+func NewMetricAnnotations(cfg *common.Config) (autodiscover.Builder, error) {


exported function NewMetricAnnotations should have comment or be unexported

houndci-bot · 2018-02-19T23:13:38Z

metricbeat/autodiscover/builder/metric_annotations/metric_annotations.go

+	ssl        = "ssl"
+
+	default_timeout  = "3s"
+	default_interval = "1m"


don't use underscores in Go names; const default_interval should be defaultInterval

houndci-bot · 2018-02-19T23:13:38Z

metricbeat/autodiscover/builder/metric_annotations/metric_annotations.go

+	timeout    = "timeout"
+	ssl        = "ssl"
+
+	default_timeout  = "3s"


don't use underscores in Go names; const default_timeout should be defaultTimeout

houndci-bot · 2018-02-19T23:13:38Z

metricbeat/autodiscover/builder/metric_annotations/metric_annotations.go

@@ -0,0 +1,141 @@
+package metric_annotations


don't use an underscore in package name

houndci-bot · 2018-02-19T23:13:38Z

metricbeat/autodiscover/builder/metric_annotations/config.go

@@ -0,0 +1,11 @@
+package metric_annotations


don't use an underscore in package name

exekias · 2018-02-20T00:10:14Z

Thank you for working on this @vjsamuel! 🎉

Sorry for the delay answering here. Builders interface is looking great! I have some thoughts/questions:

Do you have more builders in mind? I'm wondering if we should make the configuration simpler (like use_annotations: true)?
We need builders to support several providers (both Docker and Kubernetes at the moment). Perhaps in the future, even more things outside autodiscover. Current implementation relays in a hardcoded structure of annotations.

That's why I think we should introduce the concept of hints to Autodiscover events. The main idea is that each provider can fill a set of well-known hint fields, that builders can then use to build configurations from events coming from any provider. Example of a Docker event:

{
  "host": "10.4.15.9",
  "port": 2379,
  "hints": {
    "metrics": {
      "enabled": true,
      "hosts": ["${data.host}:9090"],
      "module": "prometheus",
    }
  },
  "docker": {
    "container": {
      "id": "13a2...d716"
      "name": "etcd",
      "image": "quay.io/coreos/etcd:v3.0.0",
      "labels": {
          "co.elastic.metrics/hosts": "${data.host}:9090",
          "co.elastic.metrics/module": "prometheus",
          "co.elastic.metrics/namespace": "test",
          ...
      }
    }
  }
}

Putting the responsibility of extracting hints in the provider sounds good, as it will allow us to implement the hinting system in many platforms.

What do you think?

vjsamuel · 2018-02-20T00:18:57Z

@exekias i have the graphite annotations builder coming up in a next PR once this structure is finalized and merged. hence simply use_annotations: true may not fully work as graphite has more configuration that it will take from the yml file. im not sure how hints would work in a very ephemeral environment like Kubernetes where a user has deployed all sorts of applications requiring to bootstrap multiple kinds of modules. the reason why i generate a new event and place platform agnostic information is to make the builders provider independent.

exekias · 2018-02-20T00:52:02Z

We had a chat about this, code is looking great, we agreed on doing some changes:

Introduce a hints field in autodiscover events
Providers are in charge of extracting hints from their domain into the event
Builders are in charge of taking hints and creating new configurations based on them
Builders will keep guessing the right base conf (ie prospector) by now, we may find a more agnostic way in the future

houndci-bot · 2018-02-20T08:48:51Z

metricbeat/autodiscover/builder/metric_annotations/metric_annotations.go

+func (m *metricAnnotations) getTimeout(hints common.MapStr) string {
+	if tout := builder.GetHintString(hints, m.Key, timeout); tout != "" {
+		return tout
+	} else {


if block ends with a return statement, so drop this else and outdent its block (move short variable declaration to its own line if necessary)

houndci-bot · 2018-02-20T08:48:52Z

metricbeat/autodiscover/builder/metric_annotations/metric_annotations.go

+func (m *metricAnnotations) getPeriod(hints common.MapStr) string {
+	if ival := builder.GetHintString(hints, m.Key, period); ival != "" {
+		return ival
+	} else {


if block ends with a return statement, so drop this else and outdent its block (move short variable declaration to its own line if necessary)

houndci-bot · 2018-02-20T08:48:52Z

metricbeat/autodiscover/builder/metric_annotations/metric_annotations.go

+	Key string
+}
+
+// Build a new metrics annotation builder


comment on exported function NewMetricAnnotations should be of the form "NewMetricAnnotations ..."

houndci-bot · 2018-02-20T08:48:52Z

libbeat/common/kubernetes/types.go

+	return cID
+}
+
+// GetContainerID parses the container ID to get the actual ID string


comment on exported method PodContainerStatus.GetContainerIDWithRuntime should be of the form "GetContainerIDWithRuntime ..."

houndci-bot · 2018-02-20T08:48:52Z

libbeat/autodiscover/providers/kubernetes/config.go

+	}
+}
+
+func (c *Config) Validate() {


exported method Config.Validate should have comment or be unexported

houndci-bot · 2018-02-20T08:48:52Z

libbeat/autodiscover/providers/docker/config.go

+	}
+}
+
+func (c *Config) Validate() {


exported method Config.Validate should have comment or be unexported

houndci-bot · 2018-02-20T08:48:52Z

filebeat/autodiscover/builder/log_annotations/log_annotations.go

+	hIface, ok := event["hints"]
+	if !ok {
+		return config
+	} else {


if block ends with a return statement, so drop this else and outdent its block

houndci-bot · 2018-02-20T08:48:52Z

filebeat/autodiscover/builder/log_annotations/log_annotations.go

+	Config []*common.Config
+}
+
+// Construct a log annotations builder


comment on exported function NewLogAnnotations should be of the form "NewLogAnnotations ..."

houndci-bot · 2018-02-20T08:48:52Z

filebeat/autodiscover/builder/log_annotations/log_annotations.go

@@ -0,0 +1,98 @@
+package log_annotations


don't use an underscore in package name

exekias

This is getting in shape!, I left a few comments, let's discuss them.

exekias · 2018-02-20T11:25:29Z

libbeat/autodiscover/autodiscover_test.go

@@ -10,6 +10,8 @@ import (
 	"github.com/elastic/beats/libbeat/common/bus"

 	"github.com/stretchr/testify/assert"
+
+	"github.com/elastic/beats/libbeat/autodiscover/template"


this import should go with the rest of github.com/elastic/beats/libbeat/

exekias · 2018-02-20T18:54:04Z

filebeat/autodiscover/builder/log_annotations/log_annotations.go

+)
+
+func init() {
+	autodiscover.Registry.AddBuilder("log.annotations", NewLogAnnotations)


Perhaps it should be just logs?

i thought of the same. will change it as well.

exekias · 2018-02-20T18:54:40Z

libbeat/autodiscover/builder.go

+}
+
+// ConstructBuilder reads provider configuration and instatiate one
+func (r *registry) ConstructBuilder(c *common.Config) (Builder, error) {


I think we normally use Create word for constructors

the previous constructor for provider was BuildProvider. It seemed redundant to have BuildBuilder. Thats why I made this as Construct. I ll change.

exekias · 2018-02-20T18:58:38Z

libbeat/autodiscover/provider.go

-		providers: make(map[string]ProviderBuilder, 0),
-	}
-}
+type ProviderBuilder func(bus.Bus, *template.Mapper, Builders, *common.Config) (Provider, error)


What if the provider doesn't support builders? (for instance, it doesn't give you any hints). I'm more comfortable with the current approach, letting the provider decide what it supports (templates, builders...) and plug it in.

ok. sure. i will revert this guy. makes it easier for submitting more PRs in parallel :)

exekias · 2018-02-20T19:02:35Z

libbeat/autodiscover/providers/kubernetes/kubernetes.go

-			"id":    containerIDs[c.Name],
-			"name":  c.Name,
-			"image": c.Image,
+		// Without this check there would be overlapping configurations with and without ports.


I'm ok with this change but could you open it in a separate PR? this one could grow a lot

i have removed this. i will add it in once this PR is merged.

exekias · 2018-02-20T19:06:06Z

libbeat/autodiscover/providers/kubernetes/kubernetes.go

@@ -154,6 +155,44 @@ func (p *Provider) publish(event bus.Event) {
 	// Try to match a config
 	if config := p.templates.GetConfig(event); config != nil {
 		event["config"] = config
+	} else {


this code path should be similar p.templates.GetConfig, I would extract all the logic to generate hints from the event, and then call p.builders.GetConfig

it may be a bit difficult as handling is provider specific unlike templates. WDYT?

exekias · 2018-02-21T11:23:02Z

libbeat/autodiscover/providers/docker/docker.go

-	labelMap := common.MapStr{}
-	for k, v := range container.Labels {
-		labelMap[k] = v
-	}


Why did you remove this? this was a fix introduced a few days ago, if labels is not a MapStr autodiscover conditions won't work on it

exekias

Thank you so much @vjsamuel! 🎉 🎉 let's get this in, we may want to rethink some config options, but we can do it in followup PRs

exekias · 2018-02-22T16:41:54Z

jenkins, test it please

ruflin

I think we should create a follow up issue to add docs for this awesome feature.

@exekias @vjsamuel Sounds also like a blog post to me ;-)

houndci-bot reviewed Feb 19, 2018

View reviewed changes

vjsamuel force-pushed the autodiscover_builders branch from 105041b to e9e3a84 Compare February 19, 2018 17:22

houndci-bot reviewed Feb 19, 2018

View reviewed changes

houndci-bot reviewed Feb 20, 2018

View reviewed changes

exekias reviewed Feb 20, 2018

View reviewed changes

vjsamuel added 3 commits February 21, 2018 00:16

Add builder support for autodiscover and annotations builder

e3a925c

Add metric.annotations support

b385b28

Add hint support for builders

bb4b8cd

vjsamuel force-pushed the autodiscover_builders branch 2 times, most recently from df2ed5e to df06112 Compare February 21, 2018 08:19

Adding test cases and incorporate review comments

dd889e9

vjsamuel force-pushed the autodiscover_builders branch from df06112 to dd889e9 Compare February 21, 2018 10:50

vjsamuel changed the title ~~[WIP] Add builder support for autodiscover and annotations builder~~ Add builder support for autodiscover and annotations builder Feb 21, 2018

exekias reviewed Feb 21, 2018

View reviewed changes

Make annotations to use common.MapStr

58cc1e3

exekias approved these changes Feb 22, 2018

View reviewed changes

ruflin approved these changes Feb 23, 2018

View reviewed changes

exekias merged commit 326c68f into elastic:master Feb 23, 2018

vjsamuel deleted the autodiscover_builders branch February 23, 2018 11:33


		type Builders []Builder

		type BuilderConstructor func(*common.Config) (Builder, error)

Add builder support for autodiscover and annotations builder #6408

Add builder support for autodiscover and annotations builder #6408

Conversation

vjsamuel commented Feb 19, 2018 • edited Loading

elasticmachine commented Feb 19, 2018

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

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

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

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

exekias commented Feb 20, 2018

vjsamuel commented Feb 20, 2018

exekias commented Feb 20, 2018 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

exekias 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

vjsamuel Feb 20, 2018 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

exekias left a comment

Choose a reason for hiding this comment

exekias commented Feb 22, 2018

ruflin left a comment

Choose a reason for hiding this comment

vjsamuel commented Feb 19, 2018 •

edited

Loading

exekias commented Feb 20, 2018 •

edited

Loading

vjsamuel Feb 20, 2018 •

edited

Loading