Skip to content

add send_host_metadata option to disable host metadata collection#373

Merged
xvello merged 2 commits intomasterfrom
xvello/disable_metadata_option
Jul 18, 2017
Merged

add send_host_metadata option to disable host metadata collection#373
xvello merged 2 commits intomasterfrom
xvello/disable_metadata_option

Conversation

@xvello
Copy link
Copy Markdown
Contributor

@xvello xvello commented Jul 6, 2017

What does this PR do?

Allow to disable host metadata collection with the send_host_metadata option. This is needed if dsd6 is running alongside agent5, and might be needed for agent6 in some edge cases (one agent/pod in openshift, dedicated agent for event collection...), although these cases might be tackled differently.

Please discuss whether the common option for agent is legit or whether that should be a dsd-only option.

Docker image datadog/dogstatsd:beta is built with this branch to enable for beta testing.

@xvello xvello requested a review from masci July 6, 2017 10:00
@xvello xvello force-pushed the xvello/disable_metadata_option branch 4 times, most recently from 9761b2e to 009a81b Compare July 6, 2017 13:03
Copy link
Copy Markdown
Contributor

@masci masci 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 to me, left a couple of nits otherwise 👍 .
Can we add a log INFO or maybe WARNING when the metadata collection is turned off for the agent? For dsd is less important but we can do the same.

Comment thread cmd/dogstatsd/main.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: can we call this metaScheduler or something? var collector scheduler is confusing...

Comment thread pkg/collector/dist/datadog.yaml Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Instead of removing this, can we say something like "the host metadata collector cannot be configured and is enabled by default unless all the metadata collection is turned off by setting send_host_metadata to false". (Just an example, feel free to rephrase).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good point. One thing I'm not clear about is whether this option should disable the whole metaScheduler or only the 'host' collector.

In dsd it disables the whole thing as 'host' is the only enabled collector, but what about other collectors for the agent? Should they be forcefully disabled, even if listed in metadata_collectors?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Good question. With the old agent there was no way to distinguish metadata collection (the "host" metadata actually contained a lot of stuff), so the option completely turned off the feature. If we want to do the same, the scheduler shouldn't start. At the moment I don't see any issue with this, since we don't have additional collectors yet, and we might reconsider in the (near) future.

Copy link
Copy Markdown
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.

Just a quick comment on the feature itself: we need to make it extra-clear that turning host metadata collection off should not be done unless there is another agent running with host metadata collection on, on the same host.

Turning host metadata collection off on a "regular" installation can lead to a lot of trouble:

  • duplicate hosts on DD
  • double-billing (hosts are counted twice and billed twice)

We've had cases (in the past, on agent5) of users turning it off and having very bad surprises afterwards, so let's be careful :)

So, more specifically here, I think we should log a WARNING when it's turned off on the agent, and say everywhere that turning it off on an agent can lead to double-billing.

Comment thread Dockerfiles/dogstatsd/alpine/README.md Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

could you make it clear here that host metadata is enabled by default, and that disabling it is needed when running alongside an existing datadog-agent? (currently it's not as clear IMO, but let me know what you think :) )

@xvello xvello force-pushed the xvello/disable_metadata_option branch from 009a81b to 6a1590e Compare July 17, 2017 11:22
@xvello
Copy link
Copy Markdown
Contributor Author

xvello commented Jul 17, 2017

Added the warning and updated the example conf with the consequences that might have.
For consistency, the option disables the whole metadataScheduler for the agent too.
Does that look good?

Copy link
Copy Markdown
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.

a small comment on the description in the example config file, but other than that LGTM! 👍

Comment thread pkg/collector/dist/datadog.yaml Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

the last sentence sounds a bit weird IMO (In that case, only one agent should then)

@xvello xvello force-pushed the xvello/disable_metadata_option branch 2 times, most recently from 8bc5bb0 to fad6d71 Compare July 18, 2017 09:27
Copy link
Copy Markdown
Contributor

@masci masci left a comment

Choose a reason for hiding this comment

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

I think enable_metadata_collection is the right name provided that the option prevents the scheduler to start altogether.

Comment thread pkg/config/config.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should be Datadog.BindEnv("enable_metadata_collection") I think

Comment thread cmd/dogstatsd/main.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

do we need the extra \n?

@xvello xvello force-pushed the xvello/disable_metadata_option branch from 7bc55fd to f8fbdf8 Compare July 18, 2017 11:43
@xvello xvello merged commit 38c7429 into master Jul 18, 2017
@xvello xvello deleted the xvello/disable_metadata_option branch July 18, 2017 12:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants