From 6604dd4755dcc44eb4e0011568d81900a3afd17e Mon Sep 17 00:00:00 2001
From: Alan Potter <alanjpotter@gmail.com>
Date: Fri, 28 Oct 2022 17:06:40 -0400
Subject: [PATCH 1/3] fix: use log level INFO by default

Change the default log level to INFO. Downgrade some messages from INFO
to DEBUG.

This change means the user will now see the initial record-by-default
message, but it will be labeled INFO instead of (the scary looking
WARNING.
---
 appmap/_implementation/configuration.py | 2 +-
 appmap/_implementation/env.py           | 2 +-
 appmap/_implementation/importer.py      | 2 +-
 appmap/_implementation/instrument.py    | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/appmap/_implementation/configuration.py b/appmap/_implementation/configuration.py
index 437d32e6..944fc893 100644
--- a/appmap/_implementation/configuration.py
+++ b/appmap/_implementation/configuration.py
@@ -218,7 +218,7 @@ def _load_config(self):
                 Env.current.enabled = should_enable
             except ParserError:
                 pass
-            logger.info("config: %s", self._config)
+            logger.debug("config: %s", self._config)
             return
 
         if not Env.current.enabled:
diff --git a/appmap/_implementation/env.py b/appmap/_implementation/env.py
index f18a2636..8d49f558 100644
--- a/appmap/_implementation/env.py
+++ b/appmap/_implementation/env.py
@@ -80,7 +80,7 @@ def display_params(self):
         return self.get("APPMAP_DISPLAY_PARAMS", "true").lower() == "true"
 
     def _configure_logging(self):
-        log_level = self.get("APPMAP_LOG_LEVEL", "warning").upper()
+        log_level = self.get("APPMAP_LOG_LEVEL", "info").upper()
 
         log_config = self.get("APPMAP_LOG_CONFIG")
         log_stream = self.get("APPMAP_LOG_STREAM", "stderr")
diff --git a/appmap/_implementation/importer.py b/appmap/_implementation/importer.py
index 106658dc..af77de0d 100644
--- a/appmap/_implementation/importer.py
+++ b/appmap/_implementation/importer.py
@@ -187,7 +187,7 @@ def instrument_functions(filterable):
             if not cls.filter_chain.filter(filterable):
                 return
 
-            logger.info("  looking for members of %s", filterable.obj)
+            logger.debug("  looking for members of %s", filterable.obj)
             functions = get_members(filterable.obj)
             logger.debug("  functions %s", functions)
 
diff --git a/appmap/_implementation/instrument.py b/appmap/_implementation/instrument.py
index a1673b60..4eb79b80 100644
--- a/appmap/_implementation/instrument.py
+++ b/appmap/_implementation/instrument.py
@@ -109,7 +109,7 @@ def call_instrumented(f, instance, args, kwargs):
 
 def instrument(filterable):
     """return an instrumented function"""
-    logger.info("hooking %s", filterable.fqname)
+    logger.debug("hooking %s", filterable.fqname)
     fn = filterable.obj
 
     make_call_event = event.CallEvent.make(fn, filterable.fntype)

From 65f106bc107f80110b39f5647716c91f9f6a0929 Mon Sep 17 00:00:00 2001
From: Alan Potter <alanjpotter@gmail.com>
Date: Fri, 4 Nov 2022 14:24:39 -0400
Subject: [PATCH 2/3] fix: allow concurrent remote and request recording

With these changes, remote recording and per-request recording no longer
interfere with one another.
---
 appmap/_implementation/__init__.py         |  3 +-
 appmap/_implementation/django.py           | 34 +++++++++++++
 appmap/_implementation/event.py            |  2 +-
 appmap/_implementation/flask.py            | 45 +++++++-----------
 appmap/_implementation/recorder.py         | 51 +++++++++++++-------
 appmap/_implementation/remote_recording.py | 45 ++++++++++++++++++
 appmap/_implementation/web_framework.py    | 55 ++++++++++------------
 appmap/django.py                           | 52 +++++---------------
 appmap/flask.py                            | 17 +++----
 appmap/test/test_flask.py                  |  6 ++-
 appmap/test/test_recording.py              |  1 -
 appmap/test/web_framework.py               |  3 +-
 tox.ini                                    |  2 +-
 13 files changed, 187 insertions(+), 129 deletions(-)
 create mode 100644 appmap/_implementation/django.py
 create mode 100644 appmap/_implementation/remote_recording.py

diff --git a/appmap/_implementation/__init__.py b/appmap/_implementation/__init__.py
index 9911d333..b86831e3 100644
--- a/appmap/_implementation/__init__.py
+++ b/appmap/_implementation/__init__.py
@@ -1,6 +1,6 @@
 from . import configuration
 from . import env as appmapenv
-from . import event, importer, metadata, recorder
+from . import event, importer, metadata, recorder, web_framework
 from .py_version_check import check_py_version
 
 
@@ -12,6 +12,7 @@ def initialize(**kwargs):
     recorder.initialize()
     configuration.initialize()  # needs to be initialized after recorder
     metadata.initialize()
+    web_framework.initialize()
 
 
 initialize()
diff --git a/appmap/_implementation/django.py b/appmap/_implementation/django.py
new file mode 100644
index 00000000..73becb47
--- /dev/null
+++ b/appmap/_implementation/django.py
@@ -0,0 +1,34 @@
+"""
+This file contains Django middleware that can be inserted into an app's stack to expose the remote
+recording endpoint.
+
+It expects the importer to have verified that Django is available.
+"""
+
+from http import HTTPStatus
+
+from django.http import HttpRequest, HttpResponse
+
+from . import remote_recording
+
+
+class RemoteRecording:
+    def __init__(self, get_response):
+        super().__init__()
+        self.get_response = get_response
+
+    def __call__(self, request: HttpRequest):
+        if request.path != "/_appmap/record":
+            return self.get_response(request)
+
+        handlers = {
+            "GET": remote_recording.status,
+            "POST": remote_recording.start,
+            "DELETE": remote_recording.stop,
+        }
+
+        def not_allowed():
+            return "", HTTPStatus.METHOD_NOT_ALLOWED
+
+        body, status = handlers.get(request.method, not_allowed)()
+        return HttpResponse(body, status=status, content_type="application/json")
diff --git a/appmap/_implementation/event.py b/appmap/_implementation/event.py
index b0bd128e..e54f6851 100644
--- a/appmap/_implementation/event.py
+++ b/appmap/_implementation/event.py
@@ -90,7 +90,7 @@ class Event:
     __slots__ = ["id", "event", "thread_id"]
 
     def __init__(self, event):
-        self.id = Recorder.get_current().next_event_id()
+        self.id = Recorder.next_event_id()
         self.event = event
         self.thread_id = _EventIds.get_thread_id()
 
diff --git a/appmap/_implementation/flask.py b/appmap/_implementation/flask.py
index 66932d6b..ff892131 100644
--- a/appmap/_implementation/flask.py
+++ b/appmap/_implementation/flask.py
@@ -1,39 +1,30 @@
-""" remote_recording is a Flask app that can be mounted to expose the remote-recording endpoint. """
-import json
+"""
+This file contains a Flask app that is mounted on /_appmap to expose the remote-recording endpoint
+in a user's app.
 
-from flask import Flask
+It should only be imported if other code has already verified that Flask is available.
+"""
 
-from . import generation
-from .recorder import Recorder
-from .web_framework import AppmapMiddleware
+from flask import Flask, Response
 
-remote_recording = Flask(__name__)
+from . import remote_recording
 
+app = Flask(__name__)
 
-@remote_recording.route("/record", methods=["GET"])
-def status():
-    if not AppmapMiddleware.should_record():
-        return "Appmap is disabled.", 404
 
-    return {"enabled": Recorder.get_current().get_enabled()}
+@app.route("/record", methods=["GET"])
+def status():
+    body, status = remote_recording.status()
+    return Response(body, status=status, mimetype="application/json")
 
 
-@remote_recording.route("/record", methods=["POST"])
+@app.route("/record", methods=["POST"])
 def start():
-    r = Recorder.get_current()
-    if r.get_enabled():
-        return "Recording is already in progress", 409
-
-    r.start_recording()
-    return "", 200
+    body, status = remote_recording.start()
+    return Response(body, status=status, mimetype="application/json")
 
 
-@remote_recording.route("/record", methods=["DELETE"])
+@app.route("/record", methods=["DELETE"])
 def stop():
-    r = Recorder.get_current()
-    if not r.get_enabled():
-        return "No recording is in progress", 404
-
-    r.stop_recording()
-
-    return json.loads(generation.dump(r))
+    body, status = remote_recording.stop()
+    return Response(body, status=status, mimetype="application/json")
diff --git a/appmap/_implementation/recorder.py b/appmap/_implementation/recorder.py
index fc4209d5..ac158abd 100644
--- a/appmap/_implementation/recorder.py
+++ b/appmap/_implementation/recorder.py
@@ -2,6 +2,7 @@
 import threading
 import traceback
 from abc import ABC, abstractmethod
+from copy import copy
 
 from appmap._implementation.utils import appmap_tls
 
@@ -22,10 +23,14 @@ class Recorder(ABC):
     def events(self):
         return self._events
 
-    @abstractmethod
-    def next_event_id(self):
-        self._next_event_id += 1
-        return self._next_event_id
+    _next_event_id = 0
+    _next_event_id_lock = threading.Lock()
+
+    @classmethod
+    def next_event_id(cls):
+        with cls._next_event_id_lock:
+            cls._next_event_id += 1
+            return cls._next_event_id
 
     # It might be nice to put @property on the getters here. The python maintainers have gone back
     # and forth on whether you should be able to combine @classmethod and @property. In 3.11,
@@ -44,9 +49,25 @@ def set_current(cls, r):
         """
         Set the recorder for the current thread.
         """
-        appmap_tls()[cls._RECORDER_KEY] = r
+        tls = appmap_tls()
+        if r:
+            tls[cls._RECORDER_KEY] = r
+        else:
+            del tls[cls._RECORDER_KEY]
+
         return r
 
+    @classmethod
+    def get_global(cls):
+        _, shared = cls._get_current()
+        return shared
+
+    @classmethod
+    def new_global(cls):
+        global _default_recorder
+        _default_recorder = SharedRecorder()
+        return _default_recorder
+
     @classmethod
     def get_enabled(cls):
         return cls.get_current()._enabled
@@ -85,13 +106,11 @@ def _get_current(cls):
 
     def clear(self):
         self._events = []
-        self._next_event_id = 0
 
     def __init__(self, enabled=False):
         self._events = []
         self._enabled = enabled
         self.start_tb = None
-        self._next_event_id = 0
 
     @abstractmethod
     def _start_recording(self):
@@ -122,8 +141,7 @@ def _initialize():
         threads initializing the default recorder. If you find yourself wanting to do that, you
         should probably be using per-thread recording.
         """
-        global _default_recorder
-        _default_recorder = SharedRecorder()
+        Recorder.new_global()
 
 
 class ThreadRecorder(Recorder):
@@ -135,9 +153,6 @@ class ThreadRecorder(Recorder):
     def events(self):
         return super().events
 
-    def next_event_id(self):
-        return super().next_event_id()
-
     def _start_recording(self):
         super()._start_recording()
 
@@ -155,6 +170,14 @@ class SharedRecorder(Recorder):
 
     _lock = threading.Lock()
 
+    def __init__(self):
+        super().__init__()
+        Recorder._next_event_id = 0
+
+    def clear(self):
+        super().clear()
+        Recorder._next_event_id = 0
+
     @property
     def events(self):
         with self._lock:
@@ -168,10 +191,6 @@ def _stop_recording(self):
         with self._lock:
             return super()._stop_recording()
 
-    def next_event_id(self):
-        with self._lock:
-            return super().next_event_id()
-
     def _add_event(self, event):
         with self._lock:
             super()._add_event(event)
diff --git a/appmap/_implementation/remote_recording.py b/appmap/_implementation/remote_recording.py
new file mode 100644
index 00000000..a7504316
--- /dev/null
+++ b/appmap/_implementation/remote_recording.py
@@ -0,0 +1,45 @@
+""" remote_recording is a Flask app that can be mounted to expose the remote-recording endpoint. """
+import json
+from threading import Lock
+
+from . import generation
+from .detect_enabled import DetectEnabled
+from .recorder import Recorder
+
+_enabled_lock = Lock()
+_enabled = False
+
+
+def status():
+    if not DetectEnabled.should_enable("remote"):
+        return "Appmap is disabled.", 404
+
+    with _enabled_lock:
+        return json.dumps({"enabled": _enabled}), 200
+
+
+def start():
+    global _enabled
+    with _enabled_lock:
+        if _enabled:
+            return "Recording is already in progress", 409
+
+        Recorder.new_global().start_recording()
+        _enabled = True
+        return "", 200
+
+
+def stop():
+    global _enabled
+    with _enabled_lock:
+        if not _enabled:
+            return "No recording is in progress", 404
+        r = Recorder.get_global()
+        r.stop_recording()
+        _enabled = False
+        return generation.dump(r), 200
+
+
+def initialize():
+    global _enabled
+    _enabled = False
diff --git a/appmap/_implementation/web_framework.py b/appmap/_implementation/web_framework.py
index f063e86e..197dd968 100644
--- a/appmap/_implementation/web_framework.py
+++ b/appmap/_implementation/web_framework.py
@@ -6,15 +6,16 @@
 import re
 import time
 from abc import ABC, abstractmethod
+from contextvars import ContextVar
 from hashlib import sha256
 from tempfile import NamedTemporaryFile
 
-from appmap._implementation import generation
-from appmap._implementation.detect_enabled import DetectEnabled
-from appmap._implementation.env import Env
-from appmap._implementation.event import Event, ReturnEvent, _EventIds, describe_value
-from appmap._implementation.recorder import Recorder, ThreadRecorder
-from appmap._implementation.utils import root_relative_path, scenario_filename
+from . import generation, remote_recording
+from .detect_enabled import DetectEnabled
+from .env import Env
+from .event import Event, ReturnEvent, _EventIds, describe_value
+from .recorder import Recorder, ThreadRecorder
+from .utils import root_relative_path, scenario_filename
 
 
 class TemplateEvent(Event):  # pylint: disable=too-few-public-methods
@@ -130,6 +131,9 @@ def create_appmap_file(
     headers["AppMap-File-Name"] = os.path.abspath(appmap_file_path) + APPMAP_SUFFIX
 
 
+request_recorder = ContextVar("appmap_request_recorder")
+
+
 class AppmapMiddleware(ABC):
     def __init__(self):
         self.record_url = "/_appmap/record"
@@ -140,7 +144,7 @@ def should_record():
             "requests"
         )
 
-    def before_request_hook(self, request, request_path, recording_is_running):
+    def before_request_hook(self, request, request_path):
         if request_path == self.record_url:
             return None, None, None
 
@@ -152,15 +156,9 @@ def before_request_hook(self, request, request_path, recording_is_running):
             rec = ThreadRecorder()
             Recorder.set_current(rec)
             rec.start_recording()
-            # Each time an event is added for a thread_id it's also
-            # added to the global Recorder().  So don't add the event
-            # to the global Recorder() explicitly because that would
-            # add the event in it twice.
-        elif DetectEnabled.should_enable("remote") or recording_is_running:
-            # b) APPMAP=true, or
-            # c) remote, enabled by POST to /_appmap/record, which set
-            #    recording_is_running
-            rec = Recorder.get_current()
+            request_recorder.set(rec)
+        elif DetectEnabled.should_enable("remote"):
+            rec = Recorder.get_global()
 
         if rec and rec.get_enabled():
             start, call_event_id = self.before_request_main(rec, request)
@@ -173,9 +171,7 @@ def before_request_main(self, rec):
 
     def after_request_hook(
         self,
-        request,
         request_path,
-        recording_is_running,
         request_method,
         request_base_url,
         response,
@@ -188,14 +184,9 @@ def after_request_hook(
 
         if DetectEnabled.should_enable("requests"):
             # a) requests
-            rec = Recorder.get_current()
-            # Each time an event is added for a thread_id it's also
-            # added to the global Recorder().  So don't add the event
-            # to the global Recorder() explicitly because that would
-            # add the event in it twice.
+            rec = request_recorder.get()
             try:
-                if rec.get_enabled():
-                    self.after_request_main(rec, response, start, call_event_id)
+                self.after_request_main(rec, response, start, call_event_id)
 
                 output_dir = Env.current.output_dir / "requests"
                 create_appmap_file(
@@ -209,11 +200,11 @@ def after_request_hook(
                 )
             finally:
                 rec.stop_recording()
-        elif DetectEnabled.should_enable("remote") or recording_is_running:
-            # b) APPMAP=true, or
-            # c) remote, enabled by POST to /_appmap/record, which set
-            #    recording_is_running
-            rec = Recorder.get_current()
+                Recorder.set_current(None)
+                request_recorder.set(None)
+
+        elif DetectEnabled.should_enable("remote"):
+            rec = Recorder.get_global()
             if rec.get_enabled():
                 self.after_request_main(rec, response, start, call_event_id)
 
@@ -222,3 +213,7 @@ def after_request_hook(
     @abstractmethod
     def after_request_main(self, rec, response, start, call_event_id):
         pass
+
+
+def initialize():
+    remote_recording.initialize()
diff --git a/appmap/django.py b/appmap/django.py
index 4e640a65..d5a658da 100644
--- a/appmap/django.py
+++ b/appmap/django.py
@@ -22,23 +22,22 @@
 from django.urls.exceptions import Resolver404
 from django.urls.resolvers import _route_to_regex
 
-from appmap._implementation import generation, recording, web_framework
-from appmap._implementation.env import Env
-from appmap._implementation.event import (
+from appmap._implementation.detect_enabled import DetectEnabled
+
+from ._implementation import generation
+from ._implementation.event import (
     ExceptionEvent,
     HttpServerRequestEvent,
     HttpServerResponseEvent,
     ReturnEvent,
     SqlEvent,
-    _EventIds,
 )
-from appmap._implementation.instrument import is_instrumentation_disabled
-from appmap._implementation.recorder import Recorder
-from appmap._implementation.web_framework import AppmapMiddleware
-from appmap._implementation.web_framework import TemplateHandler as BaseTemplateHandler
-
+from ._implementation.instrument import is_instrumentation_disabled
 from ._implementation.metadata import Metadata
+from ._implementation.recorder import Recorder
 from ._implementation.utils import patch_class, values_dict
+from ._implementation.web_framework import AppmapMiddleware
+from ._implementation.web_framework import TemplateHandler as BaseTemplateHandler
 
 
 def parse_pg_version(version):
@@ -233,12 +232,7 @@ def __call__(self, request):
         if not self.should_record():
             return self.get_response(request)
 
-        if request.path_info == self.record_url:
-            return self.recording(request)
-
-        rec, start, call_event_id = self.before_request_hook(
-            request, request.path_info, self.recorder.get_enabled()
-        )
+        rec, start, call_event_id = self.before_request_hook(request, request.path_info)
 
         try:
             response = self.get_response(request)
@@ -254,9 +248,7 @@ def __call__(self, request):
             raise
 
         self.after_request_hook(
-            request,
             request.path_info,
-            self.recorder.get_enabled(),
             request.method,
             request.build_absolute_uri(),
             response,
@@ -302,32 +294,14 @@ def after_request_main(self, rec, response, start, call_event_id):
         )
         rec.add_event(return_event)
 
-    def recording(self, request):
-        """Handle recording requests."""
-        if request.method == "GET":
-            return JsonResponse({"enabled": self.recorder.get_enabled()})
-        if request.method == "POST":
-            if self.recorder.get_enabled():
-                return HttpResponse("Recording is already in progress", status=409)
-            self.recorder.start_recording()
-            return HttpResponse()
-
-        if request.method == "DELETE":
-            if not self.recorder.get_enabled():
-                return HttpResponse("No recording is in progress", status=404)
-
-            self.recorder.stop_recording()
-            return HttpResponse(
-                generation.dump(self.recorder), content_type="application/json"
-            )
-
-        return HttpResponseBadRequest()
-
 
 def inject_middleware():
     """Make sure AppMap middleware is added to the stack"""
     if "appmap.django.Middleware" not in settings.MIDDLEWARE:
-        settings.MIDDLEWARE.insert(0, "appmap.django.Middleware")
+        new_middleware = ["appmap.django.Middleware"]
+        if DetectEnabled.should_enable("remote"):
+            new_middleware.insert(0, "appmap._implementation.django.RemoteRecording")
+        settings.MIDDLEWARE[0:0] = new_middleware
 
 
 original_load_middleware = BaseHandler.load_middleware
diff --git a/appmap/flask.py b/appmap/flask.py
index 7a141ecc..ba946ac8 100644
--- a/appmap/flask.py
+++ b/appmap/flask.py
@@ -1,10 +1,12 @@
+import pdb
 import re
+import sys
 import time
 
 import flask
 import flask.cli
 import jinja2
-from flask import _app_ctx_stack, request
+from flask import _app_ctx_stack, current_app, request
 from flask.cli import ScriptInfo
 from werkzeug.exceptions import BadRequest
 from werkzeug.middleware.dispatcher import DispatcherMiddleware
@@ -13,7 +15,7 @@
 from appmap._implementation.detect_enabled import DetectEnabled
 from appmap._implementation.env import Env
 from appmap._implementation.event import HttpServerRequestEvent, HttpServerResponseEvent
-from appmap._implementation.flask import remote_recording
+from appmap._implementation.flask import app as remote_recording_app
 from appmap._implementation.recorder import Recorder
 from appmap._implementation.web_framework import AppmapMiddleware
 from appmap._implementation.web_framework import TemplateHandler as BaseTemplateHandler
@@ -67,12 +69,9 @@ def __init__(self):
         super().__init__()
 
     def init_app(self, app):
-        if self.should_record():
-            self.recorder = Recorder.get_current()
-
         if DetectEnabled.should_enable("remote"):
             app.wsgi_app = DispatcherMiddleware(
-                app.wsgi_app, {"/_appmap": remote_recording}
+                app.wsgi_app, {"/_appmap": remote_recording_app}
             )
 
         app.before_request(self.before_request)
@@ -82,9 +81,7 @@ def before_request(self):
         if not self.should_record():
             return
 
-        rec, start, call_event_id = self.before_request_hook(
-            request, request.path, self.recorder.get_enabled()
-        )
+        self.before_request_hook(request, request.path)
 
     def before_request_main(self, rec, request):
         Metadata.add_framework("flask", flask.__version__)
@@ -121,9 +118,7 @@ def after_request(self, response):
             return response
 
         return self.after_request_hook(
-            request,
             request.path,
-            self.recorder.get_enabled(),
             request.method,
             request.base_url,
             response,
diff --git a/appmap/test/test_flask.py b/appmap/test/test_flask.py
index bd8fe3d4..27d32adf 100644
--- a/appmap/test/test_flask.py
+++ b/appmap/test/test_flask.py
@@ -103,7 +103,11 @@ def server_start(env_vars_str):
     @staticmethod
     def server_stop():
         exec_cmd(
-            "ps -ef | grep -i 'flask run' | grep -v grep | awk '{ print $2 }' | xargs kill -9"
+            "ps -ef"
+            + "| grep -i 'flask run'"
+            + "| grep -v grep"
+            + "| awk '{ print $2 }'"
+            + "| xargs kill -9"
         )
         wait_until_port_is("127.0.0.1", TestRecordRequests.server_port, "closed")
 
diff --git a/appmap/test/test_recording.py b/appmap/test/test_recording.py
index da379685..9a49119a 100644
--- a/appmap/test/test_recording.py
+++ b/appmap/test/test_recording.py
@@ -174,5 +174,4 @@ def add_event(name):
         for n in range(thread_count):
             events = recorders[f"thread{n}"].events
             assert len(events) == 1
-            assert events[0].id == 1
             assert events[0].event["name"] == f"thread{n}"
diff --git a/appmap/test/web_framework.py b/appmap/test/web_framework.py
index 14310f2d..ea6cc2df 100644
--- a/appmap/test/web_framework.py
+++ b/appmap/test/web_framework.py
@@ -246,6 +246,7 @@ def test_appmap_disabled(client, monkeypatch):
         monkeypatch.setenv("APPMAP", "false")
         appmap._implementation.initialize()
         assert not appmap.enabled()
+        assert not DetectEnabled.should_enable("remote")
 
         res = client.get("/_appmap/record")
         assert res.status_code == 404
@@ -280,7 +281,7 @@ def test_can_only_enable_once(client):
     @staticmethod
     @pytest.mark.appmap_enabled
     def test_can_record(data_dir, client, monkeypatch):
-        monkeypatch.setenv("APPMAP_RECORD_REQUESTS", "false")
+        # monkeypatch.setenv("APPMAP_RECORD_REQUESTS", "false")
         res = client.post("/_appmap/record")
         assert res.status_code == 200
 
diff --git a/tox.ini b/tox.ini
index 516c0f07..689fc1dc 100644
--- a/tox.ini
+++ b/tox.ini
@@ -20,7 +20,7 @@ allowlist_externals = env
 commands =
     # Turn off recording while installing. It's not necessary, and the warning messages that come
     # out of the agent confuse poetry.
-    env APPMAP=false poetry install -v
+    env APPMAP_LOG_LEVEL=warning APPMAP=false poetry install -v
     py310-web: poetry run pylint appmap
     web: poetry run {posargs:pytest -vv}
     flask1: poetry run pytest appmap/test/test_flask.py

From 521a910341efcbfb13b7deeac3f5d85a1074ae00 Mon Sep 17 00:00:00 2001
From: Alan Potter <alanjpotter@gmail.com>
Date: Sat, 5 Nov 2022 07:13:40 -0400
Subject: [PATCH 3/3] refactor: clean up some pylint warnings

---
 appmap/_implementation/django.py           |  2 +-
 appmap/_implementation/event.py            |  1 +
 appmap/_implementation/flask.py            | 12 +++++------
 appmap/_implementation/recorder.py         | 21 ++++++++++--------
 appmap/_implementation/remote_recording.py |  1 +
 appmap/_implementation/web_framework.py    |  2 +-
 appmap/django.py                           |  2 --
 appmap/flask.py                            | 25 +++++++++-------------
 appmap/test/test_django.py                 | 25 +++++++++++-----------
 appmap/test/test_flask.py                  | 13 +++++------
 appmap/test/test_recording.py              |  2 --
 appmap/test/web_framework.py               | 17 +++++++--------
 pylintrc                                   |  4 ++--
 tox.ini                                    |  2 +-
 14 files changed, 60 insertions(+), 69 deletions(-)

diff --git a/appmap/_implementation/django.py b/appmap/_implementation/django.py
index 73becb47..da05af35 100644
--- a/appmap/_implementation/django.py
+++ b/appmap/_implementation/django.py
@@ -12,7 +12,7 @@
 from . import remote_recording
 
 
-class RemoteRecording:
+class RemoteRecording:  # pylint: disable=missing-class-docstring
     def __init__(self, get_response):
         super().__init__()
         self.get_response = get_response
diff --git a/appmap/_implementation/event.py b/appmap/_implementation/event.py
index e54f6851..dc7a666b 100644
--- a/appmap/_implementation/event.py
+++ b/appmap/_implementation/event.py
@@ -1,3 +1,4 @@
+# pylint: disable=missing-module-docstring,missing-class-docstring,missing-function-docstring
 import inspect
 import logging
 import threading
diff --git a/appmap/_implementation/flask.py b/appmap/_implementation/flask.py
index ff892131..66a32901 100644
--- a/appmap/_implementation/flask.py
+++ b/appmap/_implementation/flask.py
@@ -14,17 +14,17 @@
 
 @app.route("/record", methods=["GET"])
 def status():
-    body, status = remote_recording.status()
-    return Response(body, status=status, mimetype="application/json")
+    body, rrstatus = remote_recording.status()
+    return Response(body, status=rrstatus, mimetype="application/json")
 
 
 @app.route("/record", methods=["POST"])
 def start():
-    body, status = remote_recording.start()
-    return Response(body, status=status, mimetype="application/json")
+    body, rrstatus = remote_recording.start()
+    return Response(body, status=rrstatus, mimetype="application/json")
 
 
 @app.route("/record", methods=["DELETE"])
 def stop():
-    body, status = remote_recording.stop()
-    return Response(body, status=status, mimetype="application/json")
+    body, rrstatus = remote_recording.stop()
+    return Response(body, status=rrstatus, mimetype="application/json")
diff --git a/appmap/_implementation/recorder.py b/appmap/_implementation/recorder.py
index ac158abd..8c4c6bd3 100644
--- a/appmap/_implementation/recorder.py
+++ b/appmap/_implementation/recorder.py
@@ -2,12 +2,12 @@
 import threading
 import traceback
 from abc import ABC, abstractmethod
-from copy import copy
 
 from appmap._implementation.utils import appmap_tls
 
 logger = logging.getLogger(__name__)
 
+# pylint: disable=global-statement
 _default_recorder = None
 
 
@@ -70,19 +70,19 @@ def new_global(cls):
 
     @classmethod
     def get_enabled(cls):
-        return cls.get_current()._enabled
+        return cls.get_current()._enabled  # pylint: disable=protected-access
 
     @classmethod
     def set_enabled(cls, e):
-        cls.get_current()._enabled = e
+        cls.get_current()._enabled = e  # pylint: disable=protected-access
 
     @classmethod
     def start_recording(cls):
-        cls.get_current()._start_recording()
+        cls.get_current()._start_recording()  # pylint: disable=protected-access
 
     @classmethod
     def stop_recording(cls):
-        return cls.get_current()._stop_recording()
+        return cls.get_current()._stop_recording()  # pylint: disable=protected-access
 
     @classmethod
     def add_event(cls, event):
@@ -91,15 +91,14 @@ def add_event(cls, event):
         one).
         """
         perthread, shared = cls._get_current()
-        shared._add_event(event)
+        shared._add_event(event)  # pylint: disable=protected-access
         if perthread:
-            perthread._add_event(event)
+            perthread._add_event(event)  # pylint: disable=protected-access
 
     _RECORDER_KEY = "appmap_recorder"
 
     @classmethod
     def _get_current(cls):
-        global _default_recorder
         perthread = appmap_tls().get(cls._RECORDER_KEY, None)
 
         return [perthread, _default_recorder]
@@ -153,6 +152,8 @@ class ThreadRecorder(Recorder):
     def events(self):
         return super().events
 
+    # They're not useless, because they're abtract with a default implementation
+    # pragma pylint: disable=useless-super-delegation
     def _start_recording(self):
         super()._start_recording()
 
@@ -162,6 +163,8 @@ def _stop_recording(self):
     def _add_event(self, event):
         super()._add_event(event)
 
+    # pragma pylint: enable=useless-super-delegation
+
 
 class SharedRecorder(Recorder):
     """
@@ -197,4 +200,4 @@ def _add_event(self, event):
 
 
 def initialize():
-    Recorder._initialize()
+    Recorder._initialize()  # pylint: disable=protected-access
diff --git a/appmap/_implementation/remote_recording.py b/appmap/_implementation/remote_recording.py
index a7504316..516a577e 100644
--- a/appmap/_implementation/remote_recording.py
+++ b/appmap/_implementation/remote_recording.py
@@ -6,6 +6,7 @@
 from .detect_enabled import DetectEnabled
 from .recorder import Recorder
 
+# pylint: disable=global-statement
 _enabled_lock = Lock()
 _enabled = False
 
diff --git a/appmap/_implementation/web_framework.py b/appmap/_implementation/web_framework.py
index 197dd968..4b82d91e 100644
--- a/appmap/_implementation/web_framework.py
+++ b/appmap/_implementation/web_framework.py
@@ -13,7 +13,7 @@
 from . import generation, remote_recording
 from .detect_enabled import DetectEnabled
 from .env import Env
-from .event import Event, ReturnEvent, _EventIds, describe_value
+from .event import Event, ReturnEvent, describe_value
 from .recorder import Recorder, ThreadRecorder
 from .utils import root_relative_path, scenario_filename
 
diff --git a/appmap/django.py b/appmap/django.py
index d5a658da..4171a445 100644
--- a/appmap/django.py
+++ b/appmap/django.py
@@ -16,7 +16,6 @@
 from django.db.backends.signals import connection_created
 from django.db.backends.utils import CursorDebugWrapper
 from django.dispatch import receiver
-from django.http import HttpResponse, HttpResponseBadRequest, JsonResponse
 from django.template import Template
 from django.urls import get_resolver, resolve
 from django.urls.exceptions import Resolver404
@@ -24,7 +23,6 @@
 
 from appmap._implementation.detect_enabled import DetectEnabled
 
-from ._implementation import generation
 from ._implementation.event import (
     ExceptionEvent,
     HttpServerRequestEvent,
diff --git a/appmap/flask.py b/appmap/flask.py
index ba946ac8..cf818d9e 100644
--- a/appmap/flask.py
+++ b/appmap/flask.py
@@ -1,12 +1,10 @@
-import pdb
 import re
-import sys
 import time
 
 import flask
 import flask.cli
 import jinja2
-from flask import _app_ctx_stack, current_app, request
+from flask import g, request
 from flask.cli import ScriptInfo
 from werkzeug.exceptions import BadRequest
 from werkzeug.middleware.dispatcher import DispatcherMiddleware
@@ -16,7 +14,6 @@
 from appmap._implementation.env import Env
 from appmap._implementation.event import HttpServerRequestEvent, HttpServerResponseEvent
 from appmap._implementation.flask import app as remote_recording_app
-from appmap._implementation.recorder import Recorder
 from appmap._implementation.web_framework import AppmapMiddleware
 from appmap._implementation.web_framework import TemplateHandler as BaseTemplateHandler
 
@@ -65,9 +62,6 @@ class AppmapFlask(AppmapMiddleware):
     ```
     """
 
-    def __init__(self):
-        super().__init__()
-
     def init_app(self, app):
         if DetectEnabled.should_enable("remote"):
             app.wsgi_app = DispatcherMiddleware(
@@ -87,6 +81,7 @@ def before_request_main(self, rec, request):
         Metadata.add_framework("flask", flask.__version__)
         np = None
         if request.url_rule:
+            # pragma pylint: disable=line-too-long
             # Transform request.url to the expected normalized-path form. For example,
             # "/post/<username>/<post_id>/summary" becomes "/post/{username}/{post_id}/summary".
             # Notes:
@@ -94,6 +89,7 @@ def before_request_main(self, rec, request):
             #   * the variable names in a rule can only contain alphanumerics:
             #     * flask 1: https://github.com/pallets/werkzeug/blob/1dde4b1790f9c46b7122bb8225e6b48a5b22a615/src/werkzeug/routing.py#L143
             #     * flask 2: https://github.com/pallets/werkzeug/blob/99f328cf2721e913bd8a3128a9cdd95ca97c334c/src/werkzeug/routing/rules.py#L56
+            # pragma pylint: enable=line-too-long
             r = repr(request.url_rule)
             np = NP_PARAMS.findall(r)[0].translate(NP_PARAM_DELIMS)
 
@@ -107,10 +103,10 @@ def before_request_main(self, rec, request):
         )
         rec.add_event(call_event)
 
-        appctx = _app_ctx_stack.top
-        appctx.appmap_request_event = call_event
-        appctx.appmap_request_start = time.monotonic()
-
+        # Flask 2 removed the suggestion to use _app_ctx_stack.top, and instead says extensions
+        # should use g with a private property.
+        g._appmap_request_event = call_event  # pylint: disable=protected-access
+        g._appmap_request_start = time.monotonic()  # pylint: disable=protected-access
         return None, None
 
     def after_request(self, response):
@@ -128,10 +124,9 @@ def after_request(self, response):
         )
 
     def after_request_main(self, rec, response, start, call_event_id):
-        appctx = _app_ctx_stack.top
-        parent_id = appctx.appmap_request_event.id
-        duration = time.monotonic() - appctx.appmap_request_start
-
+        parent_id = g._appmap_request_event.id  # pylint: disable=protected-access
+        start = g._appmap_request_start  # pylint: disable=protected-access
+        duration = time.monotonic() - start
         return_event = HttpServerResponseEvent(
             parent_id=parent_id,
             elapsed=duration,
diff --git a/appmap/test/test_django.py b/appmap/test/test_django.py
index b4c65c3c..31f487cd 100644
--- a/appmap/test/test_django.py
+++ b/appmap/test/test_django.py
@@ -1,9 +1,10 @@
 # flake8: noqa: E402
-# pylint: disable=unused-import, redefined-outer-name, missing-function-docstring
+# pylint: disable=missing-function-docstring,redefined-outer-name
 
 import json
 import sys
 from pathlib import Path
+from threading import Thread
 
 import django
 import django.conf
@@ -19,23 +20,17 @@
 import appmap.django  # noqa: F401
 from appmap.test.helpers import DictIncluding
 
+from .._implementation.metadata import Metadata
+from .web_framework import TestRecordRequests, exec_cmd, wait_until_port_is
+
 sys.path += [str(Path(__file__).parent / "data" / "django")]
-import app
 
-from .._implementation.metadata import Metadata
+# Import app just for the side-effects. It must happen after sys.path has been modified.
+import app  # pyright: ignore pylint: disable=import-error, unused-import,wrong-import-order
 
 # Make sure assertions in web_framework get rewritten (e.g. to show
 # diffs in generated appmaps)
 pytest.register_assert_rewrite("appmap.test.web_framework")
-from threading import Thread
-
-from .web_framework import (
-    TestRecording,
-    TestRecordRequests,
-    TestRequestCapture,
-    exec_cmd,
-    wait_until_port_is,
-)
 
 
 @pytest.mark.django_db
@@ -235,7 +230,11 @@ def server_start(env_vars_str):
     @staticmethod
     def server_stop():
         exec_cmd(
-            "ps -ef | grep -i 'manage.py runserver' | grep -v grep | awk '{ print $2 }' | xargs kill -9"
+            "ps -ef"
+            + "| grep -i 'manage.py runserver'"
+            + "| grep -v grep"
+            + "| awk '{ print $2 }'"
+            + "| xargs kill -9"
         )
         wait_until_port_is("127.0.0.1", TestRecordRequests.server_port, "closed")
 
diff --git a/appmap/test/test_flask.py b/appmap/test/test_flask.py
index 27d32adf..f03fdbd9 100644
--- a/appmap/test/test_flask.py
+++ b/appmap/test/test_flask.py
@@ -12,19 +12,16 @@
 from appmap.test.helpers import DictIncluding
 
 from .._implementation.metadata import Metadata
-
-# Make sure assertions in web_framework get rewritten (e.g. to show
-# diffs in generated appmaps)
-pytest.register_assert_rewrite("appmap.test.web_framework")
-
 from .web_framework import (  # pylint: disable=unused-import
-    TestRecording,
     TestRecordRequests,
-    TestRequestCapture,
     exec_cmd,
     wait_until_port_is,
 )
 
+# Make sure assertions in web_framework get rewritten (e.g. to show
+# diffs in generated appmaps)
+pytest.register_assert_rewrite("appmap.test.web_framework")
+
 
 @pytest.fixture(name="client")
 def flask_client(app):
@@ -38,7 +35,7 @@ def flask_app(data_dir, monkeypatch):
 
     Env.current.set("APPMAP_CONFIG", data_dir / "flask" / "appmap.yml")
 
-    import app  # pylint: disable=import-error
+    import app  # pyright: ignore pylint: disable=import-error,import-outside-toplevel
 
     importlib.reload(app)
 
diff --git a/appmap/test/test_recording.py b/appmap/test/test_recording.py
index 9a49119a..cbc6a4f3 100644
--- a/appmap/test/test_recording.py
+++ b/appmap/test/test_recording.py
@@ -108,8 +108,6 @@ def test_can_deepcopy_function(self):
             modfunc,
         )
 
-        from appmap.wrapt import FunctionWrapper
-
         rec = appmap.Recording()
         with rec:
             f1 = deepcopy(modfunc)
diff --git a/appmap/test/web_framework.py b/appmap/test/web_framework.py
index ea6cc2df..7bcc8fc8 100644
--- a/appmap/test/web_framework.py
+++ b/appmap/test/web_framework.py
@@ -244,7 +244,7 @@ class TestRecording:
     def test_appmap_disabled(client, monkeypatch):
         # since APPMAP records by default, disable it explicitly
         monkeypatch.setenv("APPMAP", "false")
-        appmap._implementation.initialize()
+        appmap._implementation.initialize()  # pylint: disable=protected-access
         assert not appmap.enabled()
         assert not DetectEnabled.should_enable("remote")
 
@@ -280,8 +280,7 @@ def test_can_only_enable_once(client):
 
     @staticmethod
     @pytest.mark.appmap_enabled
-    def test_can_record(data_dir, client, monkeypatch):
-        # monkeypatch.setenv("APPMAP_RECORD_REQUESTS", "false")
+    def test_can_record(data_dir, client):
         res = client.post("/_appmap/record")
         assert res.status_code == 200
 
@@ -298,7 +297,7 @@ def test_can_record(data_dir, client, monkeypatch):
         generated_appmap = normalize_appmap(data)
 
         expected_path = data_dir / "remote.appmap.json"
