Add exclude lists for flask integration #630

lzchen · 2020-04-29T22:53:03Z

Leverage global configurations to allow users to specify paths and hosts that they do not want to trace within their Flask applications.

OPENTELEMETRY_PYTHON_FLASK_EXCLUDED_HOSTS and OPENTELEMETRY_PYTHON_FLASK_EXCLUDED_PATHS are the env variables used. Use a comma delimited string to represent seperate hosts/paths to blacklist.

TODO:

Perhaps allow users to pro-grammatically configure these (through code, maybe Configurations should be modifiable through code as well)
Related to [make suppress_instrumentation emit a defaultspan #581]. Perhaps we should emit a default span instead of not patching. I don't really see a benefit for this though.

ocelotl · 2020-04-29T23:08:21Z

I think this fixes #565

lzchen · 2020-04-29T23:30:51Z

@ocelotl Oh yes, thanks for including that!

codeboten

Thanks for taking on this work! My suggestions are mostly around updating the terminology to use excluded/exclude list. Otherwise this is looking good.

ext/opentelemetry-ext-flask/CHANGELOG.md

ext/opentelemetry-ext-flask/README.rst

ext/opentelemetry-ext-flask/src/opentelemetry/ext/flask/__init__.py

ext/opentelemetry-ext-flask/tests/test_flask_integration.py

opentelemetry-api/src/opentelemetry/util/__init__.py

codeboten · 2020-04-30T03:37:18Z

Just looked at the go repo and it looks like they're using the term filter:
open-telemetry/opentelemetry-go@2ef25ea#diff-a86e092adb1036300f3e9802aaca1e48R44-R46

lzchen · 2020-04-30T18:46:26Z

@codeboten
Made the changes you've requested.

codeboten

LGTM!

ocelotl · 2020-04-30T22:17:18Z

Leverage global configurations to allow users to specify paths and hosts that they do not want to trace within their Flask applications.

OPENTELEMETRY_PYTHON_FLASK_EXCLUDED_HOSTS and OPENTELEMETRY_PYTHON_FLASK_EXCLUDED_PATHS are the env variables used. Use a comma delimited string to represent seperate hosts/paths to blacklist.

TODO:

Perhaps allow users to pro-grammatically configure these (through code, maybe Configurations should be modifiable through code as well)

Related to [make suppress_instrumentation emit a defaultspan #581]. Perhaps we should emit a default span instead of not patching. I don't really see a benefit for this though.

I think we need the configuration object to be immutable. Since it is global, and every component of OpenTelemetry can use it to read configuration, if a component changed one of its attributes this could break another component that relied on the original value of the same attribute.

In order to avoid doing so, the BaseInstrumentor interface for instrument supports optional arguments in **kwargs that can be used to pass values that can override the configuration attributes read in the instrument (or uninstrument) code. In this way, the user has a programmatic control of the very same options that can be configured for automatic instrumentation without putting any other component at risk.

ocelotl

Some comments suggesting using regular expressions in the environment variable(s).

opentelemetry-api/src/opentelemetry/util/__init__.py

ocelotl · 2020-04-30T22:34:32Z

ext/opentelemetry-ext-flask/src/opentelemetry/ext/flask/__init__.py

+    excluded_hosts = configuration.Configuration().FLASK_EXCLUDED_HOSTS
+    excluded_paths = configuration.Configuration().FLASK_EXCLUDED_PATHS


Is this separation necessary? I think we can just define a sequence of regular expressions in one environment variable and if the hostname or path matches any regular expression it is rejected. This will be useful for the end user who would not need to repeat a lot of related paths in the environment variable, but use a single regex that matches them all.

I worry a little bit about that, as it makes it hard if you happen to have a host or path that share the same name.

Horrible example, but what about the following:

host: localhost
path: check_if_localhost

and you only want to filter for the localhost host.

ext/opentelemetry-ext-flask/CHANGELOG.md

toumorokoshi · 2020-04-30T23:17:23Z

Perhaps allow users to pro-grammatically configure these (through code, maybe Configurations should be modifiable through code as well

I think we should just do this work in Configurations itself, But it's great that this PR is utilization the global configurations and can leverage that behavior.

toumorokoshi

The code looks good! But I think the Configuration clearing mechanism should be added so we don't have the hacky re-init and slots.

Or if you want to file an issue for that and take it on as a follow-up PR, I'm fine with that too.

ext/opentelemetry-ext-flask/tests/test_flask_integration.py

opentelemetry-api/src/opentelemetry/util/__init__.py

toumorokoshi

I'll switch this to approve, the config work can be picked up in a follow-up PR.

ocelotl

I think there might be a bug in the regex, please take a look, @lzchen.

ocelotl · 2020-05-01T23:06:51Z

ext/opentelemetry-ext-flask/README.rst

+*************
+Excludes certain hosts and paths from being tracked. Pass in comma delimited string into environment variables.
+
+Excluded hosts: OPENTELEMETRY_PYTHON_FLASK_EXCLUDED_HOSTS


It needs to be explained here that the values of these environment variables are regexes that should match with everything but the r"(https?|ftp)://.*?/" (are there any other protocols besides http, https and ftp that we want to take into consideration?) part of the URL the user wants excluded from tracing.

I've included an explanation of what hosts and paths means. I don't think we need to inform the user of how the implementation is actually doing this. As for other protocols, these are the ones being excluded in OpenCensus, any new protocols I feel can be added later on.

opentelemetry-api/src/opentelemetry/util/__init__.py

ocelotl

Looking good!

It is a common occurrence in tests that the global Configuration object needs to be "reset" between tests. This means that its attributes need to be set back to their original values. Since the Configuration object is immutable by design, some additional, non-production available mechanism is needed to perform this action. The need for this feature was mentioned in a conversation in #630.

blacklist

44e8a9d

lzchen requested a review from a team April 29, 2020 22:53

changelog

a08a4c4

codeboten suggested changes Apr 30, 2020

View reviewed changes

change to excluded

97b57de

lzchen changed the title ~~Add blacklisting for flask integration~~ Add exclude lists for flask integration Apr 30, 2020

codeboten approved these changes Apr 30, 2020

View reviewed changes

lzchen added 3 commits April 30, 2020 12:00

fix test

3d25b53

typing

d2ab9f7

black

0c913e1

codeboten added this to the Beta v0.7 milestone Apr 30, 2020

lzchen added 2 commits April 30, 2020 13:50

black

1c97d72

black

f199d74

fix test

043fe16

ocelotl reviewed Apr 30, 2020

View reviewed changes

mypy

4c1fc68

mauriciovasquezbernal reviewed Apr 30, 2020

View reviewed changes

ext/opentelemetry-ext-flask/CHANGELOG.md Outdated Show resolved Hide resolved

toumorokoshi suggested changes Apr 30, 2020

View reviewed changes

ext/opentelemetry-ext-flask/tests/test_flask_integration.py Show resolved Hide resolved

opentelemetry-api/src/opentelemetry/util/__init__.py Show resolved Hide resolved

toumorokoshi approved these changes Apr 30, 2020

View reviewed changes

ocelotl mentioned this pull request May 1, 2020

Add a standard way to "reset" a Configuration object for testing #634

Closed

lzchen added 2 commits May 1, 2020 12:58

fix regex

67a1cd2

fix lint

472b86c

ocelotl requested changes May 1, 2020

View reviewed changes

fix regex

d7045b5

ocelotl approved these changes May 1, 2020

View reviewed changes

Merge branch 'master' into blacklist

8197d2f

toumorokoshi merged commit 1d84ee9 into open-telemetry:master May 3, 2020

lzchen deleted the blacklist branch May 4, 2020 03:45

lzchen mentioned this pull request May 11, 2020

Add exclude lists for Django #670

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add exclude lists for flask integration #630

Add exclude lists for flask integration #630

lzchen commented Apr 29, 2020 •

edited

Loading

ocelotl commented Apr 29, 2020

lzchen commented Apr 29, 2020

codeboten left a comment

codeboten commented Apr 30, 2020

lzchen commented Apr 30, 2020

codeboten left a comment

ocelotl commented Apr 30, 2020

ocelotl left a comment

ocelotl Apr 30, 2020

toumorokoshi Apr 30, 2020

toumorokoshi commented Apr 30, 2020

toumorokoshi left a comment

toumorokoshi left a comment

ocelotl left a comment

ocelotl May 1, 2020

lzchen May 1, 2020

ocelotl left a comment

		excluded_hosts = configuration.Configuration().FLASK_EXCLUDED_HOSTS
		excluded_paths = configuration.Configuration().FLASK_EXCLUDED_PATHS

Add exclude lists for flask integration #630

Add exclude lists for flask integration #630

Conversation

lzchen commented Apr 29, 2020 • edited Loading

ocelotl commented Apr 29, 2020

lzchen commented Apr 29, 2020

codeboten left a comment

Choose a reason for hiding this comment

codeboten commented Apr 30, 2020

lzchen commented Apr 30, 2020

codeboten left a comment

Choose a reason for hiding this comment

ocelotl commented Apr 30, 2020

ocelotl left a comment

Choose a reason for hiding this comment

ocelotl Apr 30, 2020

Choose a reason for hiding this comment

toumorokoshi Apr 30, 2020

Choose a reason for hiding this comment

toumorokoshi commented Apr 30, 2020

toumorokoshi left a comment

Choose a reason for hiding this comment

toumorokoshi left a comment

Choose a reason for hiding this comment

ocelotl left a comment

Choose a reason for hiding this comment

ocelotl May 1, 2020

Choose a reason for hiding this comment

lzchen May 1, 2020

Choose a reason for hiding this comment

ocelotl left a comment

Choose a reason for hiding this comment

lzchen commented Apr 29, 2020 •

edited

Loading