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

Change exclude lists to just exclude_urls, add tests for flask and django #872

Merged
merged 5 commits into from
Jul 7, 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
2 changes: 2 additions & 0 deletions ext/opentelemetry-ext-django/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

## Unreleased

- Use one general exclude list instead of two ([#872](https://github.com/open-telemetry/opentelemetry-python/pull/872))

## 0.8b0

Released 2020-05-27
Expand Down
12 changes: 8 additions & 4 deletions ext/opentelemetry-ext-django/README.rst
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,15 @@ 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.
To exclude certain URLs from being tracked, set the environment variable ``OPENTELEMETRY_PYTHON_DJANGO_EXCLUDED_URLS`` with comma delimited regexes representing which URLs to exclude.
Copy link
Member

Choose a reason for hiding this comment

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

devil's advocate, but: what if the url I want to exclude has a comma?

Copy link
Member Author

Choose a reason for hiding this comment

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

good question, hadn't thought of this. Not really sure of a good solution, I guess we could make the env variable an array or give some way to escape commas in the URL but they don't sound enticing. any ideas?

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like a pretty specific case to me. I'm okay with the solution as is. Better to cover the majority of cases than to block something by fixating on a rare case.

Copy link
Contributor

@AndrewAXue AndrewAXue Jul 3, 2020

Choose a reason for hiding this comment

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

I think the only way to truly fix this is to make the env variable into an array. Any delimiter can be present in a url of the form

url.com/point?payload="delimiter"


Excluded hosts: OPENTELEMETRY_PYTHON_DJANGO_EXCLUDED_HOSTS
Excluded paths: OPENTELEMETRY_PYTHON_DJANGO_EXCLUDED_PATHS
For example,

::

export OPENTELEMETRY_PYTHON_DJANGO_EXCLUDED_URLS="client/.*/info,healthcheck"

will exclude requests such as ``https://site/client/123/info`` and ``https://site/xyz/healthcheck``.

References
----------
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
)
from opentelemetry.propagators import extract
from opentelemetry.trace import SpanKind, get_tracer
from opentelemetry.util import disable_trace
from opentelemetry.util import ExcludeList

try:
from django.utils.deprecation import MiddlewareMixin
Expand All @@ -44,12 +44,11 @@ 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, ",")
_excluded_urls = Configuration().DJANGO_EXCLUDED_URLS or []
if _excluded_urls:
_excluded_urls = ExcludeList(str.split(_excluded_urls, ","))
else:
_excluded_urls = ExcludeList(_excluded_urls)

def process_view(
self, request, view_func, view_args, view_kwargs
Expand All @@ -62,11 +61,7 @@ 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,
):
if self._excluded_urls.url_disabled(request.build_absolute_uri("?")):
return

environ = request.META
Expand Down Expand Up @@ -97,11 +92,7 @@ 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,
):
if self._excluded_urls.url_disabled(request.build_absolute_uri("?")):
return

if self._environ_activation_key in request.META.keys():
Expand All @@ -116,11 +107,7 @@ 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,
):
if self._excluded_urls.url_disabled(request.build_absolute_uri("?")):
return response

if (
Expand Down
31 changes: 28 additions & 3 deletions ext/opentelemetry-ext-django/tests/test_middleware.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
# limitations under the License.

from sys import modules
from unittest.mock import patch

from django.conf import settings
from django.conf.urls import url
Expand All @@ -24,15 +25,17 @@
from opentelemetry.test.wsgitestutil import WsgiTestBase
from opentelemetry.trace import SpanKind
from opentelemetry.trace.status import StatusCanonicalCode
from opentelemetry.util import ExcludeList

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

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

Expand Down Expand Up @@ -111,3 +114,25 @@ def test_error(self):
span.attributes["http.url"], "http://testserver/error/"
)
self.assertEqual(span.attributes["http.scheme"], "http")

@patch(
"opentelemetry.ext.django.middleware._DjangoMiddleware._excluded_urls",
ExcludeList(["http://testserver/excluded_arg/123", "excluded_noarg"]),
cnnradams marked this conversation as resolved.
Show resolved Hide resolved
)
def test_exclude_lists(self):
client = Client()
client.get("/excluded_arg/123")
cnnradams marked this conversation as resolved.
Show resolved Hide resolved
span_list = self.memory_exporter.get_finished_spans()
self.assertEqual(len(span_list), 0)

