Merge pull request #305 from getappmap/v2_20240512

disable recording by default

README.md

@@ -87,9 +87,13 @@ reenabled as soon as possible.]
 ### pytest
 
 Note that you must install the dependencies contained in
-[requirements-test.txt](requirements-test.txt) before running tests. See the explanation in
+[requirements-dev.txt](requirements-dev.txt) before running tests. See the explanation in
 [pyproject.toml](pyproject.toml) for details.
 
+Additionally, the tests currently require that you set `APPMAP=true`. You can
+either run `pytest` with `appmap-python` (see [tox.ini](tox.ini)), or you can explicitly
+set the environment variable.
+
 [pytest](https://docs.pytest.org/en/stable/) for testing:
 
 ```

_appmap/env.py

@@ -3,7 +3,6 @@
 import logging
 import logging.config
 import os
-import warnings
 from contextlib import contextmanager
 from os import environ
 from pathlib import Path
@@ -13,20 +12,6 @@
 
 from . import trace_logger
 
-_ENABLED_BY_DEFAULT_MSG = """
-
-The APPMAP environment variable is unset. Your code will be
-instrumented and recorded according to the configuration in appmap.yml.
-
-Starting with version 2, this behavior will change: when APPMAP is
-unset, no code will be instrumented. You will need to use the
-appmap-python script to run your application, or explicitly set
-APPMAP.
-
-Visit https://appmap.io/docs/reference/appmap-python.html#appmap-python-script for more
-details.
-"""
-
 _cwd = Path.cwd()
 _bootenv = environ.copy()
 
@@ -36,9 +21,9 @@ def _recording_method_key(recording_method):
 
 
 class Env(metaclass=SingletonMeta):
-    def __init__(self, env=None, cwd=None):
-        warnings.filterwarnings("once", _ENABLED_BY_DEFAULT_MSG)
+    RECORD_PROCESS_DEFAULT = "false"
 
+    def __init__(self, env=None, cwd=None):
         # root_dir and root_dir_len are going to be used when
         # instrumenting every function, so preprocess them as
         # much as possible.
@@ -54,7 +39,6 @@ def __init__(self, env=None, cwd=None):
 
         self._configure_logging()
         enabled = self._env.get("_APPMAP", None)
-        self._enabled_by_default = enabled is None
         self._enabled = enabled is None or enabled.lower() != "false"
 
         self._root_dir = str(self._cwd) + "/"
@@ -73,6 +57,9 @@ def __init__(self, env=None, cwd=None):
     def set(self, name, value):
         self._env[name] = value
 
+    def setdefault(self, name, default_value):
+        self._env.setdefault(name, default_value)
+
     def get(self, name, default=None):
         return self._env.get(name, default)
 
@@ -95,14 +82,6 @@ def output_dir(self):
     def output_dir(self, value):
         self._output_dir = value
 
-    @property
-    def enabled_by_default(self):
-        return self._enabled_by_default
-
-    def warn_enabled_by_default(self):
-        if self._enabled_by_default:
-            warnings.warn(_ENABLED_BY_DEFAULT_MSG, category=DeprecationWarning, stacklevel=2)
-
     @property
     def enabled(self):
         return self._enabled
@@ -115,14 +94,25 @@ def enables(self, recording_method, default="true"):
         if not self.enabled:
             return False
 
-        v = self.get(_recording_method_key(recording_method), default).lower()
-        return v != "false"
+        process_enabled = self._enables("process", self.RECORD_PROCESS_DEFAULT)
+        if recording_method == "process":
+            return process_enabled
+
+        # If process recording is enabled, others should be disabled
+        if process_enabled:
+            return False
+
+        # Otherwise, check the environment variable
+        return self._enables(recording_method, default)
+
+    def _enables(self, recording_method, default):
+        return self.get(_recording_method_key(recording_method), default).lower() != "false"
 
     @contextmanager
     def disabled(self, recording_method: str):
         key = _recording_method_key(recording_method)
         value = self.get(key)
-        self.set(key, "false")
+        self.setdefault(key, "false")
         try:
             yield
         finally:

_appmap/recording.py

@@ -44,7 +44,6 @@ def is_running(self):
         return Recorder.get_enabled()
 
     def __enter__(self):
-        Env.current.warn_enabled_by_default()
         self.start()
 
     def __exit__(self, exc_type, exc_value, tb):
@@ -80,9 +79,7 @@ def write_appmap(
 
 
 def initialize():
-    if Env.current.enables("process", "false"):
-        Env.current.warn_enabled_by_default()
-
+    if Env.current.enables("process", Env.RECORD_PROCESS_DEFAULT):
         r = Recording()
         r.start()
 

_appmap/test/test_configuration.py

@@ -16,10 +16,6 @@
 from _appmap.importer import Filterable, NullFilter
 
 
-def test_enabled_by_default():
-    assert appmap.enabled()
-
-
 @pytest.mark.appmap_enabled
 def test_can_be_configured():
     """
@@ -45,8 +41,6 @@ def test_reports_invalid():
 @pytest.mark.appmap_enabled(config="appmap-broken.yml")
 def test_is_disabled_when_unset():
     """Test that recording is disabled when APPMAP is unset but the config is broken"""
-    assert Env.current.get("_APPMAP", None) is None
-
     assert not appmap.enabled()
 
 

_appmap/test/test_django.py

@@ -216,6 +216,19 @@ def test_disabled(self, pytester, monkeypatch):
         result.assert_outcomes(passed=1, failed=0, errors=0)
         assert not (pytester.path / "tmp").exists()
 
+    def test_disabled_for_process(self, pytester, monkeypatch):
+        monkeypatch.setenv("APPMAP_RECORD_PROCESS", "true")
+
+        result = pytester.runpytest("-svv")
+
+        # There are two tests for remote recording. They should both fail,
+        # because process recording should disable remote recording.
+        result.assert_outcomes(passed=2, failed=2, errors=0)
+
+        assert (pytester.path / "tmp" / "appmap" / "process").exists()
+        assert not (pytester.path / "tmp" / "appmap" / "requests").exists()
+        assert not (pytester.path / "tmp" / "appmap" / "pytest").exists()
+
 
 @pytest.fixture(name="server")
 def django_server(xprocess, server_base):

_appmap/test/test_flask.py

@@ -8,10 +8,10 @@
 
 import flask
 import pytest
-from appmap.flask import AppmapFlask
 
 from _appmap.env import Env
 from _appmap.metadata import Metadata
+from appmap.flask import AppmapFlask
 
 from ..test.helpers import DictIncluding
 from .web_framework import (
@@ -144,7 +144,11 @@ def test_enabled(self, pytester):
         appmap_file = (
             pytester.path / "tmp" / "appmap" / "pytest" / "test_request.appmap.json"
         )
+
+        # No request recordings should have been created
         assert not os.path.exists(pytester.path / "tmp" / "appmap" / "requests")
+
+        # but there should be a test recording
         assert appmap_file.exists()
 
     def test_disabled(self, pytester, monkeypatch):
@@ -154,3 +158,14 @@ def test_disabled(self, pytester, monkeypatch):
 
         result.assert_outcomes(passed=1, failed=0, errors=0)
         assert not (pytester.path / "tmp" / "appmap").exists()
+
+    def test_disabled_for_process(self, pytester, monkeypatch):
+        monkeypatch.setenv("APPMAP_RECORD_PROCESS", "true")
+
+        result = pytester.runpytest("-svv")
+
+        result.assert_outcomes(passed=1, failed=0, errors=0)
+
+        assert (pytester.path / "tmp" / "appmap" / "process").exists()
+        assert not (pytester.path / "tmp" / "appmap" / "requests").exists()
+        assert not (pytester.path / "tmp" / "appmap" / "pytest").exists()

_appmap/test/test_runner.py

@@ -15,7 +15,7 @@ def test_runner_help(script_runner):
     assert result.stdout.startswith("usage")
 
 
-@pytest.mark.parametrize("recording_type", ["process", "pytest", "remote", "requests", "unittest"])
+@pytest.mark.parametrize("recording_type", ["process", "remote", "requests", "tests"])
 def test_runner_recording_type(script_runner, recording_type):
     result = script_runner.run(["appmap-python", "--record", recording_type])
     assert result.returncode == 0

_appmap/test/test_test_frameworks.py

@@ -39,11 +39,18 @@ def test_with_appmap_false(self, testdir, monkeypatch):
         assert not testdir.output().exists()
 
     def test_disabled(self, testdir, monkeypatch):
-        monkeypatch.setenv(f"APPMAP_RECORD_{self._test_type.upper()}", "false")
+        monkeypatch.setenv("APPMAP_RECORD_TESTS", "false")
 
         self.run_tests(testdir)
         assert not testdir.output().exists()
 
+    def test_disabled_for_process(self, testdir, monkeypatch):
+        monkeypatch.setenv("APPMAP_RECORD_PROCESS", "true")
+
+        self.run_tests(testdir)
+        assert (testdir.path / "tmp" / "appmap" / "process").exists()
+        assert not testdir.output().exists()
+
 
 class TestUnittestRunner(_TestTestRunner):
     @classmethod

_appmap/unittest.py

@@ -3,7 +3,6 @@
 from contextlib import contextmanager
 
 from _appmap import noappmap, testing_framework, wrapt
-from _appmap.env import Env
 from _appmap.utils import get_function_location
 
 _session = testing_framework.session("unittest", "tests")
@@ -43,7 +42,6 @@ def _args(test_case, *_, isTest=False, **__):
         with _session.record(
             test_case.__class__, method_name, location=location
         ) as metadata:
-            Env.current.warn_enabled_by_default()
             if metadata:
                 with wrapped(
                     *args, **kwargs
@@ -69,7 +67,6 @@ def callTestMethod(wrapped, test_case, args, kwargs):
         method_name = test_case.id().split(".")[-1]
         location = _get_test_location(test_case.__class__, method_name)
         with _session.record(test_case.__class__, method_name, location=location) as metadata:
-            Env.current.warn_enabled_by_default()
             if metadata:
                 with testing_framework.collect_result_metadata(metadata):
                     wrapped(*args, **kwargs)

_appmap/web_framework.py

@@ -250,8 +250,6 @@ def remote_enabled(self):
         """Return True if the AppMap middleware has enabled remote recording, False otherwise."""
 
     def run(self):
-        Env.current.warn_enabled_by_default()
-
         if not self.middleware_present():
             return self.insert_middleware()
 

appmap/__init__.py

@@ -9,40 +9,40 @@
 if _enabled is None or _enabled.upper() == "TRUE":
     if _enabled is not None:
         # Use setdefault so tests can manage _APPMAP as necessary
-        os.environ["_APPMAP"] = _enabled
-    from _appmap import generation  # noqa: F401
-    from _appmap.env import Env  # noqa: F401
-    from _appmap.importer import instrument_module  # noqa: F401
-    from _appmap.labels import labels  # noqa: F401
-    from _appmap.noappmap import decorator as noappmap  # noqa: F401
-    from _appmap.recording import Recording  # noqa: F401
-
-    try:
-        from . import django  # noqa: F401
-    except ImportError:
-        pass
-
-    try:
-        from . import flask  # noqa: F401
-    except ImportError:
-        pass
-
-    try:
-        from . import fastapi  # noqa: F401
-    except ImportError:
-        pass
-
-    try:
-        from . import uvicorn  # noqa: F401
-    except ImportError:
-        pass
-
-    # Note: pytest integration is configured as a pytest plugin, so it doesn't
-    # need to be imported here
-
-    # unittest is part of the standard library, so it should always be
-    # importable (and therefore doesn't need to be in a try .. except block)
-    from . import unittest  # noqa: F401
-
-    def enabled():
-        return Env.current.enabled
+        os.environ.setdefault("_APPMAP", _enabled)
+        from _appmap import generation  # noqa: F401
+        from _appmap.env import Env  # noqa: F401
+        from _appmap.importer import instrument_module  # noqa: F401
+        from _appmap.labels import labels  # noqa: F401
+        from _appmap.noappmap import decorator as noappmap  # noqa: F401
+        from _appmap.recording import Recording  # noqa: F401
+
+        try:
+            from . import django  # noqa: F401
+        except ImportError:
+            pass
+
+        try:
+            from . import flask  # noqa: F401
+        except ImportError:
+            pass
+
+        try:
+            from . import fastapi  # noqa: F401
+        except ImportError:
+            pass
+
+        try:
+            from . import uvicorn  # noqa: F401
+        except ImportError:
+            pass
+
+        # Note: pytest integration is configured as a pytest plugin, so it doesn't
+        # need to be imported here
+
+        # unittest is part of the standard library, so it should always be
+        # importable (and therefore doesn't need to be in a try .. except block)
+        from . import unittest  # noqa: F401
+
+        def enabled():
+            return Env.current.enabled

appmap/command/runner.py

@@ -24,10 +24,9 @@
 _RECORDING_TYPES = set(
     [
         "process",
-        "pytest",
         "remote",
         "requests",
-        "unittest",
+        "tests",
     ]
 )
 

appmap/pytest.py

@@ -22,13 +22,9 @@ def __call__(self, wrapped, _, args, kwargs):
                 return wrapped(*args, **kwargs)
 
 
-if not Env.current.is_appmap_repo and Env.current.enables("pytest"):
+if not Env.current.is_appmap_repo and Env.current.enables("tests"):
     logger.debug("Test recording is enabled (Pytest)")
 
-    @pytest.hookimpl
-    def pytest_configure(config):  # pylint: disable=unused-argument
-        Env.current.warn_enabled_by_default()
-
     @pytest.hookimpl
     def pytest_sessionstart(session):
         session.appmap = testing_framework.session(

appmap/unittest.py

@@ -2,7 +2,7 @@
 
 logger = Env.current.getLogger(__name__)
 
-if not Env.current.is_appmap_repo and Env.current.enables("unittest"):
+if not Env.current.is_appmap_repo and Env.current.enables("tests"):
     logger.debug("Test recording is enabled (unittest)")
     # pylint: disable=unused-import
     import _appmap.unittest  # pyright: ignore   # noqa: F401