From f481cc252631b2dc0f15afb342e854d5dfefd1af Mon Sep 17 00:00:00 2001 From: Emmett Butler <723615+emmettbutler@users.noreply.github.com> Date: Fri, 16 Aug 2024 08:35:03 -0700 Subject: [PATCH] chore: remove some crash-by-design behaviors from internal/module.py (#10234) This change improves the auto-injection user experience by removing some crash-by-design behaviors from `internal/module.py`. They're replaced either with idempotency checks or warn+return logic. ## Checklist - [x] PR author has checked that all the criteria below are met - The PR description includes an overview of the change - The PR description articulates the motivation for the change - The change includes tests OR the PR description describes a testing strategy - The PR description notes risks associated with the change, if any - Newly-added code is easy to change - The change follows the [library release note guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html) - The change includes or references documentation updates if necessary - Backport labels are set (if [applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)) ## Reviewer Checklist - [x] Reviewer has checked that all the criteria below are met - Title is accurate - All changes are related to the pull request's stated goal - Avoids breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes - Testing strategy adequately addresses listed risks - Newly-added code is easy to change - Release note makes sense to a user of the library - If necessary, author has acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment - Backport labels are set in a manner that is consistent with the [release branch maintenance policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting) --- ddtrace/internal/module.py | 28 +++++++++++++++++----------- tests/internal/test_module.py | 24 ++++++------------------ 2 files changed, 23 insertions(+), 29 deletions(-) diff --git a/ddtrace/internal/module.py b/ddtrace/internal/module.py index fa8ab3505f8..4b6bc2f6e26 100644 --- a/ddtrace/internal/module.py +++ b/ddtrace/internal/module.py @@ -344,7 +344,8 @@ def _remove_from_meta_path(cls) -> None: i = cls._find_in_meta_path() if i is None: - raise RuntimeError("%s is not installed" % cls.__name__) + log.warning("%s is not installed", cls.__name__) + return sys.meta_path.pop(i) @@ -412,15 +413,14 @@ def find_spec( @classmethod def _check_installed(cls) -> None: - if not cls.is_installed(): - raise RuntimeError("%s is not installed" % cls.__name__) + if cls.is_installed(): + return @classmethod def install(cls) -> None: """Install the module watchdog.""" if cls.is_installed(): - raise RuntimeError("%s is already installed" % cls.__name__) - + return cls._instance = cls() cls._instance._add_to_meta_path() log.debug("%s installed", cls) @@ -555,7 +555,8 @@ def register_origin_hook(cls, origin: Path, hook: ModuleHookType) -> None: # change, then thread-safety might become a concern. resolved_path = _resolve(origin) if resolved_path is None: - raise ValueError("Cannot resolve module origin %s" % origin) + log.warning("Cannot resolve module origin %s", origin) + return path = str(resolved_path) @@ -588,13 +589,15 @@ def unregister_origin_hook(cls, origin: Path, hook: ModuleHookType) -> None: resolved_path = _resolve(origin) if resolved_path is None: - raise ValueError("Module origin %s cannot be resolved", origin) + log.warning("Module origin %s cannot be resolved", origin) + return path = str(resolved_path) instance = t.cast(ModuleWatchdog, cls._instance) if path not in instance._hook_map: - raise ValueError("No hooks registered for origin %s" % origin) + log.warning("No hooks registered for origin %s", origin) + return try: if path in instance._hook_map: @@ -603,7 +606,8 @@ def unregister_origin_hook(cls, origin: Path, hook: ModuleHookType) -> None: if not hooks: del instance._hook_map[path] except ValueError: - raise ValueError("Hook %r not registered for origin %s" % (hook, origin)) + log.warning("Hook %r not registered for origin %s", hook, origin) + return @classmethod def register_module_hook(cls, module: str, hook: ModuleHookType) -> None: @@ -636,7 +640,8 @@ def unregister_module_hook(cls, module: str, hook: ModuleHookType) -> None: instance = t.cast(ModuleWatchdog, cls._instance) if module not in instance._hook_map: - raise ValueError("No hooks registered for module %s" % module) + log.warning("No hooks registered for module %s", module) + return try: if module in instance._hook_map: @@ -645,7 +650,8 @@ def unregister_module_hook(cls, module: str, hook: ModuleHookType) -> None: if not hooks: del instance._hook_map[module] except ValueError: - raise ValueError("Hook %r not registered for module %r" % (hook, module)) + log.warning("Hook %r not registered for module %r", hook, module) + return @classmethod def after_module_imported(cls, module: str) -> t.Callable[[ModuleHookType], None]: diff --git a/tests/internal/test_module.py b/tests/internal/test_module.py index bf3b5c2020a..7a3fd8de48b 100644 --- a/tests/internal/test_module.py +++ b/tests/internal/test_module.py @@ -97,14 +97,6 @@ def test_after_module_imported_decorator(module_watchdog): hook.assert_called_once_with(module) -def test_register_hook_without_install(): - with pytest.raises(RuntimeError): - ModuleWatchdog.register_origin_hook(__file__, mock.Mock()) - - with pytest.raises(RuntimeError): - ModuleWatchdog.register_module_hook(__name__, mock.Mock()) - - @pytest.mark.subprocess(env=dict(MODULE_ORIGIN=str(origin(tests.test_module)))) def test_import_origin_hook_for_module_not_yet_imported(): import os @@ -238,8 +230,7 @@ def test_module_unregister_origin_hook(module_watchdog): assert module_watchdog._instance._hook_map[str(path)] == [] - with pytest.raises(ValueError): - module_watchdog.unregister_origin_hook(path, hook) + module_watchdog.unregister_origin_hook(path, hook) def test_module_unregister_module_hook(module_watchdog): @@ -257,21 +248,18 @@ def test_module_unregister_module_hook(module_watchdog): module_watchdog.unregister_module_hook(module, hook) assert module_watchdog._instance._hook_map[module] == [] - with pytest.raises(ValueError): - module_watchdog.unregister_module_hook(module, hook) + module_watchdog.unregister_module_hook(module, hook) def test_module_watchdog_multiple_install(): ModuleWatchdog.install() - with pytest.raises(RuntimeError): - ModuleWatchdog.install() - + assert ModuleWatchdog.is_installed() + ModuleWatchdog.install() assert ModuleWatchdog.is_installed() ModuleWatchdog.uninstall() - with pytest.raises(RuntimeError): - ModuleWatchdog.uninstall() - + assert not ModuleWatchdog.is_installed() + ModuleWatchdog.uninstall() assert not ModuleWatchdog.is_installed()