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

[autodiscovery] Add ignore_autodiscovery_tags config parameter #4521

Merged
merged 4 commits into from
Nov 29, 2019

Conversation

clamoriniere
Copy link
Contributor

@clamoriniere clamoriniere commented Nov 28, 2019

What does this PR do?

Add ignore_autodiscovery_tags config parameter.

Motivation

In some cases, a check should not receive tags coming from the autodiscovery listeners.
By default ignore_autodiscovery_tags is set to false which didn't change the behavior of the checks.
The first check that will use it is kubernetes_state.

Additional Notes

link to this pr: DataDog/integrations-core#5105

In some cases, a check should not receive tags coming from the
autodiscovery listeners.
By default `ignore_listener_tags` is set to false which doesn't
change the behavior of the checks.
The first check that will use it is `kubernetes_state`.
@clamoriniere clamoriniere force-pushed the clamoriniere/kubernetes-state-check branch from 2984786 to 6788551 Compare November 28, 2019 18:52
@clamoriniere clamoriniere self-assigned this Nov 28, 2019
Copy link
Contributor

@remeh remeh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a comment, otherwise LGTM!

tags, err := svc.GetTags()
if err != nil {
return resolvedConfig, err
tags := []string{}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of having a slice later processed which could be empty in some cases, it may have been cleaner to have:

	for i := 0; i < len(resolvedConfig.Instances); i++ {
		err = resolvedConfig.Instances[i].MergeAdditionalTags(tags)
		if err != nil {
			return resolvedConfig, err
		}
	}

runned only if !tpl.IgnoreListenerTags (moving the svc.GetTags() also there obv).

Something like:

	if !tpl.IgnoreListenerTags {
		tags, err = svc.GetTags()
		if err != nil {
			return resolvedConfig, err
		}
		for i := 0; i < len(resolvedConfig.Instances); i++ {
			err = resolvedConfig.Instances[i].MergeAdditionalTags(tags)
			if err != nil {
				return resolvedConfig, err
			}
		}
	}

This way, all the logic around tags is in the same block in this function. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good idea

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@remeh let me know if you are ok with my change.

@clamoriniere clamoriniere force-pushed the clamoriniere/kubernetes-state-check branch 2 times, most recently from e55c0f6 to ff0019f Compare November 29, 2019 10:23
@clamoriniere clamoriniere force-pushed the clamoriniere/kubernetes-state-check branch from ff0019f to 9f0ac49 Compare November 29, 2019 10:33
remeh
remeh previously approved these changes Nov 29, 2019
Copy link
Contributor

@remeh remeh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@clamoriniere clamoriniere changed the title [autodiscovery] Add ignore_listener_tags config parameter [autodiscovery] Add ignore_autodiscovery_tags config parameter Nov 29, 2019
Co-Authored-By: Haïssam Kaj <hkaj@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants