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

Support process-agent configuration in datadog.yaml #511

Merged
merged 1 commit into from Feb 15, 2018

Conversation

conorbranagan
Copy link
Member

@conorbranagan conorbranagan commented Feb 13, 2018

We can do this as of this change being merged: DataDog/datadog-agent#1136

The configuration format is defined in https://github.com/DataDog/datadog-process-agent/blob/master/config/yaml_config.go

# Example: 'image:redis,image:nginx'
default['datadog']['process_agent']['container_blacklist'] = nil
# Whitelist is applied after the blacklist.
default['datadog']['process_agent']['container_whitelist'] = nil
Copy link
Member Author

Choose a reason for hiding this comment

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

Blacklists for containers are handled in the standard Agent 6 way as we share the tagger code.

Copy link
Member

@sunhay sunhay Feb 13, 2018

Choose a reason for hiding this comment

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

We should also at some point clean up the config blacklist code in the process-agent since it looks like it's never used: https://github.com/DataDog/datadog-process-agent/blob/master/config/config.go#L65


# If only collecting containers ('enable_process_agent' is false but docker is available)
# overrides the collection intervals for the full and real-time check.
default['datadog']['process_agent']['container_interval'] = nil
default['datadog']['process_agent']['rtcontainer_interval'] = nil
Copy link
Member Author

Choose a reason for hiding this comment

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

we're setting the defaults here explicitly so we don't have to worry about nil values in the config

Copy link
Member

Choose a reason for hiding this comment

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

alternatively, you could remove the keys that have nil values in the template logic, with something like agent_config[:process_config].reject!{ |k,v| v.nil? }.

Having the defaults defined in the attributes instead of the agent has been a bit of a pain to work with in the past, hence the suggestion

@@ -82,6 +93,20 @@ agent_config = @extra_config.merge({

# log agent options
log_enabled: node['datadog']['enable_logs_agent']

# process agent options
process_dd_url: node['datadog']['process_agent']['url'],
Copy link
Member

Choose a reason for hiding this comment

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

Note: raclette has put there endpoint within the apm_config section. Could we try to be consistent here? Would dd_url under process_config be better? I know this would imply changes to the process-agent, hopefully it's not too late.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a good idea. I have a PR on the process-agent side in DataDog/datadog-process-agent#109 that will hopefully make it into the RC1. I'll update this with the assumption that it will get merged shortly.

@@ -82,6 +93,20 @@ agent_config = @extra_config.merge({

# log agent options
log_enabled: node['datadog']['enable_logs_agent']

Copy link
Member

Choose a reason for hiding this comment

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

we're missing a comma in the line above :)

Copy link
Member

@truthbk truthbk left a comment

Choose a reason for hiding this comment

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

Looks good to me! I'll let @olivielpeau hit the green button! Thank you.

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.

@conorbranagan Made a couple of comments, let me know


# If only collecting containers ('enable_process_agent' is false but docker is available)
# overrides the collection intervals for the full and real-time check.
default['datadog']['process_agent']['container_interval'] = nil
default['datadog']['process_agent']['rtcontainer_interval'] = nil
Copy link
Member

Choose a reason for hiding this comment

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

alternatively, you could remove the keys that have nil values in the template logic, with something like agent_config[:process_config].reject!{ |k,v| v.nil? }.

Having the defaults defined in the attributes instead of the agent has been a bit of a pain to work with in the past, hence the suggestion

@@ -53,34 +52,6 @@ def conf_template_vars
notifies :restart, 'service[datadog-agent]', :delayed unless node['datadog']['agent_start'] == false
end

template process_agent_config_file do
Copy link
Member

Choose a reason for hiding this comment

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

If this code worked for the beta A6 builds, could you keep it (along with the resource datadog_monitor 'process_agent') for now? I can take care of removing it when the Agent 6 is actually GA.

If we remove it now there's no "safe" path to upgrade from A6 beta.9 to rc.1 without breaking the process agent, as no version of this cookbook would fully support both.

I'd like the next version of the cookbook (2.14.0) to support both beta.9 and the RCs, and when we make A6 GA I'll release v3.0.0 of the cookbook with A6 installed by default, and all this legacy code removed.

Also remove process.conf for Agent 6
@conorbranagan
Copy link
Member Author

@olivielpeau Should be good now, thanks

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.

looks good! Merging

@olivielpeau olivielpeau dismissed truthbk’s stale review February 15, 2018 12:30

Jaime approved but github dismissed the approval (!?)

@olivielpeau olivielpeau merged commit 884dd84 into DataDog:master Feb 15, 2018
@truthbk truthbk added this to the 2.14.0 milestone Feb 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants