Falcon 3 support #644

adriangb · 2021-09-01T05:47:10Z

No description provided.

…d to contribute to

adriangb · 2021-09-01T06:16:48Z

@ocelotl take a look when you have a chance. I'm seeing the following failure:

_ ERROR collecting instrumentation/opentelemetry-instrumentation-falcon/tests/test_falcon.py _
1956
ImportError while importing test module '/home/runner/work/opentelemetry-python-contrib/opentelemetry-python-contrib/instrumentation/opentelemetry-instrumentation-falcon/tests/test_falcon.py'.
1957
Hint: make sure your test modules/packages have valid Python names.
1958
Traceback:
1959
/opt/hostedtoolcache/Python/3.9.6/x64/lib/python3.9/importlib/__init__.py:127: in import_module
1960
    return _bootstrap._gcd_import(name[level:], package, level)
1961
test_falcon.py:19: in <module>
1962
    from opentelemetry.instrumentation.falcon import FalconInstrumentor
1963
E   ModuleNotFoundError: No module named 'opentelemetry.instrumentation.falcon'

This doesn't make much sense to me, I didn't change the filenames...

I'm also seeing lint and generate failures. I ran tox generate, it did a bunch of stuff and it's still complaining. It seems like running it locally doesn't agree with CI 🤷 .

Also, CONTRIBUTING.md is still broken 😢

owais

Nice. Looks good. Left one comment.

owais · 2021-09-01T10:25:16Z

...on/opentelemetry-instrumentation-falcon/src/opentelemetry/instrumentation/falcon/__init__.py

@@ -140,11 +140,22 @@ def instrumentation_dependencies(self) -> Collection[str]:
        return _instruments

    def _instrument(self, **kwargs):
+        self._original_falcon_api = None
+        self._original_falcon_app = None


can we use only one attribute for both 2 and 3, and conditionally read/write falcon.App or falcon.API?

Sure, if you prefer that

owais · 2021-09-01T10:28:05Z

@adriangb what command is that traceback from? I just ran tox -e py39-test-instrumentation-falcon on main and it worked perfectly for me locally.

owais · 2021-09-01T10:31:32Z

I think you probably need to update these lines as well:

https://github.com/open-telemetry/opentelemetry-python-contrib/blob/main/tox.ini#L248
https://github.com/open-telemetry/opentelemetry-python-contrib/blob/main/tox.ini#L239

adriangb · 2021-09-01T15:03:48Z

@adriangb what command is that traceback from? I just ran tox -e py39-test-instrumentation-falcon on main and it worked perfectly for me locally.

https://github.com/open-telemetry/opentelemetry-python-contrib/pull/644/checks?check_run_id=3485738088#step:7:1963

adriangb · 2021-09-01T15:06:58Z

I think you probably need to update these lines as well:

Makes sense, thank you, updated

tox.ini

Co-authored-by: Owais Lone <[email protected]>

tox.ini

Co-authored-by: Owais Lone <[email protected]>

…metry-python-contrib into falcon-3-redux

adriangb · 2021-09-01T15:52:51Z

Thanks for the help @owais

Looks like the tests are actually broken now. The tests access a private attribute of falcon.App then double [0] index on it and then accesses __self__. There are no comments as to why all of this is going on. To be honest, I'm pretty shocked that tests are monkey patching private attributes of stuff that was already monkey patched by the instrumentation. monkey patching is bad!. Nested monkey patching is worse. Nested monkey patching of private attributes is guaranteed to break stuff.

owais · 2021-09-01T16:14:32Z

Looks like Falcon changed how it handles middlewares internally so we need to update test code so it can correctly fetch the injected Otel middleware for both 2 and 3.

adriangb · 2021-09-01T16:26:23Z

Yes, I agree. However I think I think it's unfair to blame this on Falcon: the test breaks because it's fragile, and it's fragile because it accesses private attributes of a 3rd party library. They could have made the change in a patch bump, we should be thankful that they seem to have done it during a major version bump...

So while it may be possible to fix this here, I think the overall approach of monkey patching stuff (in the library, in tests) and accessing private stuff in 3rd party libraries is fundamentally flawed and brittle.

