Skip to content
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.

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

Add exclude lists for Django #670

Merged
merged 19 commits into from
May 27, 2020
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
4 changes: 3 additions & 1 deletion ext/opentelemetry-ext-django/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@

## Unreleased

- Add support for django >= 1.10 (#717)
- Add exclude list for paths and hosts to prevent from tracing
([#670](https://github.com/open-telemetry/opentelemetry-python/pull/670))
- Add support for django >= 1.10 (#717)

## 0.7b1

Expand Down
10 changes: 10 additions & 0 deletions ext/opentelemetry-ext-django/README.rst
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,16 @@ Installation

pip install opentelemetry-ext-django

Configuration
-------------

Exclude lists
*************
Excludes certain hosts and paths from being tracked. Pass in comma delimited string into environment variables.
Host refers to the entire url and path refers to the part of the url after the domain. Host matches the exact string that is given, where as path matches if the url starts with the given excluded path.
toumorokoshi marked this conversation as resolved.
Show resolved Hide resolved

Excluded hosts: OPENTELEMETRY_PYTHON_DJANGO_EXCLUDED_HOSTS
Excluded paths: OPENTELEMETRY_PYTHON_DJANGO_EXCLUDED_PATHS

References
----------
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

from logging import getLogger

from opentelemetry.configuration import Configuration
from opentelemetry.context import attach, detach
from opentelemetry.ext.django.version import __version__
from opentelemetry.ext.wsgi import (
Expand All @@ -23,6 +24,7 @@
)
from opentelemetry.propagators import extract
from opentelemetry.trace import SpanKind, get_tracer
from opentelemetry.util import disable_trace

try:
from django.utils.deprecation import MiddlewareMixin
Expand All @@ -42,6 +44,13 @@ class _DjangoMiddleware(MiddlewareMixin):
_environ_token = "opentelemetry-instrumentor-django.token"
_environ_span_key = "opentelemetry-instrumentor-django.span_key"

_excluded_hosts = Configuration().DJANGO_EXCLUDED_HOSTS or []
_excluded_paths = Configuration().DJANGO_EXCLUDED_PATHS or []
if _excluded_hosts:
_excluded_hosts = str.split(_excluded_hosts, ",")
if _excluded_paths:
_excluded_paths = str.split(_excluded_paths, ",")

def process_view(
self, request, view_func, view_args, view_kwargs
): # pylint: disable=unused-argument
Expand All @@ -53,6 +62,12 @@ def process_view(
# key.lower().replace('_', '-').replace("http-", "", 1): value
# for key, value in request.META.items()
# }
if disable_trace(
request.build_absolute_uri("?"),
self._excluded_hosts,
self._excluded_paths,
):
return

environ = request.META

Expand Down Expand Up @@ -82,6 +97,13 @@ def process_exception(self, request, exception):
# Django can call this method and process_response later. In order
# to avoid __exit__ and detach from being called twice then, the
# respective keys are being removed here.
if disable_trace(
request.build_absolute_uri("?"),
self._excluded_hosts,
self._excluded_paths,
):
return

if self._environ_activation_key in request.META.keys():
request.META[self._environ_activation_key].__exit__(
type(exception),
Expand All @@ -94,6 +116,13 @@ def process_exception(self, request, exception):
request.META.pop(self._environ_token, None)

def process_response(self, request, response):
if disable_trace(
request.build_absolute_uri("?"),
self._excluded_hosts,
self._excluded_paths,
):
return response

if (
self._environ_activation_key in request.META.keys()
and self._environ_span_key in request.META.keys()
Expand Down
7 changes: 6 additions & 1 deletion ext/opentelemetry-ext-django/tests/test_middleware.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,16 +19,20 @@
from django.test import Client
from django.test.utils import setup_test_environment, teardown_test_environment

from opentelemetry.configuration import Configuration
from opentelemetry.ext.django import DjangoInstrumentor
from opentelemetry.test.wsgitestutil import WsgiTestBase
from opentelemetry.trace import SpanKind
from opentelemetry.trace.status import StatusCanonicalCode

from .views import error, traced # pylint: disable=import-error
# pylint: disable=import-error
from .views import error, excluded, excluded2, traced

urlpatterns = [
url(r"^traced/", traced),
url(r"^error/", error),
url(r"^excluded/", excluded),
url(r"^excluded2/", excluded2),
]
_django_instrumentor = DjangoInstrumentor()

Expand All @@ -43,6 +47,7 @@ def setUp(self):
super().setUp()
setup_test_environment()
_django_instrumentor.instrument()
Configuration._reset() # pylint: disable=protected-access

def tearDown(self):
super().tearDown()
Expand Down
8 changes: 8 additions & 0 deletions ext/opentelemetry-ext-django/tests/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,11 @@ def traced(request): # pylint: disable=unused-argument

def error(request): # pylint: disable=unused-argument
raise ValueError("error")


def excluded(request): # pylint: disable=unused-argument
return HttpResponse()


def excluded2(request): # pylint: disable=unused-argument
return HttpResponse()
50 changes: 26 additions & 24 deletions ext/opentelemetry-ext-flask/src/opentelemetry/ext/flask/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -55,11 +55,7 @@ def hello():
from opentelemetry import configuration, context, propagators, trace
from opentelemetry.auto_instrumentation.instrumentor import BaseInstrumentor
from opentelemetry.ext.flask.version import __version__
from opentelemetry.util import (
disable_tracing_hostname,
disable_tracing_path,
time_ns,
)
from opentelemetry.util import disable_trace, time_ns

_logger = getLogger(__name__)

Expand All @@ -69,6 +65,24 @@ def hello():
_ENVIRON_TOKEN = "opentelemetry-flask.token"


def get_excluded_hosts():
hosts = configuration.Configuration().FLASK_EXCLUDED_HOSTS or []
if hosts:
hosts = str.split(hosts, ",")
return hosts


def get_excluded_paths():
paths = configuration.Configuration().FLASK_EXCLUDED_PATHS or []
if paths:
paths = str.split(paths, ",")
return paths


_excluded_hosts = get_excluded_hosts()
Copy link
Member

Choose a reason for hiding this comment

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

I don't believe this will work as intended, or at least not consistently.

If the instrumentation is imported before configuration is set, then these values will be None, and as a result will not exclude any paths nor hosts. This is uncommon for environment variables where things are set before the process starts, but if we start including ways to configure the Configuration object (e.g. set once), this will result in these global variables resolving before anyone can set the configuration.

Here's a short example:

from opentelemetry.ext.flask import FlaskInstrumentor # start of the file, so it loads, and sets the _excluded_hosts global
from opentelemetry import Configuration

Configuration().set(flask_excluded_path="/foo") # this will not honored, as the global was already set.

I think a safer approach would be lazy evaluation: have a getter than populates the value when it's called for the first time. Generally you can feel confident it won't be invoked until the first time it has to match the route, or after instrumentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I understand the use case. Configurations aren't able to be set, they are populated in the first instance that they are used by reading from the environment variables. If the instrumentation is imported, Configuration will just be set the moment that it is called.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently, there is no way to set Configuration pro grammatically, although we might allow this in the future (set it once, and then it becomes immutable). A lazy load getter seems to makes sense as well in this case. I believe we should have this in a separate PR, since that is more about changing the API of Configuration.

Copy link
Member

Choose a reason for hiding this comment

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

@ocelotl might have to weigh in, but I worry not doing this change now will result in a bug later on. I'll at least file an issue here.

_excluded_paths = get_excluded_paths()


def _rewrapped_app(wsgi_app):
def _wrapped_app(environ, start_response):
# We want to measure the time for route matching, etc.
Expand All @@ -78,9 +92,9 @@ def _wrapped_app(environ, start_response):
environ[_ENVIRON_STARTTIME_KEY] = time_ns()

def _start_response(status, response_headers, *args, **kwargs):

if not _disable_trace(flask.request.url):

if not disable_trace(
flask.request.url, _excluded_hosts, _excluded_paths
):
span = flask.request.environ.get(_ENVIRON_SPAN_KEY)

if span:
Expand All @@ -102,7 +116,7 @@ def _start_response(status, response_headers, *args, **kwargs):


def _before_request():
if _disable_trace(flask.request.url):
if disable_trace(flask.request.url, _excluded_hosts, _excluded_paths):
return

environ = flask.request.environ
Expand Down Expand Up @@ -134,6 +148,9 @@ def _before_request():


def _teardown_request(exc):
if disable_trace(flask.request.url, _excluded_hosts, _excluded_paths):
return

activation = flask.request.environ.get(_ENVIRON_ACTIVATION_KEY)
if not activation:
_logger.warning(
Expand Down Expand Up @@ -163,21 +180,6 @@ def __init__(self, *args, **kwargs):
self.teardown_request(_teardown_request)


def _disable_trace(url):
excluded_hosts = configuration.Configuration().FLASK_EXCLUDED_HOSTS
excluded_paths = configuration.Configuration().FLASK_EXCLUDED_PATHS

if excluded_hosts:
excluded_hosts = str.split(excluded_hosts, ",")
if disable_tracing_hostname(url, excluded_hosts):
return True
if excluded_paths:
excluded_paths = str.split(excluded_paths, ",")
if disable_tracing_path(url, excluded_paths):
return True
return False


class FlaskInstrumentor(BaseInstrumentor):
# pylint: disable=protected-access,attribute-defined-outside-init
"""An instrumentor for flask.Flask
Expand Down
108 changes: 0 additions & 108 deletions ext/opentelemetry-ext-flask/tests/base_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,34 +12,12 @@
# See the License for the specific language governing permissions and
# limitations under the License.

from unittest.mock import patch

from flask import request
from werkzeug.test import Client
from werkzeug.wrappers import BaseResponse

from opentelemetry import trace
from opentelemetry.configuration import Configuration


def expected_attributes(override_attributes):
default_attributes = {
"component": "http",
"http.method": "GET",
"http.server_name": "localhost",
"http.scheme": "http",
"host.port": 80,
"http.host": "localhost",
"http.target": "/",
"http.flavor": "1.1",
"http.status_text": "OK",
"http.status_code": 200,
}
for key, val in override_attributes.items():
default_attributes[key] = val
return default_attributes


class InstrumentationTest:
def setUp(self): # pylint: disable=invalid-name
super().setUp() # pylint: disable=no-member
Expand All @@ -66,89 +44,3 @@ def excluded2_endpoint():

# pylint: disable=attribute-defined-outside-init
self.client = Client(self.app, BaseResponse)

# pylint: disable=no-member
def test_only_strings_in_environ(self):
"""
Some WSGI servers (such as Gunicorn) expect keys in the environ object
to be strings

OpenTelemetry should adhere to this convention.
"""
nonstring_keys = set()

def assert_environ():
for key in request.environ:
if not isinstance(key, str):
nonstring_keys.add(key)
return "hi"

self.app.route("/assert_environ")(assert_environ)
self.client.get("/assert_environ")
self.assertEqual(nonstring_keys, set())

def test_simple(self):
expected_attrs = expected_attributes(
{"http.target": "/hello/123", "http.route": "/hello/<int:helloid>"}
)
self.client.get("/hello/123")

span_list = self.memory_exporter.get_finished_spans()
self.assertEqual(len(span_list), 1)
self.assertEqual(span_list[0].name, "_hello_endpoint")
self.assertEqual(span_list[0].kind, trace.SpanKind.SERVER)
self.assertEqual(span_list[0].attributes, expected_attrs)

def test_404(self):
expected_attrs = expected_attributes(
{
"http.method": "POST",
"http.target": "/bye",
"http.status_text": "NOT FOUND",
"http.status_code": 404,
}
)

resp = self.client.post("/bye")
self.assertEqual(404, resp.status_code)
resp.close()
span_list = self.memory_exporter.get_finished_spans()
self.assertEqual(len(span_list), 1)
self.assertEqual(span_list[0].name, "/bye")
self.assertEqual(span_list[0].kind, trace.SpanKind.SERVER)
self.assertEqual(span_list[0].attributes, expected_attrs)

def test_internal_error(self):
expected_attrs = expected_attributes(
{
"http.target": "/hello/500",
"http.route": "/hello/<int:helloid>",
"http.status_text": "INTERNAL SERVER ERROR",
"http.status_code": 500,
}
)
resp = self.client.get("/hello/500")
self.assertEqual(500, resp.status_code)
resp.close()
span_list = self.memory_exporter.get_finished_spans()
self.assertEqual(len(span_list), 1)
self.assertEqual(span_list[0].name, "_hello_endpoint")
self.assertEqual(span_list[0].kind, trace.SpanKind.SERVER)
self.assertEqual(span_list[0].attributes, expected_attrs)

@patch.dict(
"os.environ", # type: ignore
{
"OPENTELEMETRY_PYTHON_FLASK_EXCLUDED_HOSTS": (
"http://localhost/excluded"
),
"OPENTELEMETRY_PYTHON_FLASK_EXCLUDED_PATHS": "excluded2",
},
)
def test_excluded_path(self):
self.client.get("/hello/123")
self.client.get("/excluded")
self.client.get("/excluded2")
span_list = self.memory_exporter.get_finished_spans()
self.assertEqual(len(span_list), 1)
self.assertEqual(span_list[0].name, "_hello_endpoint")
Loading