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

[apptags] Added apptag support to dd-agent #1570

Merged
merged 1 commit into from May 11, 2015

Conversation

Projects
None yet
4 participants
@elafarge
Copy link
Contributor

elafarge commented Apr 22, 2015

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

@@ -538,3 +553,16 @@ def _should_send_additional_data(self, data_name):
return False


def _get_jmx_appnames(self):

This comment has been minimized.

Copy link
@remh

remh Apr 23, 2015

Member

that should probably be in jmxfetch.py

@@ -28,6 +30,7 @@

FLUSH_LOGGING_PERIOD = 10
FLUSH_LOGGING_INITIAL = 5
APPTAG = 'app:{0}'

This comment has been minimized.

Copy link
@remh

remh Apr 23, 2015

Member

we should probably use something less generic.

dd_check 

maybe ?

@@ -368,7 +371,19 @@ def run(self, checksd=None, start_event=True):
payload['metrics'].extend(self._agent_metrics.check(payload, self.agentConfig,
collect_duration, self.emit_duration))


# If required by the user, let's create the app:xxx host tags
if self.agentConfig['create_app_tags']:

This comment has been minimized.

Copy link
@remh

remh Apr 23, 2015

Member

It's not probably not the right location to send that. We shouldn't run this code at every iteration. It should probably be in this block:
https://github.com/DataDog/dd-agent/blob/etienne/per-app-tagging/checks/collector.py#L461

running_apps = [APPTAG.format(c.name) for c in self.initialized_checks_d]
running_apps.extend([APPTAG.format(cname) for cname in self._get_jmx_appnames()])

if 'host-tags' not in payload:

This comment has been minimized.

Copy link
@remh

remh Apr 23, 2015

Member

this is not needed. the

_build_payload

method ensures that hosts-tags will be there

if 'system' not in payload['host-tags']:
payload['host-tags']['system'] = list()

payload['host-tags']['system'].extend(running_apps)

This comment has been minimized.

Copy link
@remh

remh Apr 23, 2015

Member

so you can simply rewrite this block with

payload['host-tags']['system'] = running_apps

Also, running_apps is not the best name for this list i think.

Something like:

app_tags_list

would be more explicit

@elafarge elafarge force-pushed the etienne/per-app-tagging branch 2 times, most recently from 6f06b54 to 4dc642c Apr 23, 2015

@LeoCavaille

This comment has been minimized.

Copy link
Member

LeoCavaille commented Apr 28, 2015

@elafarge can you rebase this one on top of the current master please?

@elafarge elafarge force-pushed the etienne/per-app-tagging branch 2 times, most recently from af9220f to 40c2036 May 5, 2015

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

This comment has been minimized.

Copy link
@remh

remh May 6, 2015

Member

why are you changing the indentation here ?

@@ -137,13 +143,15 @@ def run(self, checksd=None, start_event=True):
self.run_count += 1
log.debug("Starting collection run #%s" % self.run_count)

if checksd:
self.initialized_checks_d = checksd['initialized_checks'] # is of type {check_name: check}

This comment has been minimized.

Copy link
@remh

remh May 6, 2015

Member

While you're at it can you update the comments ?

checksd['initialized_checks']

is a list of AgentCheck instances

This comment has been minimized.

Copy link
@elafarge

elafarge May 6, 2015

Author Contributor

Done (rebased with the 3rd and last commit)

@elafarge

This comment has been minimized.

Copy link
Contributor Author

elafarge commented May 6, 2015

@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'] = {}

This comment has been minimized.

Copy link
@remh

remh May 6, 2015

Member

I think that should be a list, not a dictionary.

config.py Outdated
@@ -532,6 +536,20 @@ def get_config(parse_args=True, cfg_path=None, options=None):
else:
agentConfig['ssl_certificate'] = agentConfig['ca_certs']

# Getting interval parameters for periodic metrics

This comment has been minimized.

