[core] add a Configuration system for integrations #441

palazzem · 2018-03-26T11:35:47Z

Overview

Currently, the Tracer doesn't have a configuration system for each single integration. This means that each integration has a different way to set a specific flag/option: sometimes we use application settings (i.e. Django settings) and some others we use attributes (i.e. client.distributed_tracing for requests).

This patch aims to add to the Pin class a configuration object that can be used and retrieved with a simple API. The goal of the patch, is:

provide a way to set global configurations (i.e. for the whole requests library)
provide a way to set a configuration for a specific client (i.e. the specific requests Session)
hide the Pin class, so that a nicer API can be used; the Pin will stays as is because it will be used as underlying mechanism

palazzem · 2018-03-26T12:38:42Z

ddtrace/config.py

This is the main and only API, since it's a plain dictionary that makes everything easy.

palazzem · 2018-03-26T12:40:13Z

ddtrace/config.py

Maybe I should log.debug() something here? the reason why we return a new dictionary is to avoid crashing users application. On the other hand, we fail silently.

Yes worth a log.debug

palazzem · 2018-03-26T12:41:11Z

ddtrace/pin.py

We can't set anything, made exception of _target that is an internal attribute and not used by users.

palazzem · 2018-03-26T12:51:27Z

ddtrace/pin.py

This is a caveat. The current implementation allows to:

set a pin in a global variable (i.e. requests library to set global settings)

when an instance is created from a class that has a Pin attached, the pin reference is copied to the instance, meaning they both point to the same object

because the config object is a mutable value, it means that changing the configuration for an "instance" pin, changes also the global configuration for all other pins

this results in an error

All of this is done to keep the API simple, while reusing the Pin class. Another approach is to continue to expose and suggest the Pin API, that I would rather to hide / remove from the public facing API.

Should we review our integrations to be sure this mechanism wasn't used in any of them?

