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.

Sign up for GitHub

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

Jump to bottom

[trace-agent] Leave the default enabled/disabled behavior to the agent #433

Merged
merged 2 commits into from
May 5, 2017
Merged

[trace-agent] Leave the default enabled/disabled behavior to the agent #433

merged 2 commits into from
May 5, 2017
+19 −9

Conversation

olivielpeau
Copy link
Member

Since agent 5.13.0, the trace agent is enabled by default.

This cookbook should allow explicitly enabling or disabling the trace
agent, however, when the attribute that controls this is not set, the
cookbook should leave it to the agent to decide whether to run the
trace agent.

This change modifies the default value of an attribute. Technically
this is a breaking change that requires a major version bump of the
cookbook, however I think this change actually improves backward
compatibility and can go with a minor version of the cookbook.

Follow-up to #428, cc @ed-flanagan

@olivielpeau
[trace-agent] Leave the default enabled/disabled behavior to the agent
ff9d522 
Since agent 5.13.0, the trace agent is enabled by default.

This cookbook should allow explicitly enabling or disabling the trace
agent, however, when the attribute that controls this is not set, the
cookbook should leave it to the agent to decide whether to run the
trace agent.

This change modifies the default value of an attribute. Technically
this is a breaking change that requires a major version bump of the
cookbook, however I think this change actually improves backward
compatibility and can go with a minor version of the cookbook.
@olivielpeau olivielpeau added this to the 2.10.0 milestone May 4, 2017
@olivielpeau olivielpeau requested a review from degemer May 4, 2017 19:59
degemer
degemer approved these changes May 4, 2017
View reviewed changes
Copy link
Member

@degemer degemer left a comment

Choose a reason for hiding this comment

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

Makes sense.

@ed-flanagan
Copy link
Contributor

Makes sense.
However, when nil (i.e. default), the trace.sampler/trace.receiver templates will not be used. Would the situation of setting those, but not enable_trace_agent make sense?
Perhaps restricting the if/else block to just the apm_enabled directive would be more "flexible", e.g.

<% if node['datadog']['enable_trace_agent'].is_a?(TrueClass) || node['datadog']['enable_trace_agent'].is_a?(FalseClass) -%>
apm_enabled: <%= node['datadog']['enable_trace_agent'] ? 'true' : 'false' %>
<% end -%>

@olivielpeau
[trace-agent] Don't set any value in conf unless attribute is overriden
8b6a445 
Unless specified by the user, all the conf values of the trace-agent
should be left to their defaults and the trace-agent should choose its
own defaults, not the present cookbook.

Custom settings will be applied even if the trace agent is not
explicitly enabled.
@olivielpeau
Copy link
Member Author

@ed-flanagan Thanks for your feedback! The one thing I disliked about setting the trace-agent settings even when it wasn't explicitly enabled was that the cookbook would always set default values on the trace-agent settings.
IMO when the user doesn't define any settings the cookbook shouldn't enforce its own defaults, instead it should let the trace-agent choose its own defaults.

I've pushed a commit that changes that behavior and implements what you suggested, let me know what you think

@ed-flanagan
Copy link
Contributor

Totally agree. If trace-agent already has its own defaults I feel it's better to not redefine them in the cookbook. It's less opinionated and removes the need to update the defaults if/when trace-agent's own defaults change.
I like the new changes! 👍

@olivielpeau
Copy link
Member Author

Thanks @ed-flanagan, merging this then!

@olivielpeau olivielpeau merged commit d9396fc into master May 5, 2017
@olivielpeau olivielpeau deleted the olivielpeau/fix-trace-agent-default-behavior branch May 5, 2017 15:48
@olivielpeau olivielpeau mentioned this pull request May 5, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@degemer degemer degemer approved these changes

Assignees
No one assigned
Labels
optimize
Projects
None yet
Milestone
2.10.0
Development

Successfully merging this pull request may close these issues.
3 participants
@olivielpeau @ed-flanagan @degemer