-
Notifications
You must be signed in to change notification settings - Fork 814
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
[apptags] Added apptag support to dd-agent #1570
Conversation
@@ -538,3 +553,16 @@ def _should_send_additional_data(self, data_name): | |||
return False | |||
|
|||
|
|||
def _get_jmx_appnames(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that should probably be in jmxfetch.py
6f06b54
to
4dc642c
Compare
@elafarge can you rebase this one on top of the current master please? |
af9220f
to
40c2036
Compare
if self._is_first_run(): | ||
log.info("Hostnames: %s, tags: %s" % (repr(self.metadata_cache), payload['host-tags'])) | ||
# Log the metadata on the first run | ||
if self._is_first_run(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are you changing the indentation here ?
@remh There was absolutely no reason why the indentation has been changed here. I fixed that in a separate commit (I didn't rebase because I don't know if you want me to keep the second commit or not) |
app_tags_list.extend([DD_CHECK_TAG.format(cname) for cname in jmxfetch._get_jmx_appnames()]) | ||
|
||
if 'system' not in payload['host-tags']: | ||
payload['host-tags']['system'] = {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that should be a list, not a dictionary.
Thanks that looks great overall ! |
3eb488a
to
235c612
Compare
I updated the PR with all your suggestions. Waiting for Travis to test it all, I'm re-testing it locally meanwhile. |
@@ -185,6 +186,38 @@ def test_collector(self): | |||
tag = "check:%s" % check.name | |||
assert tag in all_tags, all_tags | |||
|
|||
|
|||
@attr(requires='sysstat') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why does it require sysstat ?
235c612
to
0efdd13
Compare
The agent now sends one "dd_check:appname" tag to dogweb for each running check (including the JMX checks) if the user enables the create_dd_check_tags option. This enables slice/dice by app in the backend. The apptags are sent at regular intervals (every ten minutes by default) The running JMX apps list is retrieved from the {tmp}/jmx_status.yaml file. Indeed, these tags have to be sent from the collectors side, along with host-tags and this file happens to be the only communication channel between JMXFetch.jar and the agent. A test had been added to our test suite specifically for this new feature (checking that the apptag is sent).
0efdd13
to
6593fb2
Compare
@remh : Useless requirements removed. The CI succeeded : https://travis-ci.org/DataDog/dd-agent/builds/62118869 . The Travis Github status is currently "CI build in progress" since I just rebased & pushed in order to remove a couple of extra blank lines. |
Nice job! |
[apptags] Added apptag support to dd-agent
Changes Unknown when pulling 6593fb2 on etienne/per-app-tagging into ** on master**. |
The agent now sends one "dd_check:appname" tag to dogweb for each
running check (including the JMX checks) if the user enables the
create_dd_check_tags option. This enables slice/dice by app in the
backend. The apptags are sent at regular intervals (every ten minutes by
default)
The running JMX apps list is retrieved from the {tmp}/jmx_status.yaml
file. Indeed, these tags have to be sent from the collectors side, along
with host-tags and this file happens to be the only communication
channel between JMXFetch.jar and the agent.
A test had been added to our test suite specifically for this new
feature (checking that the apptag is sent).