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

add option to specify ec2 tag collection #159

Closed

Conversation

Projects
None yet
2 participants
@mirceal
Copy link
Contributor

mirceal commented Dec 4, 2014

issue opened on chef-datadog: #158

I have 2 opsworks stack and in order for me to differentiate between them I need to be able to collect the ec2 tags associated with the machine.
if I manually set
collect_ec2_tags: no
in /etc/dd-agent/datadog.conf I can see the tags being collected.

When I run the chef recipe via opsworks though, I don't see this parameter being configured in datadog.conf
Looking at the template erg seems that the param is not there.

Added ['datadog']['collect_ec2_tags'] and ['datadog']['api_key'] to the custom json used by opsworks and the api_key is picked up just fine (i see it ending up in config) but the collect_ec2_tags is just ignored.

Need to find a way to add collecting the ec2 tags in config when the agent is installed via chef (or an workaround)

@miketheman

This comment has been minimized.

Copy link
Collaborator

miketheman commented Jan 2, 2015

@mirceal I'm re-reading this, and I think the condition that you are applying will always default to being a truthy value, and render the collect_ec2_tags template line, even if the default is 'no', since the value of the attribute evaluates to a valid String (and not nil).

So it may make sense to provide a default of nil, and comment that this should be a valid String of yes to activate the collection.

@mirceal

This comment has been minimized.

Copy link
Contributor Author

mirceal commented Jan 5, 2015

yeah.
there was a bit of confusion on my side when I needed to figure a default value for this.
I did look at the agent before deciding on 'no':
https://github.com/DataDog/dd-agent/blob/c4118836ede5a044c5ede8a029d63d2986996352/config.py#L313

@mirceal

This comment has been minimized.

Copy link
Contributor Author

mirceal commented Jan 5, 2015

also, as a side note: this isn't as important as I thought it to be went first started with data dog.
because I was a noob and did not really get the way the data dog integrations work, I did not turn the AWS integ off the bat (and tried pulling instance tags using this method). If the AWS integ is turned on, it will do the right thing as far ar pulling the tags and matching the hosts with AWS instances.

@miketheman

This comment has been minimized.

Copy link
Collaborator

miketheman commented Jan 5, 2015

@mirceal That makes total sense - thanks for the link to the source.
I think the if/end condition is now not really useful, since the flag will always be rendered, just with the default of no, which in turn, will be False in the Agent.
So that could be simplified a bit.

Knowing that the use of the AWS Integration solves the problem as a whole, should this PR still be considered? Or does adding it provide potential for further confusion, since it's under the DEPRECATED section anyhow since 2012. DataDog/dd-agent@3073c60

@mirceal

This comment has been minimized.

Copy link
Contributor Author

mirceal commented Jan 5, 2015

this enables using the agent on AWS without enabling the data dog integration (which may be of some value).

@miketheman

This comment has been minimized.

Copy link
Collaborator

miketheman commented Jan 5, 2015

Ok, so if you wanted to cleanit up a touch, and rebase/squash the commits, it could make it in, since the defaults are no and sane. 🏆

@mirceal

This comment has been minimized.

Copy link
Contributor Author

mirceal commented Jan 5, 2015

updated from upstream and pushed to my branch + squashed commits (quite a few of them)
LMK if you want a new branch with only just the change I will send a new pull request

@miketheman

This comment has been minimized.

Copy link
Collaborator

miketheman commented Jan 5, 2015

Yeah - I suspect that you may have missed the rebase target. It's not always simple - you need to get your local master pull from upstream, then rebase/squash your local feature branch from your local master - this would put a single commit at the tip of your feature branch.
The PR would then show only your changes.

@mirceal mirceal force-pushed the mirceal:ml-add-collect-ec2-tags-conf-option branch from c2b480c to fc785ae Jan 5, 2015

Mircea
@mirceal

This comment has been minimized.

Copy link
Contributor Author

mirceal commented Jan 5, 2015

yeah... figured that out. take a look now :)

@mirceal

This comment has been minimized.

Copy link
Contributor Author

mirceal commented Jan 6, 2015

ping

@miketheman

This comment has been minimized.

Copy link
Collaborator

miketheman commented Jan 6, 2015

@mirceal Thanks - looking at the changes, I still don't see the removal if the if/end condition - the existence of anything in https://github.com/DataDog/chef-datadog/pull/159/files#diff-25e5d4a4446ae12a0d6f1162b6160375R37 no/yes/foobar negates the condition, so it's not needed.

@mirceal

This comment has been minimized.

Copy link
Contributor Author

mirceal commented Jan 6, 2015

guess I misunderstood what you were asking.
the condition was there to make it uniform with the way the config was written (i.e. consistency not need made me put it there)

@miketheman

This comment has been minimized.

Copy link
Collaborator

miketheman commented Jan 6, 2015

In that case, the default option in the attributes should be nil - so as to not write anything if the condition is not set.
If someone sets it to explicitly no, then it'll get written.
Does that make sense?

Mircea
@mirceal

This comment has been minimized.

Copy link
Contributor Author

mirceal commented Jan 6, 2015

yes it does make sense :)

@mirceal

This comment has been minimized.

Copy link
Contributor Author

mirceal commented Jan 6, 2015

updated

@miketheman miketheman added this to the Next minor milestone Jan 6, 2015

@miketheman

This comment has been minimized.

Copy link
Collaborator

miketheman commented Jan 6, 2015

Thanks! I'll likely merge soon.

@mirceal

This comment has been minimized.

Copy link
Contributor Author

mirceal commented Jan 6, 2015

thanks & thanks for the patience

@miketheman

This comment has been minimized.

Copy link
Collaborator

miketheman commented Jan 27, 2015

Merged (with a bit of rebasing and docs updates). Thanks!

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.