[Service Discovery] create a template cache to reduce calls to the KV store by hkaj · Pull Request #3060 · DataDog/dd-agent

hkaj · 2016-12-05T18:16:09Z

Note: Please remember to review the Datadog Contribution Guidelines
if you have not yet done so.

What does this PR do?

Create a cache for configuration templates

it keeps auto_configuration templates and templates coming from the KV store
it invalides the KV templates when the agent detects an update in the config store
to reload auto_conf or JMX templates we still need to restart the collector or send it a SIGHUP.

Minor clean ups and test updates

Motivation

Using this cache avoids querying the config store at every container update.

Testing Guidelines

Tests were updated accordingly. A custom docker image using this build is also available at the TODO tag.

truthbk

This looks really good. Just a few minor comments/questions.

truthbk · 2016-12-20T18:27:43Z

utils/checkfiles.py

+                init_tpl = auto_conf.get('init_config') or {}
+                instance_tpl = auto_conf.get('instances', [])
+                if image not in auto_conf_images:
+                    auto_conf_images[image] = [[check_name], [init_tpl], [instance_tpl]]


Wouldn't it end up being less error-prone to have the init and instance configs/templates in a dict with check_name as the key?

you mean like auto_conf_images[(image, check_name)] = [init_tpl, instance_tpl]?

Problem with that is if users have several instances of the same check for the same image (and a customer actually asked how to do that yesterday so it happens). In this case we still need to wrap init_tpl and instance_tpl in lists and end up with the same logic as we have now.

But maybe I didn't understand correctly your suggestion

I think I originally meant auto_conf_images[image] = {'checkname': {'init': init_tpl, 'instances: [intance_tpl_0, instance_tpl_1,...]}}

But then I read on, and I saw it wasn't necessarily a good idea given the current structure/logic. I felt it would make the code a little more maintainable (perhaps). We can maybe just keep in mind for the future (it'd be a pretty big refactor), there's nothing wrong with the current logic.

truthbk · 2016-12-20T18:36:07Z

utils/service_discovery/abstract_config_store.py

+                        log.warning("Conflicting templates in auto_conf for image %s and check %s. "
+                                "Please check your template files." % (image, check_name))
+                        continue
+                    self.auto_conf_templates[image][0].append(check_name)


Likewise, I'm not sure if this wouldn't be better as self.auto_conf_templates[image][check_name] = (init_tpl, instance_tpl)

Oh I get it, we stick to lists for "compatibility" across the board when dealing with the configs. I think we could maybe refactor in the future, but it's a legit choice.

truthbk · 2016-12-20T20:36:48Z

utils/checkfiles.py



-def get_auto_conf(agentConfig, check_name):
+def get_auto_conf(check_name):


I feel like get_auto_conf() and get_auto_config() as names are a bit too close to each other...

truthbk · 2016-12-20T21:06:45Z

utils/service_discovery/abstract_config_store.py

+            # if _issue_read doesn't return anything we still create a key in the cache
+            # so that subsequent reads don't trigger issue_read every time
+            else:
+                self.kv_templates[identifier] = None


I know this is a NOOP but I noticed that we do include in the results templates[CONFIG_FROM_TEMPLATE] = self.kv_templates[identifier] even when self.kv_templates[identifier] is None if we have a hit, but we don't do that with a miss.

Just saying because it occured to me we could refactor this as:

if identifier not in self.kv_templates: tpls = self._issue_read(identifier) if tpls: templates[CONFIG_FROM_TEMPLATE] = tpls self.kv_templates[identifier] = tpls else: self.kv_templates[identifier] = None templates[CONFIG_FROM_TEMPLATE] = self.kv_templates[identifier]

because we always populate after a hit or fetch.

man that snippet is on point! 👌

truthbk · 2016-12-20T21:13:54Z

utils/service_discovery/abstract_config_store.py

-            # lastly fallback to auto_conf
-            return set(self.identifier_to_checks[self._get_image_ident(identifier)])
+            # lastly fallback to legacy name for auto-conf
+            return self.template_cache.get_check_names(self._get_image_ident(identifier))


Question: can we have checks under the legacy name that should also be applied? So a case where identifier and the legacy self._get_image_ident(identifier) would both apply to an image?

Good catch! That's a use case we didn't support before (if user-defined templates were found we skipped auto config) but I added logic for that here and forgot to update this part. Will do it now.

truthbk · 2016-12-20T21:35:52Z

tests/core/test_service_discovery.py

-    @mock.patch.object(AbstractConfigStore, '_issue_read', side_effect=issue_read)
-    def test_read_config_from_store(self, issue_read):
+    @mock.patch.object(_TemplateCache, '_issue_read', side_effect=issue_read)
+    def test_read_config_from_store(self, args):


Big 🙇 for all these awesome tests!

truthbk

Don't see anything wrong with this!

truthbk

Just a couple of nits, but I approve! ;)

