-
Notifications
You must be signed in to change notification settings - Fork 657
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
Changes from 16 commits
bb51246
6d89ba8
6078859
fc90ce3
656ba6b
b969c00
983899d
a13a611
0c6aff3
6b2b150
5dbd8c7
f105b45
5a67155
18f1c69
f6d0bb8
aec932e
d7bd164
4ea01b3
80d92bc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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__) | ||
|
||
|
@@ -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() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
|
@@ -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: | ||
|
@@ -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 | ||
|
@@ -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( | ||
|
@@ -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 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be good to add a more descriptive changelog. Can you add a line to describe what the purpose is, or how it should be used?