feat: Check malformed path entries #303

zermelo-wisen · 2024-05-12T14:12:46Z

Fixes #302

Warning and dropping malformed configuration file package path entries, which cannot be parsed as Python module names: mod1/mod2, mod1\mod2, 123, ., .

apotterri

Thanks for this. Just a couple of minor changes to make, then it'll be good to go.

_appmap/configuration.py

apotterri · 2024-05-13T09:11:54Z

_appmap/configuration.py

+                has_separator = '/' in path or '\\' in path
+                raise AppMapPackagePathException(
+                    f"Malformed path value '{path}' in configuration file. "
+                    "Path entries should be module names"


Suggested change

"Path entries should be module names"

"Path entries must be module names"

apotterri · 2024-05-13T09:20:35Z

_appmap/configuration.py

+    def _check_path_values(self):
+        for item in self._config["packages"]:
+            path: str = item.get("path")
+            if path and not self._check_path_value(path):


This code should also raise an exception if path is None. This case is currently not handled well, and produces an ugly error message.

apotterri · 2024-05-14T10:28:46Z

I gave these changes a try. Because of the way appmap gets imported (using a .pth file), the UX is terrible:

[2024-05-14 06:25:39,785] ERROR _appmap.configuration: Malformed path value '.' in configuration file. Path entries must be module names.
Fatal Python error: init_import_site: Failed to import the site module
Python runtime state: initialized
Traceback (most recent call last):
  File "/Users/ajp/src/applandinc/worktrees/appmap-python/_appmap/configuration.py", line 227, in _load_config
    self._check_path_values()
  File "/Users/ajp/src/applandinc/worktrees/appmap-python/_appmap/configuration.py", line 294, in _check_path_values
    raise AppMapInvalidConfigException(
_appmap.configuration.AppMapInvalidConfigException: Malformed path value '.' in configuration file. Path entries must be module names.

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/Users/ajp/.asdf/installs/python/3.10.13/lib/python3.10/site.py", line 617, in <module>
    main()
  File "/Users/ajp/.asdf/installs/python/3.10.13/lib/python3.10/site.py", line 610, in main
    execsitecustomize()
  File "/Users/ajp/.asdf/installs/python/3.10.13/lib/python3.10/site.py", line 549, in execsitecustomize
    import sitecustomize
  File "/Users/ajp/src/applandinc/worktrees/appmap-python/_appmap/test/data/fastapi/init/sitecustomize.py", line 1, in <module>
    import appmap
  File "/Users/ajp/src/applandinc/worktrees/appmap-python/appmap/__init__.py", line 9, in <module>
    from _appmap import generation  # noqa: F401
  File "/Users/ajp/src/applandinc/worktrees/appmap-python/_appmap/__init__.py", line 1, in <module>
    from . import configuration
  File "/Users/ajp/src/applandinc/worktrees/appmap-python/_appmap/configuration.py", line 450, in <module>
    initialize()
  File "/Users/ajp/src/applandinc/worktrees/appmap-python/_appmap/configuration.py", line 445, in initialize
    Config().initialize()
  File "/Users/ajp/src/applandinc/worktrees/appmap-python/_appmap/configuration.py", line 146, in __init__
    self._load_config()
  File "/Users/ajp/src/applandinc/worktrees/appmap-python/_appmap/configuration.py", line 230, in _load_config
    sys.exit(1)
SystemExit: 1

So, we need to change this. When an invalid path is detected`, we should just log a warning and ignore it. This needs to happen without raising an exception, and the agent should not exit.

apotterri

#303 (comment)

zermelo-wisen · 2024-05-14T12:54:06Z

I gave these changes a try. Because of the way appmap gets imported (using a .pth file), the UX is terrible:

[2024-05-14 06:25:39,785] ERROR _appmap.configuration: Malformed path value '.' in configuration file. Path entries must be module names.
Fatal Python error: init_import_site: Failed to import the site module
Python runtime state: initialized
Traceback (most recent call last):
  File "/Users/ajp/src/applandinc/worktrees/appmap-python/_appmap/configuration.py", line 227, in _load_config
    self._check_path_values()
  File "/Users/ajp/src/applandinc/worktrees/appmap-python/_appmap/configuration.py", line 294, in _check_path_values
    raise AppMapInvalidConfigException(
_appmap.configuration.AppMapInvalidConfigException: Malformed path value '.' in configuration file. Path entries must be module names.

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/Users/ajp/.asdf/installs/python/3.10.13/lib/python3.10/site.py", line 617, in <module>
    main()
  File "/Users/ajp/.asdf/installs/python/3.10.13/lib/python3.10/site.py", line 610, in main
    execsitecustomize()
  File "/Users/ajp/.asdf/installs/python/3.10.13/lib/python3.10/site.py", line 549, in execsitecustomize
    import sitecustomize
  File "/Users/ajp/src/applandinc/worktrees/appmap-python/_appmap/test/data/fastapi/init/sitecustomize.py", line 1, in <module>
    import appmap
  File "/Users/ajp/src/applandinc/worktrees/appmap-python/appmap/__init__.py", line 9, in <module>
    from _appmap import generation  # noqa: F401
  File "/Users/ajp/src/applandinc/worktrees/appmap-python/_appmap/__init__.py", line 1, in <module>
    from . import configuration
  File "/Users/ajp/src/applandinc/worktrees/appmap-python/_appmap/configuration.py", line 450, in <module>
    initialize()
  File "/Users/ajp/src/applandinc/worktrees/appmap-python/_appmap/configuration.py", line 445, in initialize
    Config().initialize()
  File "/Users/ajp/src/applandinc/worktrees/appmap-python/_appmap/configuration.py", line 146, in __init__
    self._load_config()
  File "/Users/ajp/src/applandinc/worktrees/appmap-python/_appmap/configuration.py", line 230, in _load_config
    sys.exit(1)
SystemExit: 1

So, we need to change this. When an invalid path is detected`, we should just log a warning and ignore it. This needs to happen without raising an exception, and the agent should not exit.

Done

apotterri · 2024-05-15T13:37:56Z

_appmap/configuration.py

+                invalid_items.append(item)
+                continue
+
+        if len(invalid_items) > 0:


What's going to happen if this eliminates all the path entries?

I think, no package will be instrumented, unless there are some dist entries. We reach this state with at least one warning.

Thinking again, while loading the config file should we also check if:

packages is a yaml sequence and not a scalar,

packages is empty,

every item has path XOR dist key?

No, let's not expand the scope of this beyond checking that paths are modules.

However, please add a test that has only invalid paths, and confirm that the resulting Config has a package with an empty list of paths.

Update Config so it matches the singleton style of Env.

zermelo-wisen requested a review from apotterri May 12, 2024 14:13

zermelo-wisen force-pushed the feat/check-malformed-path-entries branch 2 times, most recently from eb3f71f to e2ab421 Compare May 13, 2024 06:56

apotterri requested changes May 13, 2024

View reviewed changes

zermelo-wisen force-pushed the feat/check-malformed-path-entries branch 2 times, most recently from 3f22958 to 6db2a85 Compare May 13, 2024 14:14

zermelo-wisen requested review from kgilpin and apotterri May 13, 2024 15:01

apotterri requested changes May 14, 2024

View reviewed changes

zermelo-wisen force-pushed the feat/check-malformed-path-entries branch from 6db2a85 to 8537fff Compare May 14, 2024 12:46

zermelo-wisen closed this May 14, 2024

zermelo-wisen reopened this May 14, 2024

zermelo-wisen requested a review from apotterri May 14, 2024 13:06

apotterri reviewed May 15, 2024

View reviewed changes

zermelo-wisen force-pushed the feat/check-malformed-path-entries branch from 8537fff to 6cbf90e Compare May 16, 2024 05:47

zermelo-wisen requested a review from apotterri May 16, 2024 06:47

refactor: clean up Config

208ab4b

Update Config so it matches the singleton style of Env.

apotterri force-pushed the feat/check-malformed-path-entries branch from 6cbf90e to 8f89354 Compare May 16, 2024 10:06

feat: check malformed path entries

f7937ee

apotterri force-pushed the feat/check-malformed-path-entries branch from 8f89354 to f7937ee Compare May 16, 2024 10:24

apotterri approved these changes May 16, 2024

View reviewed changes

apotterri merged commit 9627d70 into master May 16, 2024
1 check passed

apotterri deleted the feat/check-malformed-path-entries branch May 16, 2024 11:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Check malformed path entries #303

feat: Check malformed path entries #303

zermelo-wisen commented May 12, 2024 •

edited

Loading

apotterri left a comment

apotterri May 13, 2024

apotterri May 13, 2024

apotterri commented May 14, 2024

apotterri left a comment

zermelo-wisen commented May 14, 2024

apotterri May 15, 2024

zermelo-wisen May 15, 2024 •

edited

Loading

apotterri May 15, 2024

zermelo-wisen May 16, 2024

	"Path entries should be module names"
	"Path entries must be module names"

feat: Check malformed path entries #303

feat: Check malformed path entries #303

Conversation

zermelo-wisen commented May 12, 2024 • edited Loading

apotterri left a comment

Choose a reason for hiding this comment

apotterri May 13, 2024

Choose a reason for hiding this comment

apotterri May 13, 2024

Choose a reason for hiding this comment

apotterri commented May 14, 2024

apotterri left a comment

Choose a reason for hiding this comment

zermelo-wisen commented May 14, 2024

apotterri May 15, 2024

Choose a reason for hiding this comment

zermelo-wisen May 15, 2024 • edited Loading

Choose a reason for hiding this comment

apotterri May 15, 2024

Choose a reason for hiding this comment

zermelo-wisen May 16, 2024

Choose a reason for hiding this comment

zermelo-wisen commented May 12, 2024 •

edited

Loading

zermelo-wisen May 15, 2024 •

edited

Loading