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

[dd-agent] add multiple enpoints/api_keys conf #317

Merged
merged 4 commits into from Sep 19, 2016

Conversation

Projects
None yet
2 participants
@degemer
Copy link
Member

degemer commented Jun 27, 2016

No description provided.

@degemer degemer added this to the 2.5.0 milestone Jun 27, 2016

@degemer degemer force-pushed the quentin/multiple-endpoints branch from 3be5a56 to de05ec0 Jun 27, 2016

) do |node|
node.set['datadog'] = {
'api_key' => 'somethingnotnil',
'other_dd_urls' => ['app.example.com', 'app.datadoghq.com'],

This comment has been minimized.

Copy link
@olivielpeau

olivielpeau Jul 4, 2016

Member

nitpick since it's not really what's tested here: it'd be better to use the full urls with the protocol, i.e. https://app.example.com

@@ -181,6 +181,10 @@
default['datadog']['histogram_aggregates'] = 'max, median, avg, count'
default['datadog']['histogram_percentiles'] = '0.95'

# Multiple endpoints/api_keys settings

This comment has been minimized.

Copy link
@olivielpeau

olivielpeau Jul 4, 2016

Member

What do you think about specifying in the comment which version of the Agent these parameters will start working with? (we never do it in this file but it would be helpful IMO)

@olivielpeau

This comment has been minimized.

Copy link
Member

olivielpeau commented Jul 4, 2016

Added a few nitpicks but this is👌

Let's merge it for 2.5.0

@olivielpeau olivielpeau added the feature label Jul 8, 2016

@olivielpeau olivielpeau modified the milestones: 2.6.0, 2.5.0 Aug 3, 2016

@degemer degemer force-pushed the quentin/multiple-endpoints branch from de05ec0 to d96d7ea Aug 16, 2016

[handler] add multiple endpoints support
Pass multiple keys to the handler.

@degemer degemer force-pushed the quentin/multiple-endpoints branch 3 times, most recently from d5f7759 to 777a72e Aug 17, 2016

:api_key => node['datadog']['api_key'],
:dd_url => node['datadog']['url']
:api_key => api_keys,
:dd_url => dd_urls

This comment has been minimized.

Copy link
@olivielpeau

olivielpeau Sep 8, 2016

Member

Wouldn't it make sense to update the var names too?

This comment has been minimized.

Copy link
@degemer

degemer Sep 8, 2016

Author Member

Indeed. 🎆

@@ -53,6 +53,11 @@
#
raise "Add a ['datadog']['api_key'] attribute to configure this node's Datadog Agent." if node['datadog'] && node['datadog']['api_key'].nil?

api_keys = node['datadog']['api_key']
dd_urls = node['datadog']['dd_url']

This comment has been minimized.

Copy link
@olivielpeau

olivielpeau Sep 8, 2016

Member

this node attribute was previously named node['datadog']['url']:

  • is the change intentional?
  • if so are we sure that it's something that's only used by us internally? (I think so, but just to be extra sure)

This comment has been minimized.

Copy link
@olivielpeau

olivielpeau Sep 8, 2016

Member

actually it's probably not intentional since you're using node['datadog']['url'] in recipes/dd-handler.rb, so let's fix it :)

This comment has been minimized.

Copy link
@degemer

degemer Sep 8, 2016

Author Member

Typo. 😢

@olivielpeau

This comment has been minimized.

Copy link
Member

olivielpeau commented Sep 8, 2016

Added some comments, let me know when the tests are updated ;)

Overall it looks way better now compared to the first draft 🚀

@@ -21,9 +21,11 @@
# The Datadog api key to associate your agent's data with your organization.
# Can be found here:
# https://app.datadoghq.com/account/settings
# This can be a list of api keys

This comment has been minimized.

Copy link
@olivielpeau

olivielpeau Sep 8, 2016

Member

