Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ddtrace/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
from .pin import Pin
from .span import Span
from .tracer import Tracer
from .configuration import Config
from .settings import Config
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed the name to avoid long name and obfuscation of config


__version__ = '0.11.1'

Expand Down
85 changes: 60 additions & 25 deletions ddtrace/pin.py
Original file line number Diff line number Diff line change
@@ -1,42 +1,54 @@
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', '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 {}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

# [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 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)

Expand All @@ -46,15 +58,23 @@ 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)
"""
if hasattr(obj, '__getddpin__'):
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
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@palazzem palazzem Mar 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if pin is not None and pin._target != id(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):
Expand All @@ -63,9 +83,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
Expand All @@ -79,15 +99,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
Expand All @@ -104,25 +125,39 @@ 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 = id(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()

# 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(
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,
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

never expose config to users from the Pin class.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

)

def _send(self):
self.tracer.set_service_info(
service=self.service,
app=self.app,
app_type=self.app_type)
app_type=self.app_type,
)
20 changes: 20 additions & 0 deletions ddtrace/configuration.py → ddtrace/settings.py
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -26,6 +33,19 @@ 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:
log.debug('No configuration found for %s', obj)
return {}

return pin._config

def _add(self, integration, settings):
"""Internal API that registers an integration with given default
settings.
Expand Down
4 changes: 2 additions & 2 deletions tests/test_configuration.py → tests/test_global_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
102 changes: 102 additions & 0 deletions tests/test_instance_config.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
from unittest import TestCase

from nose.tools import eq_, ok_

from ddtrace import config
from ddtrace.pin import Pin


class InstanceConfigTestCase(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_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()
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)

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 used, the given configuration is copied
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')

def test_configuration_copy_upside_down(self):
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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