getappmap / appmap-python

AppMap client agent for Python
https://appland.org
Other
100 stars 16 forks source link

Enable process recording by default #366

Open kgilpin opened 3 months ago

kgilpin commented 3 months ago

Implement a pattern that's working well in the node agent:

See also: https://github.com/getappmap/appmap-node/blob/a8b9fbf6c5bbf384fb5f0556b49c0525379ea837/src/recorder.ts#L16

github-actions[bot] commented 3 months ago

Title:

Enable Process Recording by Default with Conditional Abortion and Control via Environment Variable

Problem:

The current implementation of the AppMap Python agent does not enable process recording by default. The requirement is to enable process recording on startup by default, allow its abortion when request, remote, or test recording begins, and introduce additional configurability by using the environment variable APPMAP_RECORDER_PROCESS_ALWAYS.

Analysis:

To achieve this functionality, multiple aspects need to be considered:

  1. Enable Process Recording by Default: Modify the initialization logic to ensure that process recording is enabled unless otherwise specified by APPMAP_RECORD_PROCESS.
  2. Conditional Abortion of Process Recording: Implement logic that aborts the process recording if any other type of recording (request, remote, or test) begins.
  3. Environment Variable Control: Utilize the APPMAP_RECORDER_PROCESS_ALWAYS environment variable to provide finer control:
    • When set to truthy, process recording should continue even if another recording type would typically abort it.
    • When set to falsey, process recording should not start automatically on startup.

Proposed Changes:

  1. Enable Process Recording in Initialization:

    • Modify the constructor in Env class to check the default value and environment variable settings to enable process recording by default.
    class Env(metaclass=SingletonMeta):
       RECORD_PROCESS_DEFAULT = "true"   # Change default to true
    
       def __init__(self, env=None, cwd=None):
           # existing code...
           if env:
               for k, v in env.items():
                   if v is not None:
                       self._env[k] = v
                   else:
                       self._env.pop(k, None)
    
           self.log_file_creation_failed = False
           self._configure_logging()
    
           enabled = self._env.get("_APPMAP", "false")
           self._enabled = enabled is None or enabled.lower() != "false"
    
           recording_always = self._env.get("APPMAP_RECORDER_PROCESS_ALWAYS", "false")
           self._process_recording_always = recording_always.lower() == "true"
    
           self._root_dir = str(self._cwd) + "/"
           self._root_dir_len = len(self._root_dir)
  2. Implement Conditional Abortion:

    • Modify the enables method to switch off process recording if any other recording (request, remote, or test) starts, unless overridden by APPMAP_RECORDER_PROCESS_ALWAYS.
    def enables(self, recording_method, default="true"):
       if not self.enabled:
           return False
    
       process_enabled = self._process_recording_always or self._enables("process", self.RECORD_PROCESS_DEFAULT)
       if recording_method == "process":
           return process_enabled
    
       # If process recording is enabled, others should be disabled, unless overridden
       if process_enabled and not self._process_recording_always:
           return False
    
       # Otherwise, check the environment variable
       return self._enables(recording_method, default)
  3. Update Test Cases:

    • Update the relevant test cases to reflect the new functionality.
    def test_disabled_for_process(self, testdir, monkeypatch):
       monkeypatch.setenv("APPMAP_RECORD_PROCESS", "true") # Ensure process recording is warranted here
       # Run tests to check process recording
    • Add test cases for APPMAP_RECORDER_PROCESS_ALWAYS behavior.
    class TestEnv:
       def test_process_recording_always_true(self, monkeypatch):
           monkeypatch.setenv("APPMAP_RECORDER_PROCESS_ALWAYS", "true")
           env = Env()
           assert env._process_recording_always
    
       def test_process_recording_always_false(self, monkeypatch):
           monkeypatch.setenv("APPMAP_RECORDER_PROCESS_ALWAYS", "false")
           env = Env()
           assert not env._process_recording_always

These changes collectively ensure that process recording is enabled by default while allowing it to be controlled and conditioned by the presence of other types of recordings and an additional environment variable.