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

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

Merged
merged 2 commits into from May 5, 2017

Conversation

Projects
None yet
3 participants
@olivielpeau
Copy link
Member

olivielpeau commented May 4, 2017

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

[trace-agent] Leave the default enabled/disabled behavior to the agent
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 the optimize label May 4, 2017

@olivielpeau olivielpeau added this to the 2.10.0 milestone May 4, 2017

@olivielpeau olivielpeau requested a review from degemer May 4, 2017

@degemer

degemer approved these changes May 4, 2017

Copy link
Member

degemer left a comment

Makes sense.

@ed-flanagan

This comment has been minimized.

Copy link
Contributor

ed-flanagan commented May 4, 2017

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 -%>
[trace-agent] Don't set any value in conf unless attribute is overriden
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

This comment has been minimized.

Copy link
Member Author

olivielpeau commented May 4, 2017

@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

This comment has been minimized.

Copy link
Contributor

ed-flanagan commented May 5, 2017

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

This comment has been minimized.

Copy link
Member Author

olivielpeau commented May 5, 2017

Thanks @ed-flanagan, merging this then!

@olivielpeau olivielpeau merged commit d9396fc into master May 5, 2017

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@olivielpeau olivielpeau deleted the olivielpeau/fix-trace-agent-default-behavior branch May 5, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.