It's also going to be hard to fix this test: I still can't run this locally (because as we've discussed before the local setup instructions are broken; and no one has been able to give me a series of valid steps of cloning repos into repos and installing stuff that works).

It looks like this portion of the code was authored by you (@owais ) and @lonewolf3739. I'd appreciate a walkthrough of why this is being done, what [0][0] is accessing, etc if I'm going to try to fix this test by blindly pushing to CI.

owais · 2021-09-01T16:30:43Z

I'm not blaming anything. I was merely pointing out what needs to be fixed in order to make the test pass again.

I'd appreciate a walkthrough of why this is being done, what [0][0] is accessing, etc if I'm going to try to fix this test by blindly pushing to CI.

Looking at the test code, it need a reference to the injected middleware which can be accessed from a falcon app instance with the help of [0][0] because of how it internally stored references to middlewares. If there is a public API to fetch references to installed middleware, we should use that but I'm not aware of any.

owais · 2021-09-01T16:31:26Z

It's also going to be hard to fix this test: I still can't run this locally (because as we've discussed before the local setup instructions are broken; and no one has been able to give me a series of valid steps of cloning repos into repos and installing stuff that works).

Is there an issue for this I can look at and answer?

adriangb · 2021-09-16T17:51:05Z

...on/opentelemetry-instrumentation-falcon/src/opentelemetry/instrumentation/falcon/__init__.py

@@ -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)


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.

Checks added in 8eda7b6

adriangb · 2021-09-16T17:51:28Z

...on/opentelemetry-instrumentation-falcon/src/opentelemetry/instrumentation/falcon/__init__.py

+    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
+        return super()._handle_exception(req, resp, ex, params)


I couldn't figure out another way to do this other than accessing this private method, FYI

adriangb · 2021-09-16T17:51:58Z

...on/opentelemetry-instrumentation-falcon/src/opentelemetry/instrumentation/falcon/__init__.py

+        # 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


I'm using the already defined but previously unused key _ENVIRON_EXC to store the status info.

and this works for both 2 and 3?

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

adriangb · 2021-09-16T17:52:18Z

...on/opentelemetry-instrumentation-falcon/src/opentelemetry/instrumentation/falcon/__init__.py

+if hasattr(falcon, "App"):
+    # Falcon 3
+    _instrument_app = "App"
+else:
+    # Falcon 2
+    _instrument_app = "API"


I centralized this logic up here so we don't duplicate it below

adriangb · 2021-09-16T17:52:50Z

...on/opentelemetry-instrumentation-falcon/src/opentelemetry/instrumentation/falcon/__init__.py



-class _InstrumentedFalconAPI(falcon.API):
+class _InstrumentedFalconAPI(getattr(falcon, _instrument_app)):


I really dislike using a getattr in a class def, but it works and it avoids a deprecation warning for Falcon 3 users.

adriangb · 2021-09-16T18:00:19Z

@owais this is now working 😄

adriangb · 2021-09-16T18:22:05Z

I'm happy to discuss any topic but lets please create separate issues or discussions for those and here just focus on getting this PR through

I opened #683 to discuss further

adriangb · 2021-09-21T23:27:44Z

@owais I merged in main. Could we move forward with this? Thanks.

adriangb · 2021-09-24T16:45:55Z

thanks for updating the branch 😄

owais · 2021-09-25T03:23:40Z

LGTM but giving others time to review as well.

owais · 2021-09-27T17:36:20Z

@adriangb please resolve conflcits and we'll merge this.

lzchen · 2021-09-27T17:40:54Z

@adriangb

Are you interested in becoming a CODEOWNER for the falcon instrumentation package? The responsibilities are outlined here

adriangb · 2021-09-27T17:44:56Z

@adriangb please resolve conflcits and we'll merge this.

Done 😄 , let's let the tests run and make sure everything is still ✅

Are you interested in becoming a CODEOWNER for the falcon instrumentation package?

At this point in time I have enough OSS work on my hands, but I will certainly consider it. I appreciate the offer.

adriangb added 2 commits September 1, 2021 00:43

Enable compatibility w/ Falcon 3

21bdf4c

add Falcon 3 to tox.ini

beb8bc5

adriangb requested review from a team, owais and NathanielRN and removed request for a team September 1, 2021 05:47

adriangb added 3 commits September 1, 2021 00:49

changelog

b630585

run generate because why not make this more complicated and convolute…

4e00693

…d to contribute to

manually apply formatting because everything has to be broken

af1075f

owais reviewed Sep 1, 2021

View reviewed changes

use single attribute

b2a09fe

update installs:

9ad8d43

owais reviewed Sep 1, 2021

View reviewed changes

tox.ini Outdated Show resolved Hide resolved

Update tox.ini

189f6f5

Co-authored-by: Owais Lone <[email protected]>

owais reviewed Sep 1, 2021

View reviewed changes

tox.ini Outdated Show resolved Hide resolved

tox.ini Outdated Show resolved Hide resolved

adriangb and others added 5 commits September 1, 2021 10:18

Update tox.ini

34887a4

Co-authored-by: Owais Lone <[email protected]>

Update tox.ini

7e76de3

Co-authored-by: Owais Lone <[email protected]>

run black manually

a7e82d5

Merge branch 'main' into falcon-3-redux

7cd3d1d

Merge branch 'falcon-3-redux' of https://github.com/adriangb/opentele…

a1ec1bc

…metry-python-contrib into falcon-3-redux

adriangb added 6 commits September 16, 2021 10:44

run tox generate

b6ef52d

ignore pylint error

5e14752

Use the same method everywhere

16f91cf

check for exc

89d1c23

Now always add attributes in middleware

9b2e8c5

update generated code

26f5d32

adriangb commented Sep 16, 2021

View reviewed changes

add status.description assertions

8eda7b6

adriangb requested a review from owais September 16, 2021 18:00

tox -e generate again

f3ce4f9

adriangb added 3 commits September 16, 2021 18:02

Merge branch 'main' into falcon-3-redux

3d9ad0a

Merge branch 'main' into falcon-3-redux

5dafc13

Merge branch 'main' into falcon-3-redux

6b73d23

Merge branch 'main' into falcon-3-redux

3266b8f

owais approved these changes Sep 25, 2021

View reviewed changes

lzchen approved these changes Sep 27, 2021

View reviewed changes

owais enabled auto-merge (squash) September 27, 2021 17:35

Merge branch 'main' into falcon-3-redux

18d4b53

auto-merge was automatically disabled September 27, 2021 17:43
Head branch was pushed to by a user without write access

Merge branch 'main' into falcon-3-redux

bff338f

lzchen enabled auto-merge (squash) September 27, 2021 19:44

lzchen merged commit 8e0d0e0 into open-telemetry:main Sep 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Falcon 3 support #644

Falcon 3 support #644

adriangb commented Sep 1, 2021

adriangb commented Sep 1, 2021 •

edited

Loading

owais left a comment

owais Sep 1, 2021

adriangb Sep 1, 2021

owais commented Sep 1, 2021

owais commented Sep 1, 2021

adriangb commented Sep 1, 2021

adriangb commented Sep 1, 2021

adriangb commented Sep 1, 2021 •

edited

Loading

owais commented Sep 1, 2021

adriangb commented Sep 1, 2021 •

edited

Loading

owais commented Sep 1, 2021

owais commented Sep 1, 2021

adriangb Sep 16, 2021

adriangb Sep 24, 2021

adriangb Sep 16, 2021

adriangb Sep 16, 2021

owais Sep 25, 2021

adriangb Sep 25, 2021

adriangb Sep 16, 2021

adriangb Sep 16, 2021

adriangb commented Sep 16, 2021

adriangb commented Sep 16, 2021

adriangb commented Sep 21, 2021

adriangb commented Sep 24, 2021

owais commented Sep 25, 2021

owais commented Sep 27, 2021

lzchen commented Sep 27, 2021

adriangb commented Sep 27, 2021



		class _InstrumentedFalconAPI(falcon.API):
		class _InstrumentedFalconAPI(getattr(falcon, _instrument_app)):

Falcon 3 support #644

Falcon 3 support #644

Conversation

adriangb commented Sep 1, 2021

adriangb commented Sep 1, 2021 • edited Loading

owais left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

owais commented Sep 1, 2021

owais commented Sep 1, 2021

adriangb commented Sep 1, 2021

adriangb commented Sep 1, 2021

adriangb commented Sep 1, 2021 • edited Loading

owais commented Sep 1, 2021

adriangb commented Sep 1, 2021 • edited Loading

owais commented Sep 1, 2021

owais commented Sep 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adriangb commented Sep 16, 2021

adriangb commented Sep 16, 2021

adriangb commented Sep 21, 2021

adriangb commented Sep 24, 2021

owais commented Sep 25, 2021

owais commented Sep 27, 2021

lzchen commented Sep 27, 2021

adriangb commented Sep 27, 2021

adriangb commented Sep 1, 2021 •

edited

Loading

adriangb commented Sep 1, 2021 •

edited

Loading

adriangb commented Sep 1, 2021 •

edited

Loading