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

[forwarder] Allow configuration of multiple endpoints #352

Merged
merged 5 commits into from
Jun 28, 2017

Conversation

olivielpeau
Copy link
Member

What does this PR do?

Allows configuration of multiple endpoints (implements related RFC).

Also, adds some logging when the forwarder starts.

Motivation

Support multiple endpoints. Needed for the agents running on our staging and prod infras.

Additional Notes

The endpoints configuration is not documented in the example datadog.yaml since I don't think we want to advertise this feature. To be confirmed though.

Also, adds some logging when the forwarder starts
Copy link
Contributor

@gmmeyer gmmeyer left a comment

Choose a reason for hiding this comment

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

I added two quick comments, let me know what you think about them. I'm prepared to approve as is, though.

return keysPerDomain, err
}

for domain, apiKeys := range additionalEndpoints {
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if there is a domain without any keys? Should that throw an error or should we just be fine with it?

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 should probably log an error and discard that domain, I'll update the PR


for domain, apiKeys := range additionalEndpoints {
if _, ok := keysPerDomain[domain]; ok {
for _, apiKey := range apiKeys {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should dedupe API Keys. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

makes sense, I'll do that

Copy link
Member

@hush-hush hush-hush left a comment

Choose a reason for hiding this comment

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

I agree with @gmmeyer on his 2 points. Feel free to merge after addressing those points.

Copy link
Contributor

@gmmeyer gmmeyer left a comment

Choose a reason for hiding this comment

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

lgtm

@olivielpeau olivielpeau merged commit 3684dc2 into master Jun 28, 2017
@olivielpeau olivielpeau deleted the olivielpeau/multiple-endpoints-config branch June 28, 2017 18:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants