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

flask: using string keys for wsgi environ values #366

Conversation

toumorokoshi
Copy link
Member

Some WSGI servers (such as Gunicorn) expect keys in the environ object
to be strings.

fixes #354.

Some WSGI servers (such as Gunicorn) expect keys in the environ object
to be strings.
@toumorokoshi toumorokoshi requested a review from a team as a code owner January 14, 2020 22:42
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.

Thanks for fixing the issue. I guess once the lint check is passing, this is good to go!

from opentelemetry import trace as trace_api
from opentelemetry.ext.testutil.wsgitestutil import WsgiTestBase
from werkzeug.test import Client
from werkzeug.wrappers import BaseResponse
Copy link
Contributor

Choose a reason for hiding this comment

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

surprised to see these imports order needing to change

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm sorting through it, something to do with how isort resolves first party vs third-party imports.

Copy link
Member

@Oberon00 Oberon00 Jan 17, 2020

Choose a reason for hiding this comment

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

Try deleting your virtualenv. I had troubles with isort too and it seems that if there is a site-packages/openetelemetry directory (even an empty one), it will think of opentelemetry as a 3rd party. On the other hand, if you install it editable, there is only a opentelemetry.egg-link file, which isort doesn't find, so it falls back to thinking of opentelemetry as a first-party.
EDIT: Or actually, this should not matter since we have opentelemetry as a "known 1st party" in

known_first_party=opentelemetry
. Strange that it still doesn't work for you.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, I've tracked this down. Effectively modules that are not in the virtualenv are considered first party packages by isort. I'm working on crafting a ticket to fire at isort to clarify this behavior.

For now I'm unblocked by install flask into my virtualenv. On that note, should we be calling out the module that the integration is meant for in the setup.py? I mistakenly thought that flask would be installed when I installed the extension.

Copy link
Member Author

Choose a reason for hiding this comment

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

For reference: PyCQA/isort#1104

_ENVIRON_STARTTIME_KEY = object()
_ENVIRON_SPAN_KEY = object()
_ENVIRON_ACTIVATION_KEY = object()
_ENVIRON_STARTTIME_KEY = "opentelemetry-flask.starttime_key"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
_ENVIRON_STARTTIME_KEY = "opentelemetry-flask.starttime_key"
_ENVIRON_STARTTIME_KEY = "opentelemetry-flask.starttime"

I think we don't need to add _key to the key.

Copy link
Member

@c24t c24t left a comment

Choose a reason for hiding this comment

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

LGTM after lint too.

@codecov-io
Copy link

codecov-io commented Jan 20, 2020

Codecov Report

Merging #366 into master will increase coverage by 0.31%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #366      +/-   ##
==========================================
+ Coverage   84.82%   85.13%   +0.31%     
==========================================
  Files          38       38              
  Lines        1845     1911      +66     
  Branches      217      225       +8     
==========================================
+ Hits         1565     1627      +62     
- Misses        214      219       +5     
+ Partials       66       65       -1
Impacted Files Coverage Δ
...-ext-flask/src/opentelemetry/ext/flask/__init__.py 88.88% <100%> (ø) ⬆️
opentelemetry-api/src/opentelemetry/util/types.py 100% <0%> (ø) ⬆️
...ntelemetry-api/src/opentelemetry/trace/__init__.py 84.56% <0%> (+0.44%) ⬆️
...emetry-sdk/src/opentelemetry/sdk/trace/__init__.py 90.94% <0%> (+1.23%) ⬆️
...elemetry-api/src/opentelemetry/metrics/__init__.py 87.93% <0%> (+1.93%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f0ba817...72872b4. Read the comment docs.

@toumorokoshi
Copy link
Member Author

@codeboten I've got the build passing, take a look when you can.

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.

Looks good. Thanks for tracking down the isort issue!

@toumorokoshi toumorokoshi merged commit 47d42aa into open-telemetry:master Jan 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flask Extension Gunicorn GLogging Error With Access Logs
5 participants