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

Falcon 3 support #644

Merged
merged 32 commits into from
Sep 27, 2021
Merged
Show file tree
Hide file tree
Changes from 24 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
21bdf4c
Enable compatibility w/ Falcon 3
adriangb Sep 1, 2021
beb8bc5
add Falcon 3 to tox.ini
adriangb Sep 1, 2021
b630585
changelog
adriangb Sep 1, 2021
4e00693
run generate because why not make this more complicated and convolute…
adriangb Sep 1, 2021
af1075f
manually apply formatting because everything has to be broken
adriangb Sep 1, 2021
b2a09fe
use single attribute
adriangb Sep 1, 2021
9ad8d43
update installs:
adriangb Sep 1, 2021
189f6f5
Update tox.ini
adriangb Sep 1, 2021
34887a4
Update tox.ini
adriangb Sep 1, 2021
7e76de3
Update tox.ini
adriangb Sep 1, 2021
a7e82d5
run black manually
adriangb Sep 1, 2021
7cd3d1d
Merge branch 'main' into falcon-3-redux
adriangb Sep 1, 2021
a1ec1bc
Merge branch 'falcon-3-redux' of https://github.com/adriangb/opentele…
adriangb Sep 1, 2021
6bf85a3
Merge branch 'main' into falcon-3-redux
owais Sep 2, 2021
afac5c9
Use falcon.App for Falcon 3, inject remote_adr since Falcon 3 does no…
adriangb Sep 2, 2021
139ccbd
Just missing exception info now
adriangb Sep 2, 2021
1e18658
Merge branch 'main' into falcon-3-redux
adriangb Sep 16, 2021
cc99277
Get exc_info from _handle_exception
adriangb Sep 16, 2021
b6ef52d
run tox generate
adriangb Sep 16, 2021
5e14752
ignore pylint error
adriangb Sep 16, 2021
16f91cf
Use the same method everywhere
adriangb Sep 16, 2021
89d1c23
check for exc
adriangb Sep 16, 2021
9b2e8c5
Now always add attributes in middleware
adriangb Sep 16, 2021
26f5d32
update generated code
adriangb Sep 16, 2021
8eda7b6
add status.description assertions
adriangb Sep 16, 2021
f3ce4f9
tox -e generate again
adriangb Sep 16, 2021
3d9ad0a
Merge branch 'main' into falcon-3-redux
adriangb Sep 16, 2021
5dafc13
Merge branch 'main' into falcon-3-redux
adriangb Sep 17, 2021
6b73d23
Merge branch 'main' into falcon-3-redux
adriangb Sep 21, 2021
3266b8f
Merge branch 'main' into falcon-3-redux
ocelotl Sep 24, 2021
18d4b53
Merge branch 'main' into falcon-3-redux
adriangb Sep 27, 2021
bff338f
Merge branch 'main' into falcon-3-redux
lzchen Sep 27, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- `opentelemetry-instrumentation-urllib3` Updated `_RequestHookT` with two additional fields - the request body and the request headers
([#660](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/660))

### Changed
- Tests for Falcon 3 support
([#644](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/644))

## [1.5.0-0.24b0](https://github.com/open-telemetry/opentelemetry-python/releases/tag/v1.5.0-0.24b0) - 2021-08-26

### Added
Expand Down
2 changes: 1 addition & 1 deletion instrumentation/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
| [opentelemetry-instrumentation-dbapi](./opentelemetry-instrumentation-dbapi) | dbapi |
| [opentelemetry-instrumentation-django](./opentelemetry-instrumentation-django) | django >= 1.10 |
| [opentelemetry-instrumentation-elasticsearch](./opentelemetry-instrumentation-elasticsearch) | elasticsearch >= 2.0 |
| [opentelemetry-instrumentation-falcon](./opentelemetry-instrumentation-falcon) | falcon ~= 2.0 |
| [opentelemetry-instrumentation-falcon](./opentelemetry-instrumentation-falcon) | falcon >= 2.0.0, < 4.0.0 |
| [opentelemetry-instrumentation-fastapi](./opentelemetry-instrumentation-fastapi) | fastapi ~= 0.58 |
| [opentelemetry-instrumentation-flask](./opentelemetry-instrumentation-flask) | flask >= 1.0, < 3.0 |
| [opentelemetry-instrumentation-grpc](./opentelemetry-instrumentation-grpc) | grpcio ~= 1.27 |
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,6 @@ OpenTelemetry Falcon Tracing
This library builds on the OpenTelemetry WSGI middleware to track web requests
in Falcon applications.

Currently, only Falcon v2 is supported.

Installation
------------

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,13 @@ def response_hook(span, req, resp):

_response_propagation_setter = FuncSetter(falcon.Response.append_header)

if hasattr(falcon, "App"):
# Falcon 3
_instrument_app = "App"
else:
# Falcon 2
_instrument_app = "API"
Comment on lines +129 to +134
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 centralized this logic up here so we don't duplicate it below



class FalconInstrumentor(BaseInstrumentor):
# pylint: disable=protected-access,attribute-defined-outside-init
Expand All @@ -138,14 +145,16 @@ def instrumentation_dependencies(self) -> Collection[str]:
return _instruments

def _instrument(self, **kwargs):
self._original_falcon_api = falcon.API
falcon.API = partial(_InstrumentedFalconAPI, **kwargs)
self._original_falcon_api = getattr(falcon, _instrument_app)
setattr(
falcon, _instrument_app, partial(_InstrumentedFalconAPI, **kwargs)
)

def _uninstrument(self, **kwargs):
falcon.API = self._original_falcon_api
setattr(falcon, _instrument_app, self._original_falcon_api)


class _InstrumentedFalconAPI(falcon.API):
class _InstrumentedFalconAPI(getattr(falcon, _instrument_app)):
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 really dislike using a getattr in a class def, but it works and it avoids a deprecation warning for Falcon 3 users.

def __init__(self, *args, **kwargs):
# inject trace middleware
middlewares = kwargs.pop("middleware", [])
Expand All @@ -169,6 +178,15 @@ def __init__(self, *args, **kwargs):
self._excluded_urls = get_excluded_urls("FALCON")
super().__init__(*args, **kwargs)

def _handle_exception(
self, req, resp, ex, params
): # pylint: disable=C0103
# Falcon 3 does not execute middleware within the context of the exception
# so we capture the exception here and save it into the env dict
_, exc, _ = exc_info()
req.env[_ENVIRON_EXC] = exc
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 using the already defined but previously unused key _ENVIRON_EXC to store the status info.

Copy link
Contributor

Choose a reason for hiding this comment

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

and this works for both 2 and 3?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the _handle_exception also exists in Falcon 2, so it seems to work 😃

return super()._handle_exception(req, resp, ex, params)
Comment on lines +181 to +188
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 couldn't figure out another way to do this other than accessing this private method, FYI


def __call__(self, env, start_response):
# pylint: disable=E1101
if self._excluded_urls.url_disabled(env.get("PATH_INFO", "/")):
Expand All @@ -193,7 +211,6 @@ def __call__(self, env, start_response):
env[_ENVIRON_ACTIVATION_KEY] = activation

def _start_response(status, response_headers, *args, **kwargs):
otel_wsgi.add_response_attributes(span, status, response_headers)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was a pretty insidious bug that was hard to figure out.

If you look at the middleware, it is already setting span.status. This re sets it. otel_wsgi.add_response_attributes sets the same status code as the middleware, but does not set the description. So after this line executes span.status.description is always None. But none of the tests were checking this, so it was not caught.

But what about test_500 then? Well, in Falcon 2 (but not 3) super().__call__(...) would re-raise the error, triggering the except clause right below this that skips calling _start_response and thus otel_wsgi.add_response_attributes(span, status, response_headers) is never called and span.status.description is carried over from the middleware.

I'll add an assertion for span.status.description to other tests so that this can be avoided in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Checks added in 8eda7b6

response = start_response(
status, response_headers, *args, **kwargs
)
Expand Down Expand Up @@ -264,29 +281,32 @@ def process_response(
if resource is None:
status = "404"
reason = "NotFound"

exc_type, exc, _ = exc_info()
if exc_type and not req_succeeded:
if "HTTPNotFound" in exc_type.__name__:
status = "404"
reason = "NotFound"
else:
if _ENVIRON_EXC in req.env:
exc = req.env[_ENVIRON_EXC]
exc_type = type(exc)
else:
status = "500"
reason = "{}: {}".format(exc_type.__name__, exc)
exc_type, exc = None, None
if exc_type and not req_succeeded:
if "HTTPNotFound" in exc_type.__name__:
status = "404"
reason = "NotFound"
else:
status = "500"
reason = "{}: {}".format(exc_type.__name__, exc)

status = status.split(" ")[0]
try:
status_code = int(status)
except ValueError:
pass
finally:
span.set_attribute(SpanAttributes.HTTP_STATUS_CODE, status_code)
span.set_status(
Status(
status_code=http_status_to_status_code(status_code),
description=reason,
)
)
except ValueError:
pass

propagator = get_global_response_propagator()
if propagator:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,4 +13,4 @@
# limitations under the License.


_instruments = ("falcon ~= 2.0",)
_instruments = ("falcon >= 2.0.0, < 4.0.0",)
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,12 @@ def on_get(self, req, resp):


def make_app():
app = falcon.API()
if hasattr(falcon, "App"):
# Falcon 3
app = falcon.App()
else:
# Falcon 2
app = falcon.API()
app.add_route("/hello", HelloWorldResource())
app.add_route("/ping", HelloWorldResource())
app.add_route("/error", ErrorResource())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,9 @@ def test_head(self):
self._test_method("HEAD")

def _test_method(self, method):
self.client().simulate_request(method=method, path="/hello")
self.client().simulate_request(
method=method, path="/hello", remote_addr="127.0.0.1"
)
spans = self.memory_exporter.get_finished_spans()
self.assertEqual(len(spans), 1)
span = spans[0]
Expand All @@ -105,7 +107,7 @@ def _test_method(self, method):
self.memory_exporter.clear()

def test_404(self):
self.client().simulate_get("/does-not-exist")
self.client().simulate_get("/does-not-exist", remote_addr="127.0.0.1")
spans = self.memory_exporter.get_finished_spans()
self.assertEqual(len(spans), 1)
span = spans[0]
Expand All @@ -129,7 +131,7 @@ def test_404(self):

def test_500(self):
try:
self.client().simulate_get("/error")
self.client().simulate_get("/error", remote_addr="127.0.0.1")
except NameError:
pass
spans = self.memory_exporter.get_finished_spans()
Expand Down
1 change: 1 addition & 0 deletions pytest.ini
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,4 @@
addopts = -rs -v
log_cli = true
log_cli_level = warning
testpaths = instrumentation/opentelemetry-instrumentation-falcon/tests/
12 changes: 7 additions & 5 deletions tox.ini
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,8 @@ envlist =
pypy3-test-instrumentation-elasticsearch{2,5,6}

; opentelemetry-instrumentation-falcon
py3{4,5,6,7,8,9}-test-instrumentation-falcon
pypy3-test-instrumentation-falcon
py3{4,5,6,7,8,9}-test-instrumentation-falcon{2,3}
pypy3-test-instrumentation-falcon{2,3}

; opentelemetry-instrumentation-fastapi
; fastapi only supports 3.6 and above.
Expand Down Expand Up @@ -175,6 +175,8 @@ deps =
; FIXME: Elasticsearch >=7 causes CI workflow tests to hang, see open-telemetry/opentelemetry-python-contrib#620
; elasticsearch7: elasticsearch-dsl>=7.0,<8.0
; elasticsearch7: elasticsearch>=7.0,<8.0
falcon2: falcon >=2.0.0,<3.0.0
falcon3: falcon >=3.0.0,<4.0.0
sqlalchemy11: sqlalchemy>=1.1,<1.2
sqlalchemy14: aiosqlite
sqlalchemy14: sqlalchemy~=1.4
Expand All @@ -193,7 +195,7 @@ changedir =
test-instrumentation-dbapi: instrumentation/opentelemetry-instrumentation-dbapi/tests
test-instrumentation-django: instrumentation/opentelemetry-instrumentation-django/tests
test-instrumentation-elasticsearch{2,5,6}: instrumentation/opentelemetry-instrumentation-elasticsearch/tests
test-instrumentation-falcon: instrumentation/opentelemetry-instrumentation-falcon/tests
test-instrumentation-falcon{2,3}: instrumentation/opentelemetry-instrumentation-falcon/tests
test-instrumentation-fastapi: instrumentation/opentelemetry-instrumentation-fastapi/tests
test-instrumentation-flask: instrumentation/opentelemetry-instrumentation-flask/tests
test-instrumentation-urllib: instrumentation/opentelemetry-instrumentation-urllib/tests
Expand Down Expand Up @@ -237,15 +239,15 @@ commands_pre =
grpc: pip install {toxinidir}/instrumentation/opentelemetry-instrumentation-grpc[test]

falcon,flask,django,pyramid,tornado,starlette,fastapi,aiohttp,asgi,requests,urllib,wsgi: pip install {toxinidir}/util/opentelemetry-util-http[test]
wsgi,falcon,flask,django,pyramid: pip install {toxinidir}/instrumentation/opentelemetry-instrumentation-wsgi[test]
wsgi,falcon{2,3},flask,django,pyramid: pip install {toxinidir}/instrumentation/opentelemetry-instrumentation-wsgi[test]
asgi,starlette,fastapi: pip install {toxinidir}/instrumentation/opentelemetry-instrumentation-asgi[test]

asyncpg: pip install {toxinidir}/instrumentation/opentelemetry-instrumentation-asyncpg[test]

boto: pip install {toxinidir}/instrumentation/opentelemetry-instrumentation-botocore[test]
boto: pip install {toxinidir}/instrumentation/opentelemetry-instrumentation-boto[test]

falcon: pip install {toxinidir}/instrumentation/opentelemetry-instrumentation-falcon[test]
falcon{2,3}: pip install {toxinidir}/instrumentation/opentelemetry-instrumentation-falcon[test]

flask: pip install {toxinidir}/instrumentation/opentelemetry-instrumentation-flask[test]

Expand Down