Add FastAPI support #287

apotterri · 2024-02-28T18:00:05Z

Add an integration with FastAPI. Enable it automatically when a FastAPI app is started using uvicorn.

The changes to instrumentation that are included here are necessary to capture an app's functions that are decorated with FastAPI's routing decorators, e.g.

@app.get("/test")
def get_test()
    return {"got":"test"}

Also, create a log file by default, instead of writing to stderr. (This behavior has proven very useful in debugging Java agent problems.)

Fixes #251.

Having the Django and Flask apps both use the module name "app" makes the tests order-dependent. If the Django "app" is loaded before the Flask tests, the latter fail because they try to import "app.main", which doesn't exist Maybe there's some other way to avoid this collision by unloading "app" and still have all the tests pass, but I couldn't find it. Just renaming them seemed simplest.

importlib.metadata was added in 3.8. Now that we no longer support 3.7, we can use it directly, rather than using the backported implementation. (We still need to use the backported version of importlib_resources until we drop 3.8.)

apotterri · 2024-03-11T21:06:09Z

_appmap/web_framework.py

@@ -145,7 +147,7 @@ def after_request_main(self, rec, status, headers, start, call_event_id) -> None
            parent_id=call_event_id,
            elapsed=duration,
            status_code=status,
-            headers=dict(headers.items()),


The keys in a headers collection need to be matched in a case-insensitive manner, so we can't just create a dict from them. This also isn't necessary, because the various web frameworks represent them with with a type that quacks enough like a dict for our purposes.

Is there any place where we're actually matching them outside of tests? I think it might be a better idea to normalize the header names in appmaps instead.

dividedmind · 2024-03-14T05:38:28Z

_appmap/test/test_django.py

+            **server_env,
+        }
+
+    xprocess.ensure("myserver", Starter)


I think it would be better to use django_test_server or something more meaningful here, even if it's unlikely to clash. (Same for flask.)

dividedmind · 2024-03-14T05:39:14Z

_appmap/test/test_django.py

+            "PYTHONUNBUFFERED": "1",
+            "APPMAP_OUTPUT_DIR": "/tmp",
+            **server_env,
+        }


Would it be a good idea to set terminate_on_interrupt = True?

Yeah, I noticed that too, and made that change here: https://github.com/getappmap/appmap-python/pull/289/files#diff-3ea36cadab578779c37a4b328bf128e35b5b45e54aee717aed08f96748114bd5R92 .

I'll bring it over.

dividedmind · 2024-03-14T05:45:39Z

_appmap/test/test_flask.py

+            f"cd {Path(__file__).parent / 'data'/ 'flask'};"
+            + f" {sys.executable} -m flask run"
+            + f" -p {port}",
+        ]


Instead of cd-ing in a bash script, I believe you can instead pass cwd= in popen_kwargs for clarity and portability. (Same for django.)

Using cwd= is currently broken, as I mentioned here: 27dec47#diff-3ea36cadab578779c37a4b328bf128e35b5b45e54aee717aed08f96748114bd5R92 .

Other versions of test_flask.py and test_django.py had this comment, too, but I guess it got deleted along the way. I'll put it back.

dividedmind · 2024-03-14T05:47:19Z

_appmap/test/test_flask.py

+                return True
+            except ConnectionRefusedError:
+                pass
+            return False


Those Django and Flask servers are awfully similar, consider whether it's possible (and worthwhile) to DRY them up.

Agreed. Let me see if it'd be a quick change.

appmap/flask.py

dividedmind · 2024-03-14T05:57:42Z

_appmap/web_framework.py

@@ -145,7 +147,7 @@ def after_request_main(self, rec, status, headers, start, call_event_id) -> None
            parent_id=call_event_id,
            elapsed=duration,
            status_code=status,
-            headers=dict(headers.items()),


Is there any place where we're actually matching them outside of tests? I think it might be a better idea to normalize the header names in appmaps instead.

dividedmind · 2024-03-14T10:26:22Z

_appmap/test/normalize.py

@@ -82,6 +86,7 @@ def normalize(dct):
            if len(dct["headers"]) == 0:
                del dct["headers"]
        if "http_server_request" in dct:
+            dct["http_server_request"].pop("headers", None)


I'd rather we kept checking for the headers. I assume you changed it because FastAPI formats them differently. If you normalize the capitalization before generating the AppMap like I suggested, you should be able to bring it back.

I updated the tests to check that the http_server_request properties that depend on header values get set correctly. I can update this so it verifies that headers is present.