(just to be more explicit, I'd explain here what a list of api keys does, i.e. that it configures the Agent and the handler to send data to multiple accounts)

This comment has been minimized.

Copy link
@degemer

degemer Sep 8, 2016

Author Member

You're right, it's not clear enough. I also changed list for array, as a list could be understood as a string "apikey1, apikey2".

@degemer degemer force-pushed the quentin/multiple-endpoints branch from 777a72e to c633cb7 Sep 8, 2016

@degemer

This comment has been minimized.

Copy link
Member Author

degemer commented Sep 8, 2016

💚 CI finally, ready for final review !

@@ -21,9 +21,13 @@
# The Datadog api key to associate your agent's data with your organization.
# Can be found here:
# https://app.datadoghq.com/account/settings
# This can be an array of api keys (to configure the agent
# data to send to multiple accounts)

This comment has been minimized.

Copy link
@olivielpeau

olivielpeau Sep 9, 2016

Member

should be to send data to multiple accounts?

@olivielpeau

This comment has been minimized.

Copy link
Member

olivielpeau commented Sep 9, 2016

Added one last comment about a comment, once that's fixed let's :shipit: !

@degemer degemer force-pushed the quentin/multiple-endpoints branch 3 times, most recently from f489e06 to b2e395e Sep 9, 2016

@degemer

This comment has been minimized.

Copy link
Member Author

degemer commented Sep 9, 2016

As we discussed, I updated this PR to use a extra_endpoints @olivielpeau. Sorry for the multiple changes, one more review for you!

@degemer degemer force-pushed the quentin/multiple-endpoints branch from b2e395e to d54c9a2 Sep 9, 2016

extra_endpoints = []
node['datadog']['extra_endpoints'].each do |_, endpoint|
next unless endpoint['enabled']
endpoint.delete('enabled')

This comment has been minimized.

Copy link
@olivielpeau

olivielpeau Sep 13, 2016

Member

This will attempt to mutate node['datadog']['extra_endpoints'] too, which chef doesn't allow from a recipe (it'll cause a compile error).

I think we should either copy the endpoint hash first (keeping in mind that it's not a regular hash but an ImmutableMash if I remember correctly) or "manually" assign the interesting fields of endpoint to a new hash.

Also, unless the resulting values of extra_endpoints var are Mashes I think their keys need to be string literals (:url instead of 'url') since that's how we access the values in the handler.

This comment has been minimized.

Copy link
@olivielpeau

olivielpeau Sep 13, 2016

Member

you can disregard my comment on the Mash thing, it's not a problem since in the handler we copy the config to a Mash since v0.5.0 (see DataDog/chef-handler-datadog#50)

@@ -26,6 +26,13 @@
# Create an application key on the Account Settings page
default['datadog']['application_key'] = nil

# Use this attribute to send data to multiple accounts

This comment has been minimized.

Copy link
@olivielpeau

olivielpeau Sep 13, 2016

Member

nitpick: multiple -> additional? Just want to make extra-clear that these attributes are used to configure accounts on top of the "default" one.

Also, do you think it's worth mentioning that this affects the Agent and the datadog handler>

@olivielpeau

This comment has been minimized.

Copy link
Member

olivielpeau commented Sep 13, 2016

Thanks, the logic looks even nicer now :)

Had 2 comments on the PR. Since the dd-handler part is not covered by the automated tests let's make sure to test it thoroughly with kitchen.

Multiple endpoints: easier configuration
For backward compatibility purpose, we keep url, api_key &
application_key, and add a extra_endpoints attributes to handle the
other endpoints.

@degemer degemer force-pushed the quentin/multiple-endpoints branch from d54c9a2 to 02e5766 Sep 13, 2016

[tests] add dd-handler spec test
Or at least a beginning of tests, with multiple endpoints

@degemer degemer force-pushed the quentin/multiple-endpoints branch from 02e5766 to bbe7268 Sep 13, 2016

@degemer

This comment has been minimized.

Copy link
Member Author

degemer commented Sep 13, 2016

Comments addressed, and a few tests added.
It's a start, because I didn't want to spend too much time on it. We should add more "one day".

@olivielpeau
Copy link
Member

olivielpeau left a comment

Looks good! Thanks for the adding these tests! 💚

@olivielpeau olivielpeau added the ready label Sep 15, 2016

@olivielpeau olivielpeau merged commit f78252d into master Sep 19, 2016

3 checks passed

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

@olivielpeau olivielpeau deleted the quentin/multiple-endpoints branch Sep 19, 2016

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.