Skip to content

[falcon] Add distributed tracing #437

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Mar 20, 2018
Merged

[falcon] Add distributed tracing #437

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1f15dee
[falcon] Adds distributed tracing.
willgittoes-dd Mar 8, 2018
d978302
[falcon] Forgot to add test file!
willgittoes-dd Mar 8, 2018
cb0efb0
[falcon] Use iteritems in py2 and py3. Hardcode headers in tests to r…
willgittoes-dd Mar 19, 2018
3f5b0a3
[falcon] Fix lint errors
willgittoes-dd Mar 19, 2018
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
14 changes: 13 additions & 1 deletion ddtrace/contrib/falcon/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,20 @@
from ddtrace import tracer
from ddtrace.contrib.falcon import TraceMiddleware

mw = TraceMiddleware(tracer, 'my-falcon-app')
mw = TraceMiddleware(tracer, 'my-falcon-app', distributed_tracing=True)
falcon.API(middleware=[mw])

You can also use the autopatching functionality:

import falcon
from ddtrace import tracer, patch

patch(falcon=True)

app = falcon.API()

To enable distributed tracing when using autopatching, set the
DATADOG_FALCON_DISTRIBUTED_TRACING environment variable to true.
"""
from ..util import require_modules

Expand Down
12 changes: 11 additions & 1 deletion ddtrace/contrib/falcon/middleware.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,18 @@
import sys

from ddtrace.ext import http as httpx
from ddtrace.propagation.http import HTTPPropagator
from ...compat import iteritems
from ...ext import AppTypes


class TraceMiddleware(object):

def __init__(self, tracer, service="falcon"):
def __init__(self, tracer, service="falcon", distributed_tracing=False):
# store tracing references
self.tracer = tracer
self.service = service
self._distributed_tracing = distributed_tracing

# configure Falcon service
self.tracer.set_service_info(
Expand All @@ -19,6 +22,13 @@ def __init__(self, tracer, service="falcon"):
)

def process_request(self, req, resp):
if self._distributed_tracing:
# Falcon uppercases all header names.
headers = dict((k.lower(), v) for k, v in iteritems(req.headers))
propagator = HTTPPropagator()
context = propagator.extract(headers)
self.tracer.context_provider.activate(context)

span = self.tracer.trace(
"falcon.request",
service=self.service,
Expand Down
5 changes: 4 additions & 1 deletion ddtrace/contrib/falcon/patch.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
from ddtrace import tracer

from .middleware import TraceMiddleware
from ...util import asbool


def patch():
Expand All @@ -21,8 +22,10 @@ def patch():
def traced_init(wrapped, instance, args, kwargs):
mw = kwargs.pop('middleware', [])
service = os.environ.get('DATADOG_SERVICE_NAME') or 'falcon'
distributed_tracing = asbool(os.environ.get(
'DATADOG_FALCON_DISTRIBUTED_TRACING')) or False
Copy link
Contributor Author

@willgittoes-dd willgittoes-dd Mar 8, 2018

Choose a reason for hiding this comment

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

Copied this pattern from somewhere else, but is there a smarter thing I should do?

Copy link

@palazzem palazzem Mar 9, 2018

Choose a reason for hiding this comment

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

I'm rewriting this part because we're polluting the system with too many env vars. We can be smart here.

Copy link
Contributor Author

@willgittoes-dd willgittoes-dd Mar 19, 2018

Choose a reason for hiding this comment

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

Seems like we could just all use the same env var name eg DATADOG_DISTRIBUTED_TRACING. If a user runs multiple distributed services on a single host they can still just set the env vars per-executable themselves. But is a more structured config process in the pipes?

Copy link

@palazzem palazzem Mar 19, 2018

Choose a reason for hiding this comment

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

Actually yes. For instance you may have your intake application that doesn't have distributed tracing for incoming calls (you don't accept it from clients) but you want to activate it for outgoing calls to other services. So it's still better to provide a way to select exactly what distributed tracing you want to enable.

The configuration system will make it easier, but till now I prefer that all integrations are compliant.


mw.insert(0, TraceMiddleware(tracer, service))
mw.insert(0, TraceMiddleware(tracer, service, distributed_tracing))
kwargs['middleware'] = mw

wrapped(*args, **kwargs)
5 changes: 3 additions & 2 deletions tests/contrib/falcon/app/app.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,10 @@
from . import resources


def get_app(tracer=None):
def get_app(tracer=None, distributed_tracing=False):
# initialize a traced Falcon application
middleware = [TraceMiddleware(tracer)] if tracer else []
middleware = [TraceMiddleware(
tracer, distributed_tracing=distributed_tracing)] if tracer else []
app = falcon.API(middleware=middleware)

# add resource routing
Expand Down
55 changes: 55 additions & 0 deletions tests/contrib/falcon/test_distributed_tracing.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
from ddtrace.propagation.http import HTTPPropagator
from ddtrace.ext import errors as errx, http as httpx, AppTypes
from falcon import testing
from nose.tools import eq_, ok_
from tests.test_tracer import get_dummy_tracer

from .app import get_app


class DistributedTracingTestCase(testing.TestCase):
"""Executes tests using the manual instrumentation so a middleware
is explicitly added.
"""

def setUp(self):
super(DistributedTracingTestCase, self).setUp()
self._service = 'falcon'
self.tracer = get_dummy_tracer()
self.api = get_app(tracer=self.tracer, distributed_tracing=True)

def test_distributred_tracing(self):
headers = {
'x-datadog-trace-id': '100',
'x-datadog-parent-id': '42',
}
out = self.simulate_get('/200', headers=headers)
eq_(out.status_code, 200)
eq_(out.content.decode('utf-8'), 'Success')

traces = self.tracer.writer.pop_traces()

eq_(len(traces), 1)
eq_(len(traces[0]), 1)

eq_(traces[0][0].parent_id, 42)
eq_(traces[0][0].trace_id, 100)

def test_distributred_tracing_disabled(self):
Copy link

@palazzem palazzem Mar 19, 2018

Choose a reason for hiding this comment

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

👍

self.tracer = get_dummy_tracer()
self.api = get_app(tracer=self.tracer)
headers = {
'x-datadog-trace-id': '100',
'x-datadog-parent-id': '42',
}
out = self.simulate_get('/200', headers=headers)
eq_(out.status_code, 200)
eq_(out.content.decode('utf-8'), 'Success')

traces = self.tracer.writer.pop_traces()

eq_(len(traces), 1)
eq_(len(traces[0]), 1)

ok_(traces[0][0].parent_id is not 42)
ok_(traces[0][0].trace_id is not 100)
7 changes: 7 additions & 0 deletions tests/contrib/falcon/test_suite.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ def test_404(self):
eq_(span.resource, 'GET 404')
eq_(span.get_tag(httpx.STATUS_CODE), '404')
eq_(span.get_tag(httpx.URL), 'http://falconframework.org/fake_endpoint')
eq_(span.parent_id, None)

def test_exception(self):
try:
Expand All @@ -47,6 +48,7 @@ def test_exception(self):
eq_(span.resource, 'GET tests.contrib.falcon.app.resources.ResourceException')
eq_(span.get_tag(httpx.STATUS_CODE), '500')
eq_(span.get_tag(httpx.URL), 'http://falconframework.org/exception')
eq_(span.parent_id, None)

def test_200(self):
out = self.simulate_get('/200')
Expand All @@ -62,6 +64,7 @@ def test_200(self):
eq_(span.resource, 'GET tests.contrib.falcon.app.resources.Resource200')
eq_(span.get_tag(httpx.STATUS_CODE), '200')
eq_(span.get_tag(httpx.URL), 'http://falconframework.org/200')
eq_(span.parent_id, None)

def test_201(self):
out = self.simulate_post('/201')
Expand All @@ -77,6 +80,7 @@ def test_201(self):
eq_(span.resource, 'POST tests.contrib.falcon.app.resources.Resource201')
eq_(span.get_tag(httpx.STATUS_CODE), '201')
eq_(span.get_tag(httpx.URL), 'http://falconframework.org/201')
eq_(span.parent_id, None)

def test_500(self):
out = self.simulate_get('/500')
Expand All @@ -92,6 +96,7 @@ def test_500(self):
eq_(span.resource, 'GET tests.contrib.falcon.app.resources.Resource500')
eq_(span.get_tag(httpx.STATUS_CODE), '500')
eq_(span.get_tag(httpx.URL), 'http://falconframework.org/500')
eq_(span.parent_id, None)

def test_404_exception(self):
out = self.simulate_get('/not_found')
Expand All @@ -106,6 +111,7 @@ def test_404_exception(self):
eq_(span.resource, 'GET tests.contrib.falcon.app.resources.ResourceNotFound')
eq_(span.get_tag(httpx.STATUS_CODE), '404')
eq_(span.get_tag(httpx.URL), 'http://falconframework.org/not_found')
eq_(span.parent_id, None)

def test_404_exception_no_stacktracer(self):
# it should not have the stacktrace when a 404 exception is raised
Expand All @@ -120,3 +126,4 @@ def test_404_exception_no_stacktracer(self):
eq_(span.service, self._service)
eq_(span.get_tag(httpx.STATUS_CODE), '404')
ok_(span.get_tag(errx.ERROR_TYPE) is None)
eq_(span.parent_id, None)
2 changes: 1 addition & 1 deletion tox.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,7 @@ commands =
flaskcache{012,013}: nosetests {posargs} tests/contrib/flask_cache
flask{010,011,012}: nosetests {posargs} tests/contrib/flask
flask-autopatch{010,011,012}: ddtrace-run nosetests {posargs} tests/contrib/flask_autopatch
falcon{10,11,12}: nosetests {posargs} tests/contrib/falcon/test_middleware.py
falcon{10,11,12}: nosetests {posargs} tests/contrib/falcon/test_middleware.py tests/contrib/falcon/test_distributed_tracing.py
Copy link
Contributor Author

@willgittoes-dd willgittoes-dd Mar 8, 2018

Choose a reason for hiding this comment

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

Also not sure about this. It seemed silly to make a new test directive, but test_distributed doesn't actually depend on the method of patching either.

Copy link

@palazzem palazzem Mar 9, 2018

Choose a reason for hiding this comment

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

can't you simply rewrite this directive in:

falcon{10,11,12}: nosetests {posargs} tests/contrib/falcon

Copy link
Contributor Author

@willgittoes-dd willgittoes-dd Mar 19, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

That tries to execute all the tests, including the autopatch tests (which it seems are usually kept separate by convention?) and misinterpreting the test-cases mixin test_suite.py as actual tests to run as well.

I think if I rename test_suite.py and we're OK with doing the autopatch/middleware tests under the same directive, then it is possible to use falcon{10,11,12}: nosetests {posargs} tests/contrib/falcon

Copy link

@palazzem palazzem Mar 19, 2018

Choose a reason for hiding this comment

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

sorry forgot that we have an autopatch test also for Falcon. It's fine to keep it separated like we do in other frameworks so all good here.

falcon-autopatch{10,11,12}: ddtrace-run nosetests {posargs} tests/contrib/falcon/test_autopatch.py
gevent{11,12}: nosetests {posargs} tests/contrib/gevent
gevent{10}: nosetests {posargs} tests/contrib/gevent
Expand Down