From 38f99ad8bd2edf3e42c16ab04423cd0504dbe0e5 Mon Sep 17 00:00:00 2001 From: Emanuele Palazzetti Date: Wed, 7 Mar 2018 12:01:46 +0100 Subject: [PATCH 1/8] [core] refactoring Pin tests --- tests/test_pin.py | 167 ++++++++++++++++++++++++++-------------------- 1 file changed, 93 insertions(+), 74 deletions(-) diff --git a/tests/test_pin.py b/tests/test_pin.py index a832530293b..195b449e8ee 100644 --- a/tests/test_pin.py +++ b/tests/test_pin.py @@ -1,76 +1,95 @@ +from unittest import TestCase from ddtrace import Pin -from nose.tools import eq_ - - -def test_pin(): - class A(object): - pass - - a = A() - pin = Pin(service="abc") - pin.onto(a) - - got = Pin.get_from(a) - assert pin.service == got.service - assert pin is got - -def test_cant_pin(): - - class Thing(object): - __slots__ = ['t'] - - t = Thing() - t.t = 1 - - Pin(service="a").onto(t) - -def test_cant_modify(): - p = Pin(service="abc") - try: - p.service = "other" - except AttributeError: - pass - -def test_copy(): - p1 = Pin(service="a", app="app_type", tags={"a":"b"}) - p2 = p1.clone(service="b") - assert p1.service == "a" - assert p2.service == "b" - assert p1.app == "app_type" - assert p2.app == "app_type" - eq_(p1.tags, p2.tags) - assert not (p1.tags is p2.tags) - assert p1.tracer is p2.tracer - -def test_none(): - assert None is Pin.get_from(None) - -def test_repr(): - p = Pin(service="abc") - assert p.service == "abc" - assert 'abc' in str(p) - -def test_override(): - class A(object): - pass - - Pin(service="foo", app="blah").onto(A) - a = A() - Pin.override(a, app="bar") - eq_(Pin.get_from(a).app, "bar") - eq_(Pin.get_from(a).service, "foo") - - b = A() - eq_(Pin.get_from(b).service, "foo") - eq_(Pin.get_from(b).app, "blah") - - -def test_overide_missing(): - class A(): - pass - - a = A() - assert not Pin.get_from(a) - Pin.override(a, service="foo") - assert Pin.get_from(a).service == "foo" +from nose.tools import eq_, ok_, assert_raises + + +class PinTestCase(TestCase): + """TestCase for the `Pin` object that is used when an object is wrapped + with our tracing functionalities. + """ + def setUp(self): + # define a simple class object + class Obj(object): + pass + + self.Obj = Obj + + def test_pin(self): + # ensure a Pin can be attached to an instance + obj = self.Obj() + pin = Pin(service='metrics') + pin.onto(obj) + + got = Pin.get_from(obj) + eq_(got.service, pin.service) + ok_(got is pin) + + def test_cant_pin_with_slots(self): + # ensure a Pin can't be attached if the __slots__ is defined + class Obj(object): + __slots__ = ['value'] + + obj = Obj() + obj.value = 1 + + Pin(service='metrics').onto(obj) + got = Pin.get_from(obj) + ok_(got is None) + + def test_cant_modify(self): + # ensure a Pin is immutable once initialized + pin = Pin(service='metrics') + with assert_raises(AttributeError): + pin.service = 'intake' + + def test_copy(self): + # ensure a Pin is copied when using the clone methods + p1 = Pin(service='metrics', app='flask', tags={'key': 'value'}) + p2 = p1.clone(service='intake') + # values are the same + eq_(p1.service, 'metrics') + eq_(p2.service, 'intake') + eq_(p1.app, 'flask') + eq_(p2.app, 'flask') + # but it's a copy + ok_(p1.tags is not p2.tags) + # of almost everything + ok_(p1.tracer is p2.tracer) + + def test_none(self): + # ensure get_from returns None if a Pin is not available + ok_(Pin.get_from(None) is None) + + def test_repr(self): + # ensure the service name is in the string representation of the Pin + pin = Pin(service='metrics') + ok_('metrics' in str(pin)) + + def test_override(self): + # ensure Override works for an instance object + class A(object): + pass + + Pin(service='metrics', app='flask').onto(A) + a = A() + Pin.override(a, app='django') + eq_(Pin.get_from(a).app, 'django') + eq_(Pin.get_from(a).service, 'metrics') + + b = A() + eq_(Pin.get_from(b).app, 'flask') + eq_(Pin.get_from(b).service, 'metrics') + + def test_override_missing(self): + # ensure overriding an instance doesn't override the Class + class A(object): + pass + + a = A() + ok_(Pin.get_from(a) is None) + Pin.override(a, service='metrics') + eq_(Pin.get_from(a).service, 'metrics') + + b = A() + ok_(Pin.get_from(b) is None) From 6151fafbd257ee2c5a5c3ac6f67bf8ebcba0bcac Mon Sep 17 00:00:00 2001 From: Emanuele Palazzetti Date: Mon, 26 Mar 2018 12:26:31 +0200 Subject: [PATCH 2/8] [core] introduce the `config` attribute for Datadog Pin --- ddtrace/pin.py | 69 ++++++++++++++++++++++++++++++----------------- tests/test_pin.py | 66 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 111 insertions(+), 24 deletions(-) diff --git a/ddtrace/pin.py b/ddtrace/pin.py index 81d811fa3f8..eef96ccdbff 100644 --- a/ddtrace/pin.py +++ b/ddtrace/pin.py @@ -1,42 +1,46 @@ import logging -import wrapt +import wrapt import ddtrace + log = logging.getLogger(__name__) -_DD_PIN_NAME = '_datadog_pin' # To set attributes on wrapt proxy objects use this prefix: # http://wrapt.readthedocs.io/en/latest/wrappers.html +_DD_PIN_NAME = '_datadog_pin' _DD_PIN_PROXY_NAME = '_self_' + _DD_PIN_NAME class Pin(object): - """ Pin (a.k.a Patch INfo) is a small class which is used to - set tracing metadata on a particular traced connection. - This is useful if you wanted to, say, trace two different - database clusters. + """Pin (a.k.a Patch INfo) is a small class which is used to + set tracing metadata on a particular traced connection. + This is useful if you wanted to, say, trace two different + database clusters. >>> conn = sqlite.connect("/tmp/user.db") >>> # Override a pin for a specific connection >>> pin = Pin.override(conn, service="user-db") >>> conn = sqlite.connect("/tmp/image.db") """ + __slots__ = ['app', 'app_type', 'service', 'tags', 'tracer', '_target', '_config', '_initialized'] - __slots__ = ['app', 'app_type', 'service', 'tags', 'tracer', '_initialized'] - - def __init__(self, service, app=None, app_type=None, tags=None, tracer=None): + def __init__(self, service, app=None, app_type=None, tags=None, tracer=None, _config=None): tracer = tracer or ddtrace.tracer self.service = service self.app = app self.app_type = app_type self.tags = tags self.tracer = tracer + self._target = None + # keep the configuration attribute internal because the + # public API to access it is not the Pin class + self._config = _config or {} self._initialized = True def __setattr__(self, name, value): - if hasattr(self, '_initialized'): + if getattr(self, '_initialized', False) and name is not '_target': raise AttributeError("can't mutate a pin, use override() or clone() instead") super(Pin, self).__setattr__(name, value) @@ -46,7 +50,10 @@ def __repr__(self): @staticmethod def get_from(obj): - """ Return the pin associated with the given object. + """Return the pin associated with the given object. If a pin is attached to + `obj` but the instance is not the owner of the pin, a new pin is cloned and + attached. This ensures that a pin inherited from a class is a copy for the new + instance, avoiding that a specific instance overrides other pins values. >>> pin = Pin.get_from(conn) """ @@ -54,7 +61,12 @@ def get_from(obj): return obj.__getddpin__() pin_name = _DD_PIN_PROXY_NAME if isinstance(obj, wrapt.ObjectProxy) else _DD_PIN_NAME - return getattr(obj, pin_name, None) + pin = getattr(obj, pin_name, None) + # detect if the PIN has been inherited from a class + if pin is not None and pin._target is not obj: + pin = pin.clone() + pin.onto(obj) + return pin @classmethod def override(cls, obj, service=None, app=None, app_type=None, tags=None, tracer=None): @@ -63,9 +75,9 @@ def override(cls, obj, service=None, app=None, app_type=None, tags=None, tracer= That's the recommended way to customize an already instrumented client, without losing existing attributes. - >>> conn = sqlite.connect("/tmp/user.db") - >>> # Override a pin for a specific connection - >>> pin = Pin.override(conn, service="user-db") + >>> conn = sqlite.connect("/tmp/user.db") + >>> # Override a pin for a specific connection + >>> Pin.override(conn, service="user-db") """ if not obj: return @@ -79,15 +91,16 @@ def override(cls, obj, service=None, app=None, app_type=None, tags=None, tracer= app=app, app_type=app_type, tags=tags, - tracer=tracer).onto(obj) + tracer=tracer, + ).onto(obj) def enabled(self): - """ Return true if this pin's tracer is enabled. """ + """Return true if this pin's tracer is enabled. """ return bool(self.tracer) and self.tracer.enabled def onto(self, obj, send=True): - """ Patch this pin onto the given object. If send is true, it will also - queue the metadata to be sent to the server. + """Patch this pin onto the given object. If send is true, it will also + queue the metadata to be sent to the server. """ # pinning will also queue the metadata for service submission. this # feels a bit side-effecty, but bc it's async and pretty clearly @@ -104,25 +117,33 @@ def onto(self, obj, send=True): return obj.__setddpin__(self) pin_name = _DD_PIN_PROXY_NAME if isinstance(obj, wrapt.ObjectProxy) else _DD_PIN_NAME + + # set the target reference; any get_from, clones and retarget the new PIN + self._target = obj return setattr(obj, pin_name, self) except AttributeError: log.debug("can't pin onto object. skipping", exc_info=True) def clone(self, service=None, app=None, app_type=None, tags=None, tracer=None): - """ Return a clone of the pin with the given attributes replaced. """ + """Return a clone of the pin with the given attributes replaced.""" + # do a shallow copy of Pin dicts if not tags and self.tags: - # do a shallow copy of the tags if needed. - tags = {k:v for k, v in self.tags.items()} + tags = self.tags.copy() + + config = self._config.copy() return Pin( service=service or self.service, app=app or self.app, app_type=app_type or self.app_type, tags=tags, - tracer=tracer or self.tracer) # no copy of the tracer + tracer=tracer or self.tracer, # do not clone the Tracer + _config=config, + ) def _send(self): self.tracer.set_service_info( service=self.service, app=self.app, - app_type=self.app_type) + app_type=self.app_type, + ) diff --git a/tests/test_pin.py b/tests/test_pin.py index 195b449e8ee..8a40e82d15d 100644 --- a/tests/test_pin.py +++ b/tests/test_pin.py @@ -54,6 +54,7 @@ def test_copy(self): eq_(p2.app, 'flask') # but it's a copy ok_(p1.tags is not p2.tags) + ok_(p1._config is not p2._config) # of almost everything ok_(p1.tracer is p2.tracer) @@ -93,3 +94,68 @@ class A(object): b = A() ok_(Pin.get_from(b) is None) + + def test_pin_config(self): + # ensure `Pin` has a configuration object that can be modified + obj = self.Obj() + Pin.override(obj, service='metrics') + pin = Pin.get_from(obj) + ok_(pin._config is not None) + pin._config['distributed_tracing'] = True + ok_(pin._config['distributed_tracing'] is True) + + def test_pin_config_is_a_copy(self): + # ensure that when a `Pin` is cloned, the config is a copy + obj = self.Obj() + Pin.override(obj, service='metrics') + p1 = Pin.get_from(obj) + ok_(p1._config is not None) + p1._config['distributed_tracing'] = True + + Pin.override(obj, service='intake') + p2 = Pin.get_from(obj) + ok_(p2._config is not None) + p2._config['distributed_tracing'] = False + + ok_(p1._config['distributed_tracing'] is True) + ok_(p2._config['distributed_tracing'] is False) + + def test_pin_does_not_override_global(self): + # ensure that when a `Pin` is created from a class, the specific + # instance doesn't override the global one + class A(object): + pass + + Pin.override(A, service='metrics') + global_pin = Pin.get_from(A) + global_pin._config['distributed_tracing'] = True + + a = A() + pin = Pin.get_from(a) + ok_(pin is not None) + ok_(pin._config['distributed_tracing'] is True) + pin._config['distributed_tracing'] = False + + ok_(global_pin._config['distributed_tracing'] is True) + ok_(pin._config['distributed_tracing'] is False) + + def test_pin_does_not_override_global_with_new_instance(self): + # ensure that when a `Pin` is created from a class, the specific + # instance doesn't override the global one, even if only the + # `onto()` API has been used + class A(object): + pass + + pin = Pin(service='metrics') + pin.onto(A) + global_pin = Pin.get_from(A) + global_pin._config['distributed_tracing'] = True + + a = A() + pin = Pin.get_from(a) + ok_(pin is not None) + ok_(pin._config['distributed_tracing'] is True) + pin._config['distributed_tracing'] = False + + ok_(global_pin._config['distributed_tracing'] is True) + ok_(pin._config['distributed_tracing'] is False) From 1454e97cbfd81f59660ddef94cbc462e1cc402eb Mon Sep 17 00:00:00 2001 From: Emanuele Palazzetti Date: Mon, 26 Mar 2018 14:36:47 +0200 Subject: [PATCH 3/8] [core] add the Configuration API --- ddtrace/config.py | 14 +++++++++++++ tests/test_config.py | 49 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 63 insertions(+) create mode 100644 ddtrace/config.py create mode 100644 tests/test_config.py diff --git a/ddtrace/config.py b/ddtrace/config.py new file mode 100644 index 00000000000..bda6b5adc52 --- /dev/null +++ b/ddtrace/config.py @@ -0,0 +1,14 @@ +from .pin import Pin + + +def get_from(obj): + """Retrieves the configuration for the given object. + Any object that has an attached `Pin` must have a configuration + and if a wrong object is given, an empty `dict` is returned + for safety reasons. + """ + pin = Pin.get_from(obj) + if pin is None: + return {} + + return pin._config diff --git a/tests/test_config.py b/tests/test_config.py new file mode 100644 index 00000000000..550010f6af4 --- /dev/null +++ b/tests/test_config.py @@ -0,0 +1,49 @@ +from unittest import TestCase + +from nose.tools import ok_ + +from ddtrace import config +from ddtrace.pin import Pin + + +class ConfigTestCase(TestCase): + """TestCase for the Configuration API that is used to define + global settings and for each `Pin` instance. + """ + def setUp(self): + class Klass(object): + """Helper class where a Pin is always attached""" + pass + + # define the Class and attach a Pin to it + self.Klass = Klass + Pin(service='metrics').onto(Klass) + + def test_configuration_get_from(self): + # ensure a dictionary is returned + cfg = config.get_from(self.Klass) + ok_(isinstance(cfg, dict)) + + def test_configuration_set(self): + # ensure the configuration can be updated in the Pin + instance = self.Klass() + cfg = config.get_from(instance) + cfg['distributed_tracing'] = True + ok_(config.get_from(instance)['distributed_tracing'] is True) + + def test_global_configuration_inheritance(self): + # ensure global configuration is inherited when it's set + cfg = config.get_from(self.Klass) + cfg['distributed_tracing'] = True + instance = self.Klass() + ok_(config.get_from(instance)['distributed_tracing'] is True) + + def test_configuration_override_instance(self): + # ensure instance configuration doesn't override global settings + global_cfg = config.get_from(self.Klass) + global_cfg['distributed_tracing'] = True + instance = self.Klass() + cfg = config.get_from(instance) + cfg['distributed_tracing'] = False + ok_(config.get_from(self.Klass)['distributed_tracing'] is True) + ok_(config.get_from(instance)['distributed_tracing'] is False) From 1eb097c050913a44bc2c77acad34fe3d82fceef2 Mon Sep 17 00:00:00 2001 From: Emanuele Palazzetti Date: Tue, 27 Mar 2018 13:57:22 +0200 Subject: [PATCH 4/8] [core] integrate instance config with global configurations --- ddtrace/__init__.py | 2 +- ddtrace/config.py | 14 -------------- ddtrace/{configuration.py => settings.py} | 12 ++++++++++++ ...test_configuration.py => test_global_config.py} | 4 ++-- tests/{test_config.py => test_instance_config.py} | 2 +- 5 files changed, 16 insertions(+), 18 deletions(-) delete mode 100644 ddtrace/config.py rename ddtrace/{configuration.py => settings.py} (77%) rename tests/{test_configuration.py => test_global_config.py} (94%) rename tests/{test_config.py => test_instance_config.py} (97%) diff --git a/ddtrace/__init__.py b/ddtrace/__init__.py index 9ea1ac164c8..d17cef90adb 100644 --- a/ddtrace/__init__.py +++ b/ddtrace/__init__.py @@ -2,7 +2,7 @@ from .pin import Pin from .span import Span from .tracer import Tracer -from .configuration import Config +from .settings import Config __version__ = '0.11.1' diff --git a/ddtrace/config.py b/ddtrace/config.py deleted file mode 100644 index bda6b5adc52..00000000000 --- a/ddtrace/config.py +++ /dev/null @@ -1,14 +0,0 @@ -from .pin import Pin - - -def get_from(obj): - """Retrieves the configuration for the given object. - Any object that has an attached `Pin` must have a configuration - and if a wrong object is given, an empty `dict` is returned - for safety reasons. - """ - pin = Pin.get_from(obj) - if pin is None: - return {} - - return pin._config diff --git a/ddtrace/configuration.py b/ddtrace/settings.py similarity index 77% rename from ddtrace/configuration.py rename to ddtrace/settings.py index 51b6f607067..573778d76b5 100644 --- a/ddtrace/configuration.py +++ b/ddtrace/settings.py @@ -26,6 +26,18 @@ def __getattr__(self, name): 'Integration "{}" is not registered in this configuration'.format(e.message) ) + def get_from(self, obj): + """Retrieves the configuration for the given object. + Any object that has an attached `Pin` must have a configuration + and if a wrong object is given, an empty `dict` is returned + for safety reasons. + """ + pin = Pin.get_from(obj) + if pin is None: + return {} + + return pin._config + def _add(self, integration, settings): """Internal API that registers an integration with given default settings. diff --git a/tests/test_configuration.py b/tests/test_global_config.py similarity index 94% rename from tests/test_configuration.py rename to tests/test_global_config.py index 9251124e3b6..ee6769bbc75 100644 --- a/tests/test_configuration.py +++ b/tests/test_global_config.py @@ -3,10 +3,10 @@ from nose.tools import eq_, ok_, assert_raises from ddtrace import config as global_config -from ddtrace.configuration import Config, ConfigException +from ddtrace.settings import Config, ConfigException -class ConfigTestCase(TestCase): +class GlobalConfigTestCase(TestCase): """Test the `Configuration` class that stores integration settings""" def setUp(self): self.config = Config() diff --git a/tests/test_config.py b/tests/test_instance_config.py similarity index 97% rename from tests/test_config.py rename to tests/test_instance_config.py index 550010f6af4..dc8d82b04a8 100644 --- a/tests/test_config.py +++ b/tests/test_instance_config.py @@ -6,7 +6,7 @@ from ddtrace.pin import Pin -class ConfigTestCase(TestCase): +class InstanceConfigTestCase(TestCase): """TestCase for the Configuration API that is used to define global settings and for each `Pin` instance. """ From 7d06be5d691261760e49789cf44859b4d3a0284f Mon Sep 17 00:00:00 2001 From: Emanuele Palazzetti Date: Tue, 27 Mar 2018 14:08:07 +0200 Subject: [PATCH 5/8] [core] improving Pin and configuration implementation --- ddtrace/pin.py | 8 +++++++- ddtrace/settings.py | 8 ++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/ddtrace/pin.py b/ddtrace/pin.py index eef96ccdbff..8314a84490a 100644 --- a/ddtrace/pin.py +++ b/ddtrace/pin.py @@ -119,7 +119,7 @@ def onto(self, obj, send=True): pin_name = _DD_PIN_PROXY_NAME if isinstance(obj, wrapt.ObjectProxy) else _DD_PIN_NAME # set the target reference; any get_from, clones and retarget the new PIN - self._target = obj + self._target = id(obj) return setattr(obj, pin_name, self) except AttributeError: log.debug("can't pin onto object. skipping", exc_info=True) @@ -130,6 +130,12 @@ def clone(self, service=None, app=None, app_type=None, tags=None, tracer=None): if not tags and self.tags: tags = self.tags.copy() + # we use a copy instead of a deepcopy because we expect configurations + # to have only a root level dictionary without nested objects. Using + # deepcopy introduces a big overhead: + # + # copy: 0.00654911994934082 + # deepcopy: 0.2787208557128906 config = self._config.copy() return Pin( diff --git a/ddtrace/settings.py b/ddtrace/settings.py index 573778d76b5..31d0e898984 100644 --- a/ddtrace/settings.py +++ b/ddtrace/settings.py @@ -1,5 +1,12 @@ +import logging + from copy import deepcopy +from .pin import Pin + + +log = logging.getLogger(__name__) + class ConfigException(Exception): """Configuration exception when an integration that is not available @@ -34,6 +41,7 @@ def get_from(self, obj): """ pin = Pin.get_from(obj) if pin is None: + log.debug('No configuration found for %s', obj) return {} return pin._config From fa00f0dfacc1dc44fc7a5e9bbe5c3fc3236a53ea Mon Sep 17 00:00:00 2001 From: Emanuele Palazzetti Date: Tue, 27 Mar 2018 14:32:43 +0200 Subject: [PATCH 6/8] [core] add backward compatibility for `pin.service` --- ddtrace/pin.py | 12 ++++++++++-- tests/test_instance_config.py | 32 +++++++++++++++++++++++++++++++- 2 files changed, 41 insertions(+), 3 deletions(-) diff --git a/ddtrace/pin.py b/ddtrace/pin.py index 8314a84490a..51b4e447279 100644 --- a/ddtrace/pin.py +++ b/ddtrace/pin.py @@ -24,11 +24,10 @@ class Pin(object): >>> pin = Pin.override(conn, service="user-db") >>> conn = sqlite.connect("/tmp/image.db") """ - __slots__ = ['app', 'app_type', 'service', 'tags', 'tracer', '_target', '_config', '_initialized'] + __slots__ = ['app', 'app_type', 'tags', 'tracer', '_target', '_config', '_initialized'] def __init__(self, service, app=None, app_type=None, tags=None, tracer=None, _config=None): tracer = tracer or ddtrace.tracer - self.service = service self.app = app self.app_type = app_type self.tags = tags @@ -37,8 +36,17 @@ def __init__(self, service, app=None, app_type=None, tags=None, tracer=None, _co # keep the configuration attribute internal because the # public API to access it is not the Pin class self._config = _config or {} + # [Backward compatibility]: service argument updates the `Pin` config + self._config['service_name'] = service self._initialized = True + @property + def service(self): + """Backward compatibility: accessing to `pin.service` returns the underlying + configuration value. + """ + return self._config['service_name'] + def __setattr__(self, name, value): if getattr(self, '_initialized', False) and name is not '_target': raise AttributeError("can't mutate a pin, use override() or clone() instead") diff --git a/tests/test_instance_config.py b/tests/test_instance_config.py index dc8d82b04a8..00c39bcc6ec 100644 --- a/tests/test_instance_config.py +++ b/tests/test_instance_config.py @@ -1,6 +1,6 @@ from unittest import TestCase -from nose.tools import ok_ +from nose.tools import eq_, ok_ from ddtrace import config from ddtrace.pin import Pin @@ -47,3 +47,33 @@ def test_configuration_override_instance(self): cfg['distributed_tracing'] = False ok_(config.get_from(self.Klass)['distributed_tracing'] is True) ok_(config.get_from(instance)['distributed_tracing'] is False) + + def test_service_name_for_pin(self): + # ensure for backward compatibility that changing the service + # name via the Pin object also updates integration config + Pin(service='intake').onto(self.Klass) + instance = self.Klass() + cfg = config.get_from(instance) + eq_(cfg['service_name'], 'intake') + + def test_service_attribute_priority(self): + # ensure the `service` arg has highest priority over configuration + # for backward compatibility + global_config = { + 'service_name': 'primary_service', + } + Pin(service='service', _config=global_config).onto(self.Klass) + instance = self.Klass() + cfg = config.get_from(instance) + eq_(cfg['service_name'], 'service') + + def test_configuration_copy(self): + # ensure when a Pin is created, it copies the given configuration + global_config = { + 'service_name': 'service', + } + Pin(service='service', _config=global_config).onto(self.Klass) + instance = self.Klass() + cfg = config.get_from(instance) + cfg['service_name'] = 'metrics' + eq_(global_config['service_name'], 'service') From 65fa4baa73e936fdb3a4ddf424acf48854b37f1b Mon Sep 17 00:00:00 2001 From: Emanuele Palazzetti Date: Tue, 27 Mar 2018 15:18:42 +0200 Subject: [PATCH 7/8] [core] regression test to avoid copying the configuration at init time --- tests/test_instance_config.py | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/tests/test_instance_config.py b/tests/test_instance_config.py index 00c39bcc6ec..99b62d92fa0 100644 --- a/tests/test_instance_config.py +++ b/tests/test_instance_config.py @@ -68,7 +68,7 @@ def test_service_attribute_priority(self): eq_(cfg['service_name'], 'service') def test_configuration_copy(self): - # ensure when a Pin is created, it copies the given configuration + # ensure when a Pin is used, the given configuration is copied global_config = { 'service_name': 'service', } @@ -77,3 +77,18 @@ def test_configuration_copy(self): cfg = config.get_from(instance) cfg['service_name'] = 'metrics' eq_(global_config['service_name'], 'service') + + def test_configuration_copy_upside_down(self): + # ensure when a Pin is created, it does not copy the given configuration + # until it's used for at least once + global_config = { + 'service_name': 'service', + } + Pin(service='service', _config=global_config).onto(self.Klass) + # override the global config: users do that before using the integration + global_config['service_name'] = 'metrics' + # use the Pin via `get_from` + instance = self.Klass() + cfg = config.get_from(instance) + # it should have users updated value + eq_(cfg['service_name'], 'metrics') From 885f4f981c55871d7647b2d24ba03c543a7e701e Mon Sep 17 00:00:00 2001 From: Emanuele Palazzetti Date: Tue, 27 Mar 2018 15:28:46 +0200 Subject: [PATCH 8/8] [core] check properly the object id when deciding to clone or not the `Pin` --- ddtrace/pin.py | 2 +- tests/test_instance_config.py | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/ddtrace/pin.py b/ddtrace/pin.py index 51b4e447279..762f2f65860 100644 --- a/ddtrace/pin.py +++ b/ddtrace/pin.py @@ -71,7 +71,7 @@ def get_from(obj): pin_name = _DD_PIN_PROXY_NAME if isinstance(obj, wrapt.ObjectProxy) else _DD_PIN_NAME pin = getattr(obj, pin_name, None) # detect if the PIN has been inherited from a class - if pin is not None and pin._target is not obj: + if pin is not None and pin._target != id(obj): pin = pin.clone() pin.onto(obj) return pin diff --git a/tests/test_instance_config.py b/tests/test_instance_config.py index 99b62d92fa0..9f765e48f69 100644 --- a/tests/test_instance_config.py +++ b/tests/test_instance_config.py @@ -24,6 +24,14 @@ def test_configuration_get_from(self): cfg = config.get_from(self.Klass) ok_(isinstance(cfg, dict)) + def test_configuration_get_from_twice(self): + # ensure the configuration is the same if `get_from` is used + # in the same instance + instance = self.Klass() + cfg1 = config.get_from(instance) + cfg2 = config.get_from(instance) + ok_(cfg1 is cfg2) + def test_configuration_set(self): # ensure the configuration can be updated in the Pin instance = self.Klass()