truthbk · 2017-01-24T04:39:53Z

agent.py

            self.sd_backend = get_sd_backend(self._agentConfig)

-        if _is_affirmative(self._agentConfig.get('sd_jmx_enable')):
+        if _is_affirmative(self._agentConfig.get('sd_jmx_enable', '')):


This would probably be more readable as: _is_affirmative(self._agentConfig.get('sd_jmx_enable', False)).... Am I missing something?

truthbk · 2017-01-24T16:39:30Z

utils/service_discovery/abstract_config_store.py

+                log.exception('Failed to retrieve a template for %s.' % identifier)
+            # create a key in the cache even if _issue_read doesn't return a tpl
+            # so that subsequent reads don't trigger issue_read
+            self.kv_templates[identifier] = tpls or None


I'm not sure you need the or None? tpls will already be something if self._issue_read(identifier) returns either a list or None, or the None we've set it in the except blocks.

This cache is invalidated when a template change is detected. It doesn't detect changes to the auto_conf folder or JMX checks, only to the KV store. Refreshing configs from files still requires a SIGHUP or a restart.

hkaj · 2017-01-25T16:30:26Z

Thanks @truthbk ! I addressed your comments and squashed the commits. Will merge once the tests pass 🎉

hkaj force-pushed the haissam/sd-cache-kv-tpls branch from 9122643 to 6defada Compare December 5, 2016 18:23

hkaj assigned truthbk Dec 5, 2016

hkaj added checks improvement service discovery labels Dec 5, 2016

hkaj added this to the 5.11.0 milestone Dec 5, 2016

hkaj force-pushed the haissam/sd-cache-kv-tpls branch from 6defada to e3094d3 Compare December 6, 2016 14:34

hkaj removed the checks label Dec 7, 2016

truthbk approved these changes Dec 20, 2016

View reviewed changes

hkaj force-pushed the haissam/sd-cache-kv-tpls branch from e3094d3 to 999f7bd Compare December 21, 2016 14:41

truthbk approved these changes Dec 27, 2016

View reviewed changes

hkaj force-pushed the haissam/sd-cache-kv-tpls branch 8 times, most recently from 7543301 to 2eaab69 Compare January 10, 2017 00:05

truthbk approved these changes Jan 24, 2017

View reviewed changes

masci modified the milestones: 5.11.0, 5.12.0 Jan 24, 2017

[SD] create a template cache to reduce calls to the KV store

13d8a24

This cache is invalidated when a template change is detected. It doesn't detect changes to the auto_conf folder or JMX checks, only to the KV store. Refreshing configs from files still requires a SIGHUP or a restart.

hkaj force-pushed the haissam/sd-cache-kv-tpls branch from 2eaab69 to 13d8a24 Compare January 25, 2017 16:28

hkaj merged commit df1a506 into master Jan 25, 2017

hkaj deleted the haissam/sd-cache-kv-tpls branch January 25, 2017 16:58



		def get_auto_conf(agentConfig, check_name):
		def get_auto_conf(check_name):

Conversation

hkaj commented Dec 5, 2016

What does this PR do?

Motivation

Testing Guidelines

Uh oh!

truthbk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

truthbk left a comment

Choose a reason for hiding this comment

Uh oh!

truthbk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hkaj commented Jan 25, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants