[AD] Exclude logs and metrics separately #5312

ahmed-mez · 2020-04-09T17:25:46Z

What does this PR do?

Offers the possibility to filter out the collection of logs OR metrics separately for auto-discovered containers by introducing new filtering parameters
- container_include_metrics
- container_exclude_metrics
- container_include_logs
- container_exclude_logs
Replaces ac_include and ac_exclude by container_include and container_exclude and ensure backward compatibility

Motivation

More flexible AD filtering

Additional Notes

Brief explanation of the expected behaviour

If the AD service (container) matches container_exclude, the AD listener ignores it and doesn't create the AD service
If the AD service (container) doesn't match container_exclude but matches container_exclude_metrics or container_exclude_logs
- The AD listener stores that information for the service
- The AD config resolver passes the information to the schedulers through the config
- Each scheduler (logs/checks) checks out the corresponding information before scheduling the config.

pkg/autodiscovery/listeners/common.go

clamoriniere · 2020-04-14T10:05:56Z

pkg/autodiscovery/configresolver/configresolver_test.go

@@ -86,6 +86,16 @@ func (s *dummyService) GetCheckNames() []string {
 	return s.CheckNames
 }

+// IsMetricExcluded returns false
+func (s *dummyService) IsMetricExcluded() bool {


what do you think if we create only once new method but with one parameter. It can ease futur integration

func (s *dummyService) IsExcluded(types containers.FilterType) bool { // ... }

agreed 👍

clamoriniere · 2020-04-16T08:35:35Z

pkg/autodiscovery/listeners/docker.go

+func (s *DockerService) IsExcluded(filter containers.FilterType) bool {
+	switch filter {
+	case containers.Metrics:
+		return s.metricsExcluded
+	case containers.Logs:
+		return s.logsExcluded
+	}
+	return false
+}


why we don't have container.Global in the switch case?

because we don't even create services that are globally excluded in AD, see https://github.com/DataDog/datadog-agent/blob/master/pkg/autodiscovery/listeners/docker.go#L221-L224 for example
it's the current behaviour, I tried to explain it more in the PR description

If the AD service (container) matches container_exclude, the AD listener ignores it and doesn't create the AD service

If the AD service (container) doesn't match container_exclude but matches container_exclude_metrics or container_exclude_logs

The AD listener stores that information for the service

The AD config resolver passes the information to the schedulers through the config

Each scheduler (logs/checks) checks out the corresponding information before scheduling the config.

I had the same question as @clamoriniere. Can you add a single line comment indicating that ? For instance:

// no containers.Global case here because we don't create services that are globally excluded in AD

or something like that? Same goes in integration/config.go.

will do 👍

remeh

Left a couple comments but nice addition!

remeh · 2020-04-17T14:04:11Z

pkg/util/containers/types.go

+type FilterType string
+
+// Global is used to cover both Metrics and Logs filter types
+const Global FilterType = "Global"


It is cosmetic but that looks weird to me to have global variables named containers.Global, containers.Metrics and containers.Logs actually being FilterType, don't you think? Not a big deal though.

I would have been with containers.GlobalFilter, containers.MetricsFilter and containers.LogsFilter I think and called IsExcluded -> HasFilter instead.

good point, will do that thanks!

remeh · 2020-04-17T14:13:56Z

pkg/autodiscovery/listeners/docker.go

+func (s *DockerService) IsExcluded(filter containers.FilterType) bool {
+	switch filter {
+	case containers.Metrics:
+		return s.metricsExcluded
+	case containers.Logs:
+		return s.logsExcluded
+	}
+	return false
+}


I had the same question as @clamoriniere. Can you add a single line comment indicating that ? For instance:

// no containers.Global case here because we don't create services that are globally excluded in AD

or something like that? Same goes in integration/config.go.

remeh

👍

ahmed-mez added component/autodiscovery team/containers labels Apr 9, 2020

ahmed-mez added this to the 7.20.0 milestone Apr 9, 2020

ahmed-mez force-pushed the ahmed-mez/exclude-logs-or-metrics branch from 7d91e50 to f298a48 Compare April 9, 2020 18:27

ahmed-mez marked this pull request as ready for review April 9, 2020 19:18

ahmed-mez requested review from a team as code owners April 9, 2020 19:18

ahmed-mez force-pushed the ahmed-mez/exclude-logs-or-metrics branch 2 times, most recently from 3b6e36b to 9eaa547 Compare April 10, 2020 11:30

clamoriniere reviewed Apr 10, 2020

View reviewed changes

pkg/autodiscovery/listeners/common.go Show resolved Hide resolved

clamoriniere reviewed Apr 14, 2020

View reviewed changes

ahmed-mez force-pushed the ahmed-mez/exclude-logs-or-metrics branch 2 times, most recently from ed336d9 to b1c9e0a Compare April 14, 2020 11:14

clamoriniere reviewed Apr 16, 2020

View reviewed changes

clamoriniere previously approved these changes Apr 16, 2020

View reviewed changes

remeh previously approved these changes Apr 17, 2020

View reviewed changes

exclude logs and metrics separately

4be1394

ahmed-mez dismissed stale reviews from remeh and clamoriniere via 4be1394 April 17, 2020 14:49

ahmed-mez force-pushed the ahmed-mez/exclude-logs-or-metrics branch from b1c9e0a to 4be1394 Compare April 17, 2020 14:49

Merge branch 'master' into ahmed-mez/exclude-logs-or-metrics

71c1db8

remeh approved these changes Apr 17, 2020

View reviewed changes

clamoriniere approved these changes Apr 17, 2020

View reviewed changes

gzussa approved these changes Apr 17, 2020

View reviewed changes

ahmed-mez merged commit 42cc96c into master Apr 17, 2020

ahmed-mez deleted the ahmed-mez/exclude-logs-or-metrics branch April 17, 2020 18:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AD] Exclude logs and metrics separately #5312

[AD] Exclude logs and metrics separately #5312

ahmed-mez commented Apr 9, 2020

clamoriniere Apr 14, 2020 •

edited

Loading

ahmed-mez Apr 14, 2020

clamoriniere Apr 16, 2020

ahmed-mez Apr 16, 2020

remeh Apr 17, 2020

ahmed-mez Apr 17, 2020

remeh left a comment

remeh Apr 17, 2020

ahmed-mez Apr 17, 2020

remeh Apr 17, 2020

remeh left a comment

[AD] Exclude logs and metrics separately #5312

[AD] Exclude logs and metrics separately #5312

Conversation

ahmed-mez commented Apr 9, 2020

What does this PR do?

Motivation

Additional Notes

clamoriniere Apr 14, 2020 • 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

remeh 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

remeh left a comment

Choose a reason for hiding this comment

clamoriniere Apr 14, 2020 •

edited

Loading