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 Flask integration based on WSGI ext #206

Merged
merged 17 commits into from
Nov 23, 2019

Conversation

Oberon00
Copy link
Member

@Oberon00 Oberon00 commented Oct 8, 2019

The flask integration has (only) two advantages over the plain WSGI middleware approach:

In addition, it also has an easier syntax to enable (you don't have to know about Flask.wsgi_app).

(above summary copied from #206 (comment))

I did not add flask to the dependencies (except the test extra) because it seems kinda useless: To use the extension, you have to pass a Flask object which makes it obvious that you need Flask. If you think that's too odd, I will split the flask integration into new distribution that depends on the WSGI integration.

span.set_attribute("http.url", url)


def add_response_attributes(
Copy link
Member Author

Choose a reason for hiding this comment

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

Since this is now a public API (no underscore & docstring), I thought it is better to add the response_haders argument, since we might very well need it in the future, should we ever want to capture certain response headers as span attributes.

@a-feld
Copy link
Member

a-feld commented Oct 8, 2019

Why is a Flask integration needed in addition to the base WSGI integration? Flask supports WSGI middlewares through the mechanism described in the example document.

@Oberon00
Copy link
Member Author

Oberon00 commented Oct 8, 2019

The flask integration has (only) two advantages:

In addition, it also has an easier syntax to enable (you don't have to know about Flask.wsgi_app).

I'll copy that to the PR description, as it should have been there from the beginning. Thanks for asking that important question, @a-feld 👍

@a-feld
Copy link
Member

a-feld commented Oct 8, 2019

That makes sense, thanks for clarifying @Oberon00 ! Those bullet points definitely seem worth pursuing! I wonder if there's any way to layer / compose those additions so that we maintain separation between the WSGI middleware and the flask specific features?

One approach I can think of is to use the OpenTelemetryMiddlware on the wsgi_app and then use the before_request and after_request hooks to name the span appropriately (using update_name) / add the route attributes while relying purely on the OpenTelemetryMiddlware to create the initial span.

We could continue the approach of inserting the WSGI middleware + before_request / after_request hooks with a wrap_flask function!

I also agree with your assessment that it makes sense to split the flask integration into its own package!

@Oberon00
Copy link
Member Author

Oberon00 commented Oct 8, 2019

But using update_name is "highly discouraged" and in fact the prototype Dynatrace-prototype-API based implementation that caused me to write this can't handle update_name properly (the spanName metadata would be updated but the spans would be grouped according to the initial span name).

host += ":" + port

# NOTE: Nonstandard (but see
# https://github.com/open-telemetry/opentelemetry-specification/pull/263)
Copy link
Member

Choose a reason for hiding this comment

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

since this will likely be merged anyway (3 approvals), maybe just remove the comment?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not yet, according to that spec PR the http.host must only be set if there is an http.host header.

# //foo which looks like scheme-relative but isn't.
url = environ["wsgi.url_scheme"] + "://" + host + url
elif not url.startswith(environ["wsgi.url_scheme"] + ":"):
# Something fishy is in RAW_URL. Let's fall back to request_uri()
Copy link
Member

Choose a reason for hiding this comment

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

when does this happen? Just out of curiosity. Sounds like a common occurence.

Copy link
Member Author

Choose a reason for hiding this comment

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

Why do you think this is common? Personally, I don't really know how often that occurs in practice, but I imagine a buggy client could send a line like GET foobar HTTP/1.0 instead of GET /foobar HTTP/1.0.

otel_wsgi.add_request_attributes(span, environ)
if flask_request.url_rule:
# For 404 that result from no route found, etc, we don't have a url_rule.
span.set_attribute("http.route", flask_request.url_rule.rule)
Copy link
Member

Choose a reason for hiding this comment

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

should this default to the method then? as recommended in the opentelemetry-specification#270?

Copy link
Member Author

Choose a reason for hiding this comment

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

open-telemetry/opentelemetry-specification#270 only talks about the span name, the http.route attribute is unrelated.

@Oberon00 Oberon00 requested review from a-feld, reyang and c24t and removed request for a-feld, reyang and c24t October 15, 2019 16:59
@Oberon00
Copy link
Member Author

Anyone care to review this?

@Oberon00 Oberon00 changed the title Add flask integration to wsgi ext Add flask integration based on WSGI ext Oct 23, 2019
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.

The middleware changes LGTM, but I've got some reservations about the package structure changes.

I'd be interested to get another pass from @a-feld after your last round of changes.

from opentelemetry.ext.flask_util import instrument_app

app = Flask(__name__)
instrument_app(app) # This is where the magic happens. ✨
Copy link
Member

Choose a reason for hiding this comment

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

🤬

Copy link
Member Author

Choose a reason for hiding this comment

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

You think I should tone down the emojis? 😁

ext/opentelemetry-ext-flask/README.rst Outdated Show resolved Hide resolved
opentelemetry-ext-wsgi

[options.extras_require]
test = flask~=1.0
Copy link
Member

Choose a reason for hiding this comment

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

That's interesting, do you think we should specify all our test dependencies this way?

This is a bit weird since the tests still wouldn't be included in the distribution if you do pip install 'opentelemetry-ext-flask[test]', this is only useful for editable installs.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, I'm not sure. Since we now have separate coverage & test targets, in the interest of not growing the toxfile too much, I think I'll keep it here.

@@ -0,0 +1,3 @@
This (non-installable) directory should be added to the PYTHONPATH of any unit tests that require it.
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 the cure is worse than the disease here. It may be inevitable that we'll have enough shared code between tests that we'll have to do something like this, but for now duplicating WsgiTestBase seems less bad than forcing users to edit the PYTHONPATH.

I don't think we should expect that people will only run the tests via tox. Doing e.g. pytest ext/opentelemetry-ext-flask/tests/test_flask_integration.py is much faster while developing.

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 changed this to an ordinary installable package, so no manual PYTHONPATH manipulations required anymore.

tox.ini Outdated Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented Nov 13, 2019

Codecov Report

Merging #206 into master will increase coverage by 0.01%.
The diff coverage is 91.11%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #206      +/-   ##
==========================================
+ Coverage   85.65%   85.66%   +0.01%     
==========================================
  Files          33       33              
  Lines        1610     1612       +2     
  Branches      180      180              
==========================================
+ Hits         1379     1381       +2     
  Misses        185      185              
  Partials       46       46
Impacted Files Coverage Δ
...app/src/opentelemetry_example_app/flask_example.py 100% <100%> (ø) ⬆️
...ry-ext-wsgi/src/opentelemetry/ext/wsgi/__init__.py 90.9% <90.69%> (+0.24%) ⬆️

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 fb3e715...b20aea5. Read the comment docs.

@Oberon00
Copy link
Member Author

@open-telemetry/python-approvers I think this is now ready for another review, I hope I could address all issues satisfyingly.

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 overall. Only blocking comment is the change to the example in the README

@Oberon00
Copy link
Member Author

Technically, this branch is ready to be merged, even though @toumorokoshi' review is outdated. Personally I think the PR is ready.

Also, I'd like to get this merged before working on updating the WSGI extension to follow open-telemetry/opentelemetry-specification#263 to avoid merge conflicts.

otel_wsgi.get_header_from_environ, environ
)

tracer = trace.tracer()
Copy link
Contributor

Choose a reason for hiding this comment

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

(non-blocking comment) There was a mention in the past about allowing users to optionally specify their own Tracer instead of relying on the global one. Probably something to consider adding in the future ;)


tracer = trace.tracer()

span = tracer.create_span(
Copy link
Contributor

Choose a reason for hiding this comment

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

We will be changing create_span() to not exist in the public API, IIRC?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, PR #290 adapted the code, whoever is merged second will have to resolve merge conflicts.

@Oberon00 Oberon00 added the PR:please merge This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.) label Nov 21, 2019
@c24t c24t merged commit da8b8d9 into open-telemetry:master Nov 23, 2019
@c24t c24t removed the PR:please merge This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.) label Nov 23, 2019
srikanthccv pushed a commit to srikanthccv/opentelemetry-python that referenced this pull request Nov 1, 2020
…n-telemetry#206)

* fix(basic-tracer): performance.now() is relative from start time

* refactor(time): tuples

* refactor(span): change name of toHrTime

* fix(span): duration default

* fix(hrtime): conver to tuple

* fix(basic-tracer): nanosecond accuracy

* feat(core): move time utils to core

* feat(core): add millis and nanos converter

* test(basic-tracer): fix span time tests
srikanthccv pushed a commit to srikanthccv/opentelemetry-python that referenced this pull request Nov 1, 2020
* fix: performance.now() is relative from start time

* fix: grpc plugin tests

* fix: import performance
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.

7 participants