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

Conversation

lzchen
Copy link
Contributor

@lzchen lzchen commented May 11, 2020

Similar to exclude listing for flask, implement for Django.

Also fixed some tests in flask that weren't being executed.

@lzchen lzchen requested a review from a team May 11, 2020 19:52
@codeboten codeboten added the needs reviewers PRs with this label are ready for review and needs people to review to move forward. label May 14, 2020
Copy link
Member

@toumorokoshi toumorokoshi left a comment

Choose a reason for hiding this comment

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

Functionally looks good! see comments for a performance improvement I think is quite important.

Copy link
Contributor

@ocelotl ocelotl left a comment

Choose a reason for hiding this comment

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

I think this can still cause an excluded path to reject an URL regardless of its host, but if this is the functionality you want to implement, it is fine 👍

Just requesting changes for a couple of prints. If they are necessary let me know and I'll approve.

opentelemetry-api/src/opentelemetry/util/__init__.py Outdated Show resolved Hide resolved
opentelemetry-api/src/opentelemetry/util/__init__.py Outdated Show resolved Hide resolved
opentelemetry-api/src/opentelemetry/util/__init__.py Outdated Show resolved Hide resolved
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

Please update the changelog

Copy link
Member

@toumorokoshi toumorokoshi left a comment

Choose a reason for hiding this comment

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

Thanks for the changes! And refactoring looks good. There's a slight issue with the global cache that I think should be addressed now.

@@ -2,6 +2,8 @@

## Unreleased

- Add exclude lists
Copy link
Member

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?

ext/opentelemetry-ext-django/README.rst Show resolved Hide resolved
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.

Copy link
Member

@toumorokoshi toumorokoshi left a comment

Choose a reason for hiding this comment

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

great, thanks!

@toumorokoshi toumorokoshi merged commit 807e106 into open-telemetry:master May 27, 2020
@lzchen lzchen deleted the exclude-django branch August 16, 2020 02:04
@lzchen
Copy link
Contributor Author

lzchen commented Aug 16, 2020

#565

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs reviewers PRs with this label are ready for review and needs people to review to move forward.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants