Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable process recording by default #366

Open
kgilpin opened this issue Aug 5, 2024 · 1 comment
Open

Enable process recording by default #366

kgilpin opened this issue Aug 5, 2024 · 1 comment
Assignees
Labels
enhancement New feature or request navie-plan Plan the issue with Navie

Comments

@kgilpin
Copy link
Contributor

kgilpin commented Aug 5, 2024

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

  • Process recording is enabled by default
  • If a request, remote, or test recording begins, the process recording is aborted
  • Process recording can be further controlled with env var APPMAP_RECORDER_PROCESS_ALWAYS
    • When truthy, process recording remains enabled even if another recording method would otherwise abort it.
    • When falsey, process recording does not begin on startup.

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

@kgilpin kgilpin added enhancement New feature or request navie-plan Plan the issue with Navie labels Aug 5, 2024
Copy link

github-actions bot commented Aug 8, 2024

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.

@apotterri apotterri self-assigned this Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request navie-plan Plan the issue with Navie
Projects
None yet
Development

No branches or pull requests

2 participants