client.get("/excluded_arg/125")
span_list = self.memory_exporter.get_finished_spans()
self.assertEqual(len(span_list), 1)

client.get("/excluded_noarg/")
span_list = self.memory_exporter.get_finished_spans()
self.assertEqual(len(span_list), 1)

client.get("/excluded_noarg2/")
span_list = self.memory_exporter.get_finished_spans()
self.assertEqual(len(span_list), 1)
6 changes: 5 additions & 1 deletion ext/opentelemetry-ext-django/tests/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,5 +13,9 @@ def excluded(request): # pylint: disable=unused-argument
return HttpResponse()


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


def excluded_noarg2(request): # pylint: disable=unused-argument
return HttpResponse()
2 changes: 2 additions & 0 deletions ext/opentelemetry-ext-flask/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

## Unreleased

- Use one general exclude list instead of two ([#872](https://github.com/open-telemetry/opentelemetry-python/pull/872))

## 0.7b1

Released 2020-05-12
Expand Down
11 changes: 7 additions & 4 deletions ext/opentelemetry-ext-flask/README.rst
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,15 @@ 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.
To exclude certain URLs from being tracked, set the environment variable ``OPENTELEMETRY_PYTHON_FLASK_EXCLUDED_URLS`` with comma delimited regexes representing which URLs to exclude.

Excluded hosts: OPENTELEMETRY_PYTHON_FLASK_EXCLUDED_HOSTS
Excluded paths: OPENTELEMETRY_PYTHON_FLASK_EXCLUDED_PATHS
For example,

::

export OPENTELEMETRY_PYTHON_FLASK_EXCLUDED_URLS="client/.*/info,healthcheck"

will exclude requests such as ``https://site/client/123/info`` and ``https://site/xyz/healthcheck``.

References
----------
Expand Down
30 changes: 10 additions & 20 deletions ext/opentelemetry-ext-flask/src/opentelemetry/ext/flask/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ def hello():
from opentelemetry import configuration, context, propagators, trace
from opentelemetry.ext.flask.version import __version__
from opentelemetry.instrumentation.instrumentor import BaseInstrumentor
from opentelemetry.util import disable_trace, time_ns
from opentelemetry.util import ExcludeList, time_ns

_logger = getLogger(__name__)

Expand All @@ -65,22 +65,14 @@ 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_urls():
urls = configuration.Configuration().FLASK_EXCLUDED_URLS or []
if urls:
urls = str.split(urls, ",")
return ExcludeList(urls)


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


_excluded_hosts = get_excluded_hosts()
_excluded_paths = get_excluded_paths()
_excluded_urls = get_excluded_urls()


def _rewrapped_app(wsgi_app):
Expand All @@ -92,9 +84,7 @@ 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, _excluded_hosts, _excluded_paths
):
if not _excluded_urls.url_disabled(flask.request.url):
span = flask.request.environ.get(_ENVIRON_SPAN_KEY)

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


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

environ = flask.request.environ
Expand Down Expand Up @@ -148,7 +138,7 @@ def _before_request():


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

activation = flask.request.environ.get(_ENVIRON_ACTIVATION_KEY)
Expand Down
24 changes: 24 additions & 0 deletions ext/opentelemetry-ext-flask/tests/test_programmatic.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,15 @@
# See the License for the specific language governing permissions and
# limitations under the License.

from unittest.mock import patch

from flask import Flask, request

from opentelemetry import trace
from opentelemetry.ext.flask import FlaskInstrumentor
from opentelemetry.test.test_base import TestBase
from opentelemetry.test.wsgitestutil import WsgiTestBase
from opentelemetry.util import ExcludeList

# pylint: disable=import-error
from .base_test import InstrumentationTest
Expand Down Expand Up @@ -139,3 +142,24 @@ def test_internal_error(self):
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(
"opentelemetry.ext.flask._excluded_urls",
ExcludeList(["http://localhost/excluded_arg/123", "excluded_noarg"]),
)
def test_exclude_lists(self):
self.client.get("/excluded_arg/123")
cnnradams marked this conversation as resolved.
Show resolved Hide resolved
span_list = self.memory_exporter.get_finished_spans()
self.assertEqual(len(span_list), 0)

self.client.get("/excluded_arg/125")
span_list = self.memory_exporter.get_finished_spans()
self.assertEqual(len(span_list), 1)

self.client.get("/excluded_noarg")
span_list = self.memory_exporter.get_finished_spans()
self.assertEqual(len(span_list), 1)

self.client.get("/excluded_noarg2")
span_list = self.memory_exporter.get_finished_spans()
self.assertEqual(len(span_list), 1)
2 changes: 2 additions & 0 deletions ext/opentelemetry-ext-pyramid/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

## Unreleased

- Use one general exclude list instead of two ([#872](https://github.com/open-telemetry/opentelemetry-python/pull/872))

## 0.9b0

Released 2020-06-10
Expand Down
11 changes: 11 additions & 0 deletions ext/opentelemetry-ext-pyramid/README.rst
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,17 @@ Installation

pip install opentelemetry-ext-pyramid

Exclude lists
*************
To exclude certain URLs from being tracked, set the environment variable ``OPENTELEMETRY_PYTHON_PYRAMID_EXCLUDED_URLS`` with comma delimited regexes representing which URLs to exclude.

For example,

::

export OPENTELEMETRY_PYTHON_PYRAMID_EXCLUDED_URLS="client/.*/info,healthcheck"

will exclude requests such as ``https://site/client/123/info`` and ``https://site/xyz/healthcheck``.

References
----------
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
import opentelemetry.ext.wsgi as otel_wsgi
from opentelemetry import configuration, context, propagators, trace
from opentelemetry.ext.pyramid.version import __version__
from opentelemetry.util import disable_trace, time_ns
from opentelemetry.util import ExcludeList, time_ns

TWEEN_NAME = "opentelemetry.ext.pyramid.trace_tween_factory"
SETTING_TRACE_ENABLED = "opentelemetry-pyramid.trace_enabled"
Expand All @@ -22,22 +22,14 @@
_logger = getLogger(__name__)


def get_excluded_hosts():
hosts = configuration.Configuration().PYRAMID_EXCLUDED_HOSTS or []
if hosts:
hosts = str.split(hosts, ",")
return hosts
def get_excluded_urls():
urls = configuration.Configuration().PYRAMID_EXCLUDED_URLS or []
if urls:
urls = str.split(urls, ",")
return ExcludeList(urls)


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


_excluded_hosts = get_excluded_hosts()
_excluded_paths = get_excluded_paths()
_excluded_urls = get_excluded_urls()


def includeme(config):
Expand Down Expand Up @@ -119,7 +111,7 @@ def disabled_tween(request):

# make a request tracing function
def trace_tween(request):
if disable_trace(request.url, _excluded_hosts, _excluded_paths):
if _excluded_urls.url_disabled(request.url):
request.environ[_ENVIRON_ENABLED_KEY] = False
# short-circuit when we don't want to trace anything
return handler(request)
Expand Down
9 changes: 3 additions & 6 deletions ext/opentelemetry-ext-pyramid/tests/test_programmatic.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
from opentelemetry.ext.pyramid import PyramidInstrumentor
from opentelemetry.test.test_base import TestBase
from opentelemetry.test.wsgitestutil import WsgiTestBase
from opentelemetry.util import ExcludeList

# pylint: disable=import-error
from .pyramid_base_test import InstrumentationTest
Expand Down Expand Up @@ -169,12 +170,8 @@ def test_warnings(self, mock_logger):
self.assertEqual(mock_logger.warning.called, True)

@patch(
"opentelemetry.ext.pyramid.callbacks._excluded_hosts",
["http://localhost/excluded_arg/123"],
)
@patch(
"opentelemetry.ext.pyramid.callbacks._excluded_paths",
["excluded_noarg"],
"opentelemetry.ext.pyramid.callbacks._excluded_urls",
ExcludeList(["http://localhost/excluded_arg/123", "excluded_noarg"]),
)
def test_exclude_lists(self):
self.client.get("/excluded_arg/123")
Expand Down
Loading