feat: Record by default #176

symwell · 2022-10-04T20:12:13Z

Record appmaps by default.

Fixes #166

appmap/test/test_detect_enabled.py

appmap/_implementation/detect_enabled.py

appmap/_implementation/testing_framework.py

appmap/test/test_detect_enabled.py

apotterri

As I mentioned in one of my comments, please use one or more fixup commits when you make changes after a review. It'll make it much easier see the new code.

appmap/_implementation/detect_enabled.py

apotterri · 2022-10-07T20:44:09Z

appmap/_implementation/detect_enabled.py

+
+    @classmethod
+    def is_appmap_repo(cls):
+        return os.path.exists("appmap/_implementation/event.py") and os.path.exists(


As I said in Slack, I think this would be better managed with an environment variable.

As a developer I would prefer to not have to define an environment variable, to not have to document a variable, to not have to read documentation to find if a variable should be defined, and to not have to think if I should comment-out any environment variable already defined depending on whether I'm developing for appmap-python or using it. I would prefer this decision to be derived automatically.

I still don't love this solution. But, if we have to go this way, let's at least test for a file that's less likely to move around if we refactor, e.g. appmap/__init__.py, or appmap/_implementation/__init__.py.

appmap/_implementation/detect_enabled.py

appmap/test/test_detect_enabled.py

apotterri · 2022-10-07T21:14:32Z

appmap/_implementation/detect_enabled.py

+    @classmethod
+    def initialize(cls):
+        cls._instance = None
+        # because apparently __new__ and __init__ don't get called


Right, because you're not actually asking to create an instance, i.e. you don't say DetectEnabled() anywhere.

When I try, whether I say DetectEnabled() or not, self._detected_for_method = {} from __init__ doesn't get set.

It looks to me like it gets set. Maybe I don't understand what you mean?

diff --git a/appmap/_implementation/detect_enabled.py b/appmap/_implementation/detect_enabled.py index 6d03c7b..aeea80c 100644 --- a/appmap/_implementation/detect_enabled.py +++ b/appmap/_implementation/detect_enabled.py @@ -38,11 +38,12 @@ class DetectEnabled: def initialize(cls): cls._instance = None # because apparently __new__ and __init__ don't get called - cls._detected_for_method = {} + # cls._detected_for_method = {} @classmethod def clear_cache(cls): - cls._detected_for_method = {} + # cls._detected_for_method = {} + pass @classmethod def is_appmap_repo(cls): sw/feat/record_by_default *$ ajp@Alans-MacBook-Pro appmap-python-2 % python Python 3.10.7 (main, Sep 15 2022, 04:53:36) [Clang 13.1.6 (clang-1316.0.21.2.5)] on darwin Type "help", "copyright", "credits" or "license" for more information. >>> from appmap._implementation.detect_enabled import DetectEnabled >>> de = DetectEnabled() >>> print(de._detected_for_method) {} >>>

If I apply the first diff -- if I comment out cls._detected_for_method = {} in initialize -- the Flask testcases don't pass

poetry run pytest appmap/test/test_flask.py

Well, ok, but that's not (necessarily) the same thing as "init doesn't set a property of self", which is what it sounds like you meant.

appmap/_implementation/detect_enabled.py

appmap/django.py

appmap/test/test_detect_enabled.py

apotterri

Somehow, GitHub decided that some of my comments should be one-offs, and others should be part of a review. So, I'm just submitting this as "general feedback"....

apotterri · 2022-10-10T20:12:42Z

appmap/_implementation/detect_enabled.py

+
+    @classmethod
+    def is_appmap_repo(cls):
+        return os.path.exists("appmap/_implementation/event.py") and os.path.exists(


I still don't love this solution. But, if we have to go this way, let's at least test for a file that's less likely to move around if we refactor, e.g. appmap/__init__.py, or appmap/_implementation/__init__.py.

apotterri · 2022-10-10T20:14:34Z

appmap/_implementation/detect_enabled.py

+    @classmethod
+    def initialize(cls):
+        cls._instance = None
+        # because apparently __new__ and __init__ don't get called


Well, ok, but that's not (necessarily) the same thing as "init doesn't set a property of self", which is what it sounds like you meant.

symwell · 2022-10-11T19:30:33Z

This is ready to review. The build passes now.

Had to change a lot of testcases after the recent rebase.

apotterri

One more issue to resolve, then this should be ready for a final rebase.

apotterri · 2022-10-11T20:48:09Z

appmap/flask.py

        self.app = app
        if app is not None:
            self.init_app(app)

    def init_app(self, app):
-        if not Env.current.enabled:


Removing all these checks means that even if recording is disabled (e.g. with APPMAP=false), the remote-recording routes will still be added to the app, right? Is there some reason that's necessary?

That's right, the remote-recording routes will still be added.

Without removing this check some testcase wasn't working. I think it was test_starts_disabled

I wish my commit history wasn't squashed into a single commit so that I could find through the commit comments which testcase this was.

Making this change required adding an extra check in record_get, which is the corresponding check of if not Env.current.enabled.

I don't think we want the routes added if recording is disabled. To fix test_starts_disabled, couldn't you instead make sure that remote recording is enabled (e.g. with APPMAP=true?

This was before the rebase of the events pr, so let me try to put it back and see what I get.

It was failing two other testcases, not the one I said:

FAILED appmap/test/test_flask.py::TestRecordRequestsFlask::test_record_request_appmap_not_enabled_requests_enabled_and_remote - AssertionError FAILED appmap/test/test_flask.py::TestRecordRequestsFlask::test_record_request_appmap_not_enabled_requests_enabled_no_remote - KeyError: 'appmap-file-name'

Also, it was failing this testcases only for Flask. Apparently there was no such check for the routes for Django.

I think it's because I was trying to test the case of APPMAP=false (appmap not enabled) but then enabling remote recording through the testcase issuing a POST.

My understanding is that remote recording and APPMAP=true are separate features. Is this true, or is remote recording possible only if APPMAP=true?

Is this true, or is remote recording possible only if APPMAP=true?

Remote recording is only possible if APPMAP=true.

Apparently there was no such check for the routes for Django.

This seems like a bug.

I think something's wrong. In test_can_record I'm seeing remote requests enabled with a POST but then by the time the DELETE is issued to disable them recording is already off. As if some .stop_recording() call disabled recording.

apotterri · 2022-10-12T08:54:28Z

I wish my commit history wasn't squashed into a single commit so that I could find through the commit comments which testcase this was.

That seems reasonable. It's basically the workflow described here: https://git-scm.com/docs/git-rebase#_interactive_mode, amended for GitHub PRs:

have a wonderful idea
hack on the code
prepare a series for submission, request a review

a. apply fixup! commits to address review comments

b. re-request review

c. repeat a and b as necessary
final history cleanup, squashing original commits into one or two final, orthogonal commits

My experience has been that step 4 can be somewhat challenging, if steps 1-3 have touched many files. Sometimes you can avoid this by making the initial task smaller, but not always.

…'s not possible to test for appmap_not_enabled_requests_enabled_and_remote: when APPMAP=false the routes for remote recording are disabled

…=true

…istent with Django that uses recorder.

apotterri reviewed Oct 4, 2022

View reviewed changes

appmap/test/test_detect_enabled.py Outdated Show resolved Hide resolved

apotterri reviewed Oct 4, 2022

View reviewed changes

appmap/test/test_detect_enabled.py Outdated Show resolved Hide resolved

apotterri reviewed Oct 4, 2022

View reviewed changes

appmap/_implementation/detect_enabled.py Outdated Show resolved Hide resolved

apotterri reviewed Oct 4, 2022

View reviewed changes

appmap/_implementation/testing_framework.py Outdated Show resolved Hide resolved

apotterri reviewed Oct 4, 2022

View reviewed changes

appmap/test/test_detect_enabled.py Show resolved Hide resolved

symwell force-pushed the sw/feat/record_by_default branch 25 times, most recently from fdaa71c to 7cfc095 Compare October 6, 2022 18:43

symwell force-pushed the sw/feat/record_by_default branch 2 times, most recently from c94d583 to 3168889 Compare October 7, 2022 13:48

symwell marked this pull request as ready for review October 7, 2022 13:57

symwell force-pushed the sw/feat/record_by_default branch from 3168889 to b731575 Compare October 7, 2022 14:19

apotterri self-requested a review October 7, 2022 14:23

symwell force-pushed the sw/feat/record_by_default branch 4 times, most recently from 99b2d6c to f5f7b30 Compare October 7, 2022 17:42

apotterri suggested changes Oct 7, 2022

View reviewed changes

symwell force-pushed the sw/feat/record_by_default branch from 3eb313f to 9fe0a46 Compare October 10, 2022 12:51

apotterri reviewed Oct 10, 2022

View reviewed changes

apotterri force-pushed the sw/feat/record_by_default branch 2 times, most recently from 8d9993d to 4dd10e1 Compare October 11, 2022 09:20

symwell requested a review from apotterri October 11, 2022 14:25

symwell force-pushed the sw/feat/record_by_default branch 2 times, most recently from 838d741 to b53cc51 Compare October 11, 2022 18:58

apotterri reviewed Oct 11, 2022

View reviewed changes

symwell force-pushed the sw/feat/record_by_default branch from 4ec8119 to ac54c8a Compare October 12, 2022 13:35

apotterri approved these changes Oct 12, 2022

View reviewed changes

symwell added 5 commits October 12, 2022 13:38

feat: record by default

209f118

fix: Remove a testcase for Flask and a testcase for Django because it…

5817b6e

…'s not possible to test for appmap_not_enabled_requests_enabled_and_remote: when APPMAP=false the routes for remote recording are disabled

fix: The remote recording routes for Flask are enabled only if APPMAP…

45a297b

…=true

refactor: Have Flask use recorder rather than recording, to stay cons…

bc29f5b

…istent with Django that uses recorder.

fix: For Flask always set a before_request and an after_request handler

fb73d80

symwell force-pushed the sw/feat/record_by_default branch from 95d17ef to fb73d80 Compare October 12, 2022 17:38

symwell merged commit 717d39a into master Oct 12, 2022

symwell deleted the sw/feat/record_by_default branch October 12, 2022 18:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Record by default #176

feat: Record by default #176

symwell commented Oct 4, 2022 •

edited

Loading

apotterri left a comment

apotterri Oct 7, 2022

symwell Oct 10, 2022

apotterri Oct 10, 2022

apotterri Oct 7, 2022

symwell Oct 10, 2022

apotterri Oct 10, 2022

symwell Oct 10, 2022

apotterri Oct 10, 2022

apotterri left a comment

apotterri Oct 10, 2022

apotterri Oct 10, 2022

symwell commented Oct 11, 2022

apotterri left a comment

apotterri Oct 11, 2022

symwell Oct 11, 2022

symwell Oct 11, 2022

apotterri Oct 11, 2022

symwell Oct 11, 2022 •

edited

Loading

symwell Oct 11, 2022

apotterri Oct 11, 2022 •

edited

Loading

apotterri Oct 11, 2022

symwell Oct 12, 2022

apotterri commented Oct 12, 2022

feat: Record by default #176

feat: Record by default #176

Conversation

symwell commented Oct 4, 2022 • edited Loading

apotterri left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apotterri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

symwell commented Oct 11, 2022

apotterri left a 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

symwell Oct 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apotterri Oct 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apotterri commented Oct 12, 2022

symwell commented Oct 4, 2022 •

edited

Loading

symwell Oct 11, 2022 •

edited

Loading

apotterri Oct 11, 2022 •

edited

Loading