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

Deprecate undocumented process_agent_config.host_ips in favor of process_config.docker_host_ips #7694

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

lbunschoten
Copy link

What does this PR do?

Fixes #5081

  • Get rid of the class cast warning noise when trying to read the process_agent_config.host_ips property
  • Deprecated process_agent_config.host_ips in favor of process_config.docker_host_ips. The old variables naming scheme is incosistent with other properties for the process agent. The old property is still supported. When both properties are used, the new one will take precedence.
  • Added missing documentation for this property
  • Added sane default (list of empty string)
  • This bit of code was hardly tested, so I've added a couple of tests

Motivation

I am opening this pull request to address the issue described in #5081. The default configuration for the process agent causes a warning to be logged every couple of hours. This is just noise, that can lead to confusion. The main reason seems to be an incorrect default (nil vs empty list)

Additional Notes

This is my first PR for the datadog-agent. I tried to follow allow the guidelines, but please do let me know if I have missed something.

Describe your test plan

  • The automatic tests should pass
  • Set the new property in the config and verify that the property is picked up.
  • Set both the new and the deprecated property in the config. Verify that the new property is picked up.
  • Set the deprecated property in the config. Verify that the deprecated property is picked up.
  • Using the default process_agent config, verify that the log line 2020-03-05 05:45:40 UTC | PROCESS | WARN | (pkg/config/viper.go:168 in GetStringSlice) | failed to get configuration value for key "process_agent_config.host_ips": unable to cast <nil> of type <nil> to []string is not printed

@lbunschoten lbunschoten requested review from a team as code owners March 21, 2021 14:23
@bits-bot
Copy link
Collaborator

bits-bot commented Mar 21, 2021

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@ruthnaebeck ruthnaebeck left a comment

Choose a reason for hiding this comment

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

Docs review

@lbunschoten lbunschoten requested a review from ruthnaebeck March 25, 2021 19:20
Copy link
Contributor

@ruthnaebeck ruthnaebeck left a comment

Choose a reason for hiding this comment

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

👍 for docs

@lbunschoten
Copy link
Author

Would anyone from the team also be available to review the actual code? There are quite some +1s on the related issue, so I'd appreciate it if we could work towards fixing this

@djmitche djmitche added this to the Triage milestone Aug 13, 2021
@olivielpeau olivielpeau requested a review from a team February 24, 2022 09:45
@olivielpeau olivielpeau added the [deprecated] team/processes DEPRECATED: please use team/container-intake label Feb 24, 2022
Copy link
Member

@olivielpeau olivielpeau left a comment

Choose a reason for hiding this comment

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

Requesting a review from @DataDog/processes to review the code changes in the context of the recent improvements to the process agent config options (and whether it makes sense to rebase this PR on top of them).

@mbotarro
Copy link
Contributor

Thanks for pointing it out, @olivielpeau. In fact we should rebase this PR on top of the config refactor that is introduced on process-agent 7.35.0. I'll leave some comments on the PR about what should be changed.

@@ -583,6 +583,7 @@ func InitConfig(config Config) {
}

config.BindEnv("process_config.process_dd_url", "") //nolint:errcheck
config.BindEnvAndSetDefault("process_config.docker_host_ips", []string{})
Copy link
Contributor

Choose a reason for hiding this comment

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

We should move the setting declaration to the setupProcess in the new pkg/config/process.go file and use procBindEnv. This helper function will automatically bind the env vars DD_PROCESS_CONFIG_DOCKER_HOST_IPS and DD_PROCESS_AGENT_DOCKER_HOST_IPS to it.

@lbunschoten
Copy link
Author

Nice to see that there is some sudden interest in this PR :) It has been a while since I've created it. I don't think I'll be able to help out much anymore, since I haven't worked with the datadog-agent lately and can't tell if this is still an issue. But, feel free to continue with it and still merge it in (once the conflicts and so on are resolved) :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[deprecated] team/processes DEPRECATED: please use team/container-intake
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Keep bumping warning logs for reading process_agent_config.host_ips
7 participants