Is the combination of those two checks sufficient?

OK, though maybe it could use a comment.

_appmap/test/test_recording.py

_appmap/utils.py

dividedmind · 2024-03-14T10:32:48Z

_appmap/web_framework.py

@@ -142,6 +142,7 @@ def before_request_main(self, rec, req: Any) -> Tuple[float, int]:
        raise NotImplementedError

    def after_request_main(self, rec, status, headers, start, call_event_id) -> None:
+


Is this intended?

Remove our bespoke test server management, use pytest-xprocess instead. Ensures that pytest doesn't hang if a test fails, and that the tests run correctly in VS Code (maybe also on Windows, still to be determined). Also fixes a couple of issues identified by pylint.

Add support for the FastAPI framework. Load it automatically when uvicorn is run with a FastAPI app.

Create a log file by default, messages at INFO and above to it. Provide the env var APPMAP_DISABLE_LOG_FILE to instead log (at WARNING, by default) to stderr.

apotterri force-pushed the fastapi_20240227 branch 4 times, most recently from 682f1e9 to 6558881 Compare March 4, 2024 13:46

apotterri mentioned this pull request Mar 4, 2024

Add FastAPI support #282

Closed

apotterri force-pushed the fastapi_20240227 branch 3 times, most recently from 1f11b75 to 5811e44 Compare March 7, 2024 13:59

apotterri force-pushed the fastapi_20240227 branch from 5eb86d8 to b88a040 Compare March 11, 2024 18:17

apotterri added 2 commits March 11, 2024 16:15

refactor: update importlib

56e672e

importlib.metadata was added in 3.8. Now that we no longer support 3.7, we can use it directly, rather than using the backported implementation. (We still need to use the backported version of importlib_resources until we drop 3.8.)

apotterri force-pushed the fastapi_20240227 branch 2 times, most recently from 84cf3a7 to a7fd460 Compare March 11, 2024 20:46

apotterri commented Mar 11, 2024

View reviewed changes

apotterri force-pushed the fastapi_20240227 branch 2 times, most recently from aac19d7 to 27dec47 Compare March 11, 2024 21:21

apotterri marked this pull request as ready for review March 11, 2024 22:11

dustinbyrne requested a review from dividedmind March 12, 2024 14:05

dividedmind approved these changes Mar 14, 2024

View reviewed changes

apotterri force-pushed the fastapi_20240227 branch 2 times, most recently from 6af4508 to 1ac9865 Compare March 15, 2024 13:35

apotterri added 2 commits March 15, 2024 10:07

feat: FastAPI support

3dba2a2

Add support for the FastAPI framework. Load it automatically when uvicorn is run with a FastAPI app.

fix: log to a file by default

3da004f

Create a log file by default, messages at INFO and above to it. Provide the env var APPMAP_DISABLE_LOG_FILE to instead log (at WARNING, by default) to stderr.

apotterri force-pushed the fastapi_20240227 branch from 1ac9865 to 3da004f Compare March 15, 2024 14:07

apotterri merged commit 37fcb7c into master Mar 15, 2024
1 check passed

apotterri deleted the fastapi_20240227 branch March 15, 2024 14:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add FastAPI support #287

Add FastAPI support #287

apotterri commented Feb 28, 2024 •

edited

Loading

apotterri Mar 11, 2024

dividedmind Mar 14, 2024 •

edited

Loading

dividedmind Mar 14, 2024

dividedmind Mar 14, 2024

apotterri Mar 14, 2024

dividedmind Mar 14, 2024

apotterri Mar 14, 2024

dividedmind Mar 14, 2024

apotterri Mar 14, 2024

dividedmind Mar 14, 2024 •

edited

Loading

dividedmind Mar 14, 2024

apotterri Mar 14, 2024

dividedmind Mar 14, 2024

dividedmind Mar 14, 2024

		@@ -142,6 +142,7 @@ def before_request_main(self, rec, req: Any) -> Tuple[float, int]:
		raise NotImplementedError

		def after_request_main(self, rec, status, headers, start, call_event_id) -> None:

Add FastAPI support #287

Add FastAPI support #287

Conversation

apotterri commented Feb 28, 2024 • edited Loading

Choose a reason for hiding this comment

dividedmind Mar 14, 2024 • edited Loading

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

dividedmind Mar 14, 2024 • edited Loading

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

apotterri commented Feb 28, 2024 •

edited

Loading

dividedmind Mar 14, 2024 •

edited

Loading

dividedmind Mar 14, 2024 •

edited

Loading