Yes it's part of the task and it seems we don't do that. Mostly because the caveat is present only for the config object and in the current implementation only the tags attribute is affected (so it's a bugfix in that sense). Indeed, if we change a service name for an instance of the main class, we must use the override() method that clones the Pin.

This behavior protects only the tags and config object from that change, so a bugfix and a new feature. We should be good here.

palazzem · 2018-03-26T12:52:19Z

ddtrace/pin.py

Everytime a Pin is attached onto an object, we set the pin's owner.

Any change it can badly affect the garbage collector?

Yes I'm investigating if it's better to just use the memory address instead of the object pointer.

if we use id(obj) we avoid any kind of reference counting for the GC. This introduces a little overhead:

In [53]: timeit.timeit("id({'a':5})", number=10000) Out[53]: 0.0017991065979003906 In [34]: timeit.timeit("{'a':5}", number=10000) Out[34]: 0.0015370845794677734

that I think it's not relevant. The only caveat is that we have these results for CPython, and it could be different for other interpreters, but at the moment we officially support only CPython.

palazzem · 2018-03-26T12:52:44Z

ddtrace/pin.py

very fast operation, considering our settings have a limited cardinality (<10)

Does it need to be a deepcopy? Don't we risk to have one day some nested config?

This is something we have to consider, but here some benchmarks:

# from Django config, just to simulate a small set of configurations with an empty dict DEFAULTS = {'AGENT_HOSTNAME': 'localhost', 'AGENT_PORT': 8126, 'AUTO_INSTRUMENT': True, 'DEFAULT_DATABASE_PREFIX': '', 'DEFAULT_SERVICE': 'django', 'DISTRIBUTED_TRACING': False, 'ENABLED': True, 'INSTRUMENT_CACHE': True, 'INSTRUMENT_DATABASE': True, 'INSTRUMENT_TEMPLATE': True, 'TAGS': {}, 'TRACER': 'ddtrace.tracer' } # test 1 DEFAULTS.copy() # test 2 copy.deepcopy(DEFAULTS)

And here the results:

test 1: 0.00654911994934082 test 2: 0.2787208557128906

The huge overhead is because of the string copy, so if nested dictionaries are used and we go with a deepcopy, we should take in consideration such overhead.

What do you think?

Yeah that's a big difference. If for now copy is enough for all configuration options we want to provide, no reason to do more.

OK I will add a comment to explain why we use a copy for our use case

palazzem · 2018-03-26T12:53:05Z

ddtrace/pin.py

never expose config to users from the Pin class.

Is putting this underscore _config hiding it from the public API?
Technically it is still part of the __init__ which is publicly exposed ; I'm afraid it mislead people / allow bad things.

yes it's a convention to hide attributes and elements that are prefixed with an underscore. Of course anyone can use it, but if it has an underscore it's considered extremely dangerous. All frameworks do that, though because of the nature of Python, anyone can access to any attribute that is prefixed.

palazzem · 2018-03-26T12:53:35Z

tests/test_config.py

Without the patch for the caveats described above, this test fails.

palazzem · 2018-03-26T12:55:56Z

ddtrace/pin.py

@LotharSee probably I may proxy this value to the self._config['service_name'] so that we don't lose the only setting available in the Pin.

palazzem · 2018-03-27T12:09:07Z

ddtrace/__init__.py

 from .span import Span
 from .tracer import Tracer
-from .configuration import Config
+from .settings import Config


changed the name to avoid long name and obfuscation of config

palazzem · 2018-03-27T13:19:36Z

ddtrace/pin.py

+        self._target = None
+        # keep the configuration attribute internal because the
+        # public API to access it is not the Pin class
+        self._config = _config or {}


We don't need to copy it here, since any operation to use the pin, calls the clone() method once. Indeed, it's possible that we:

initialize the global config with our defaults

first set the Pin in one of our integrations

users update the configuration

when the Pin is used in the integration for the first time (via get_from) it gets cloned

If we copy the dictionary, we'll have always our defaults.

palazzem · 2018-03-27T13:20:07Z

tests/test_instance_config.py

+        cfg['service_name'] = 'metrics'
+        eq_(global_config['service_name'], 'service')
+
+    def test_configuration_copy_upside_down(self):


Covers https://github.com/DataDog/dd-trace-py/pull/441/files#r177420040

palazzem · 2018-03-27T13:31:18Z

tests/test_pin.py

This test before was ok_(got is pin). We now clone the pin once, when get_from is used to safe-protect mutable structure like config and tags. Not doing that means introducing a bug where an instance can update global configurations (and we don't want that).

Fixed using another approach.

… `Pin`

palazzem added core labels Mar 26, 2018

palazzem added this to the 0.12.0 milestone Mar 26, 2018

palazzem commented Mar 26, 2018

View reviewed changes

palazzem requested a review from LotharSee March 26, 2018 12:53

palazzem removed the do-not-merge/WIP label Mar 26, 2018

palazzem commented Mar 26, 2018

View reviewed changes

LotharSee approved these changes Mar 26, 2018

View reviewed changes

palazzem added the do-not-merge/WIP label Mar 27, 2018

palazzem force-pushed the palazzem/config-system branch from 70eb0ae to 7a667c9 Compare March 27, 2018 12:08

palazzem changed the base branch from 0.12-dev to palazzem/global-config March 27, 2018 12:08

palazzem commented Mar 27, 2018

View reviewed changes

palazzem force-pushed the palazzem/config-system branch from 17a2ea8 to cce600a Compare March 27, 2018 13:38

palazzem force-pushed the palazzem/global-config branch from 9fd89a2 to f34941b Compare April 9, 2018 16:00

palazzem removed the do-not-merge/WIP label Apr 9, 2018

palazzem force-pushed the palazzem/config-system branch from cce600a to 51aa8c3 Compare April 9, 2018 17:20

palazzem changed the base branch from palazzem/global-config to 0.12-dev April 9, 2018 17:20

Emanuele Palazzetti added 8 commits April 19, 2018 12:59

[core] refactoring Pin tests

38f99ad

[core] introduce the config attribute for Datadog Pin

6151faf

[core] add the Configuration API

1454e97

[core] integrate instance config with global configurations

1eb097c

[core] improving Pin and configuration implementation

7d06be5

[core] add backward compatibility for pin.service

fa00f0d

[core] regression test to avoid copying the configuration at init time

65fa4ba

[core] check properly the object id when deciding to clone or not the…

885f4f9

… `Pin`

palazzem force-pushed the palazzem/config-system branch from 51aa8c3 to 885f4f9 Compare April 19, 2018 16:59

palazzem merged commit 80748f4 into 0.12-dev Apr 19, 2018

palazzem deleted the palazzem/config-system branch April 19, 2018 20:04

[core] add a Configuration system for integrations #441

[core] add a Configuration system for integrations #441

Uh oh!

Conversation

palazzem commented Mar 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

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!

palazzem Mar 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

palazzem commented Mar 26, 2018 •

edited

Loading

palazzem Mar 26, 2018 •

edited

Loading