Copy link
@remh

remh May 6, 2015

Member

We don't want that configurable. Can you remove that please ?

This comment has been minimized.

Copy link
@elafarge

elafarge May 6, 2015

Author Contributor

My bad, I thought we did because of this agentConfig.get call : https://github.com/DataDog/dd-agent/blob/master/checks/collector.py#L60 . I'll remove that at once.

@@ -79,6 +79,21 @@ use_mount: no
# histogram_aggregates: max, median, avg, count
# histogram_percentiles: 0.95

# Periodic metadata intervals : select the interval at which you want different
# metadata to be sent by the Agent (in seconds)

This comment has been minimized.

Copy link
@remh

remh May 6, 2015

Member

See comment above

jmxfetch.py Outdated
jmx_status_path = os.path.join(get_jmx_status_path(), "jmx_status.yaml")
if os.path.exists(jmx_status_path):
jmx_checks = yaml.load(file(jmx_status_path)).get('checks', {})
for name, instances in jmx_checks.get('initialized_checks', {}).iteritems():

This comment has been minimized.

Copy link
@remh

remh May 6, 2015

Member
for name in jmx_checks.get('initialized_checks', {}).iterkeys():

This comment has been minimized.

Copy link
@remh

remh May 6, 2015

Member

or even more pythonic:

check_names = [k for k in jmx_checks.get('initialized_checks', {}).iteritems()

(without an explicit loop)

})

# We check that the redis DD_CHECK_TAG is sent in the payload
assert DD_CHECK_TAG.format('redisdb') in payload['host-tags']['system']

This comment has been minimized.

Copy link
@remh

remh May 6, 2015

Member

Shouldn't it be:

dd_check:redisdb

instead ?

This comment has been minimized.

Copy link
@remh

remh May 6, 2015

Member

also please use

self.assertTrue

instead

This comment has been minimized.

Copy link
@elafarge

elafarge May 6, 2015

Author Contributor

I may be wrong but I think it's the same : DD_CHECK_TG='dd_check:{0}'. The advantage is that the tag prefixed is defined only at one place (in collector.py).

I mostly did this because at the time the tag prefix wasn't officially defined and I knew I would forget to update the test if I had to change it. But I can replace that with dd_check:redisdb if you think it's important to check that we didn't change the apptag prefix by accident in our test suite. What should I do ?

@remh

This comment has been minimized.

Copy link
Member

remh commented May 6, 2015

Thanks that looks great overall !
I was actually looking for such a feature while demoing yesterday!

@elafarge elafarge force-pushed the etienne/per-app-tagging branch 2 times, most recently from 3eb488a to 235c612 May 6, 2015

@elafarge

This comment has been minimized.

Copy link
Contributor Author

elafarge commented May 6, 2015

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

This comment has been minimized.

Copy link
@remh

remh May 11, 2015

Member

why does it require sysstat ?

@elafarge elafarge force-pushed the etienne/per-app-tagging branch from 235c612 to 0efdd13 May 11, 2015

Etienne LAFARGE
[apptags] Added apptag support to dd-agent
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).

@elafarge elafarge force-pushed the etienne/per-app-tagging branch from 0efdd13 to 6593fb2 May 11, 2015

@elafarge

This comment has been minimized.

Copy link
Contributor Author

elafarge commented May 11, 2015

@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.

@remh

This comment has been minimized.

Copy link
Member

remh commented May 11, 2015

Nice job!

remh added a commit that referenced this pull request May 11, 2015

Merge pull request #1570 from DataDog/etienne/per-app-tagging
[apptags] Added apptag support to dd-agent

@remh remh merged commit d4e1982 into master May 11, 2015

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@elafarge elafarge deleted the etienne/per-app-tagging branch May 11, 2015

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jan 11, 2017

Coverage Status

Changes Unknown when pulling 6593fb2 on etienne/per-app-tagging into ** on master**.

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.