-        with open(expected_path) as expected:
+        with open(expected_path, encoding="utf-8") as expected:
             expected_appmap = json.load(expected)
 
         assert (
@@ -317,7 +316,7 @@ def exec_cmd(command):
         stdin=subprocess.PIPE,
         stderr=subprocess.STDOUT,
     )
-    output, errors = p.communicate()
+    output, _ = p.communicate()
     return output.decode()
 
 
@@ -327,7 +326,7 @@ def port_state(address, port):
     try:
         s.connect((address, port))
         ret = "open"
-    except:
+    except Exception:  # pylint: disable=broad-except
         ret = "closed"
         s.close()
     return ret
@@ -354,7 +353,7 @@ class TestRecordRequests:
     server_port = 8000
 
     @abstractmethod
-    def server_start():
+    def server_start(env_vars_str):
         """
         Start the webserver in the background. Don't block execution.
         """
@@ -406,7 +405,7 @@ def record_request(
             for future in concurrent.futures.as_completed(future_to_request_number):
                 try:
                     response = future.result()
-                except Exception as e:
+                except Exception:  # pylint: disable=broad-except
                     traceback.print_exc()
                 assert response.status_code == 200
 
@@ -434,7 +433,7 @@ def record_request(
                     == "http_127_0_0_1_8000_test.appmap.json"
                 )
 
-                with open(appmap_file_name) as f:
+                with open(appmap_file_name, encoding="utf-8") as f:
                     appmap = json.loads(f.read())
 
                     # Every event should come from the same thread
diff --git a/pylintrc b/pylintrc
index 1232389e..ae737ef2 100644
--- a/pylintrc
+++ b/pylintrc
@@ -6,7 +6,7 @@
 extension-pkg-whitelist=
 
 # Specify a score threshold to be exceeded before program exits with error.
-fail-under=7.6
+fail-under=7.9
 
 # Add files or directories to the blacklist. They should be base names, not
 # paths.
@@ -60,7 +60,7 @@ confidence=
 # --enable=similarities". If you want to run only the classes checker, but have
 # no Warning level messages displayed, use "--disable=all --enable=classes
 # --disable=W".
-disable=
+disable=consider-using-fstring, invalid-name, missing-function-docstring, missing-class-docstring, missing-module-docstring, consider-using-f-string
 
 # Enable the message, report, category or checker with the given id(s). You can
 # either give multiple identifier separated by comma (,) or put this option
diff --git a/tox.ini b/tox.ini
index 689fc1dc..868415b5 100644
--- a/tox.ini
+++ b/tox.ini
@@ -21,7 +21,7 @@ commands =
     # Turn off recording while installing. It's not necessary, and the warning messages that come
     # out of the agent confuse poetry.
     env APPMAP_LOG_LEVEL=warning APPMAP=false poetry install -v
-    py310-web: poetry run pylint appmap
+    py310-web: poetry run pylint -j 0 appmap
     web: poetry run {posargs:pytest -vv}
     flask1: poetry run pytest appmap/test/test_flask.py
     django2: poetry run pytest appmap/test/test_django.py