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

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

Merged
merged 4 commits into from
Sep 19, 2016
Merged

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

merged 4 commits into from
Sep 19, 2016

Conversation

degemer
Copy link
Member

@degemer 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 Compare June 27, 2016 21:22
olivielpeau
olivielpeau reviewed Jul 4, 2016
View reviewed changes
spec/dd-agent_spec.rb
) do |node|
node.set['datadog'] = {
'api_key' => 'somethingnotnil',
'other_dd_urls' => ['app.example.com', 'app.datadoghq.com'],
Copy link
Member

Choose a reason for hiding this comment

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

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

@olivielpeau
Copy link
Member

Added a few nitpicks but this is👌

Let's merge it for 2.5.0

@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 Compare August 16, 2016 20:33
@degemer
[handler] add multiple endpoints support
4819c38 
Pass multiple keys to the handler.
@degemer degemer force-pushed the quentin/multiple-endpoints branch 3 times, most recently from d5f7759 to 777a72e Compare August 23, 2016 15:45
olivielpeau
olivielpeau reviewed Sep 8, 2016
View reviewed changes
recipes/dd-agent.rb
:api_key => node['datadog']['api_key'],
:dd_url => node['datadog']['url']
:api_key => api_keys,
:dd_url => dd_urls
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed. 🎆

@olivielpeau
Copy link
Member

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

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

olivielpeau
olivielpeau reviewed Sep 8, 2016
View reviewed changes
attributes/default.rb
@@ -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
Copy link
Member

Choose a reason for hiding this comment

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

(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)

Copy link
Member Author

Choose a reason for hiding this comment

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

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 Compare September 8, 2016 21:32
@degemer
Copy link
Member Author

degemer commented Sep 8, 2016

💚 CI finally, ready for final review !

olivielpeau
olivielpeau reviewed Sep 9, 2016
View reviewed changes
attributes/default.rb
@@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

should be to send data to multiple accounts?

@olivielpeau
Copy link
Member

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 Compare September 9, 2016 20:28
@degemer
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 Compare September 9, 2016 21:04
olivielpeau
olivielpeau reviewed Sep 13, 2016
View reviewed changes
recipes/dd-handler.rb
extra_endpoints = []
node['datadog']['extra_endpoints'].each do |_, endpoint|
next unless endpoint['enabled']
endpoint.delete('enabled')
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

@olivielpeau olivielpeau Sep 13, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

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)

@olivielpeau
Copy link
Member

olivielpeau commented Sep 13, 2016 •
edited
Loading

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.

@olivielpeau olivielpeau mentioned this pull request Sep 13, 2016
Merged
@degemer
Multiple endpoints: easier configuration
0bea55d 
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 Compare September 13, 2016 21:12
@degemer
[tests] add dd-handler spec test
bbe7268 
Or at least a beginning of tests, with multiple endpoints
@degemer degemer force-pushed the quentin/multiple-endpoints branch from 02e5766 to bbe7268 Compare September 13, 2016 21:20
@degemer
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
olivielpeau approved these changes Sep 15, 2016
View reviewed changes
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! Thanks for the adding these tests! 💚

@olivielpeau olivielpeau merged commit f78252d into master Sep 19, 2016
@olivielpeau olivielpeau deleted the quentin/multiple-endpoints branch September 19, 2016 15:43
@mlcooper mlcooper mentioned this pull request Jun 23, 2017
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@olivielpeau olivielpeau olivielpeau approved these changes

Assignees

@olivielpeau olivielpeau

Labels
feature ready
Projects
None yet
Milestone
2.6.0
Development

Successfully merging this pull request may close these issues.
2 participants
@degemer @olivielpeau