From 1d81723ee8e35e430103620ab8994c932d2298ab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Rzepecki?= Date: Sat, 4 Apr 2026 18:06:47 +0200 Subject: [PATCH 1/2] chore: move cyclic-imports pylint exclusion from tox.ini to pylintrc --- pylintrc | 1 + tox.ini | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/pylintrc b/pylintrc index e281a970..853e0308 100644 --- a/pylintrc +++ b/pylintrc @@ -418,6 +418,7 @@ confidence=HIGH, # Disable unidiomatic-typecheck. Using isinstance() invokes the descriptor protocol, which can have # side effects. Using type() avoids this. disable=unidiomatic-typecheck, + cyclic-import, raw-checker-failed, bad-inline-option, locally-disabled, diff --git a/tox.ini b/tox.ini index 2b8ed82f..852b3390 100644 --- a/tox.ini +++ b/tox.ini @@ -62,7 +62,7 @@ commands = # It doesn't seem great to disable cyclic-import checking, but the imports # aren't currently causing any problems. They should probably get fixed # sometime soon. - {posargs:pylint --disable=cyclic-import -j 0 appmap _appmap} + {posargs:pylint -j 0 appmap _appmap} [testenv:vendoring] skip_install = True From b324b48f075d0286015a7cd5939d5947703527cc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Rzepecki?= Date: Sat, 4 Apr 2026 18:20:50 +0200 Subject: [PATCH 2/2] feat: Capture argument values of labeled functions by default Introduces a new mode to APPMAP_DISPLAY_PARAMS, which now can be: - 'true': capture argument values for all functions, - 'false': disable all argument value capture, - 'labeled' (new, default): capture only for labeled functions. The labeled default provides useful argument visibility for semantically important functions (such as HTTP, crypto, etc.) without the performance cost of global capture. The capture policy is evaluated once per function at instrumentation time and cached on the `_InstrumentedFn` tuple, eliminating per-call environment lookups from the hot path. Co-Authored-By: kgilpin --- _appmap/env.py | 16 +++++++-- _appmap/event.py | 42 ++++++++++++---------- _appmap/instrument.py | 19 +++++++--- _appmap/test/data/example_class.py | 4 +++ _appmap/test/test_describe_value.py | 24 ++++++++++--- _appmap/test/test_events.py | 56 +++++++++++++++++++++++++++++ _appmap/test/test_params.py | 4 ++- _appmap/web_framework.py | 2 +- appmap/__init__.py | 4 +-- 9 files changed, 138 insertions(+), 33 deletions(-) diff --git a/_appmap/env.py b/_appmap/env.py index aa61b233..be23a61d 100644 --- a/_appmap/env.py +++ b/_appmap/env.py @@ -45,8 +45,16 @@ def __init__(self, env=None, cwd=None): # them. enabled = self._env.get("_APPMAP", None) self._enabled = enabled is not None and enabled.lower() != "false" - display_params = self._env.get("_APPMAP_DISPLAY_PARAMS", None) - self._display_params = display_params is not None and display_params.lower() != "false" + display_params = self._env.get("_APPMAP_DISPLAY_PARAMS", "labeled").lower() + if display_params == "true": + self._display_params = True + self._display_labeled_params = True + elif display_params == "false": + self._display_params = False + self._display_labeled_params = False + else: # "labeled" or "auto" or anything else defaults to labeled + self._display_params = False + self._display_labeled_params = True logger = logging.getLogger(__name__) # The user shouldn't set APPMAP_OUTPUT_DIR, but some tests depend on being able to use it. @@ -134,6 +142,10 @@ def is_appmap_repo(self): def display_params(self): return self._display_params + @property + def display_labeled_params(self): + return self._display_labeled_params + def getLogger(self, name) -> trace_logger.TraceLogger: return cast(trace_logger.TraceLogger, logging.getLogger(name)) diff --git a/_appmap/event.py b/_appmap/event.py index 0094412e..4bdcf3af 100644 --- a/_appmap/event.py +++ b/_appmap/event.py @@ -45,13 +45,13 @@ def reset(cls): cls._next_thread_id = 0 -def display_string(val): +def display_string(val, display_value=False): # If we're asked to display parameters, make a best-effort attempt # to get a string value for the parameter using repr(). If parameter # display is disabled, or repr() has raised, just formulate a value # from the class and id. value = None - if Env.current.display_params: + if display_value: try: value = repr(val) except Exception: # pylint: disable=broad-except @@ -79,7 +79,7 @@ def _is_list_or_dict(val_type): return issubclass(val_type, list), issubclass(val_type, dict) -def _describe_schema(name, val, depth, max_depth): +def _describe_schema(name, val, depth, max_depth, display_value=False): val_type = type(val) @@ -88,6 +88,9 @@ def _describe_schema(name, val, depth, max_depth): ret["name"] = name ret["class"] = fqname(val_type) + if not display_value: + return ret + islist, isdict = _is_list_or_dict(val_type) if not (islist or isdict) or (depth >= max_depth and isdict): return ret @@ -100,7 +103,10 @@ def _describe_schema(name, val, depth, max_depth): elts = val.items() schema_key = "properties" - schema = [_describe_schema(k, v, depth + 1, max_depth) for k, v in elts] + schema = [ + _describe_schema(k, v, depth + 1, max_depth, display_value=display_value) + for k, v in elts + ] # schema will be [None] if depth is exceeded, don't use it if any(schema): ret[schema_key] = schema @@ -108,15 +114,14 @@ def _describe_schema(name, val, depth, max_depth): return ret -def describe_value(name, val, max_depth=5): - ret = { +def describe_value(name, val, max_depth=5, display_value=False): + ret = _describe_schema(name, val, 0, max_depth, display_value=display_value) + ret.update({ "object_id": id(val), - "value": display_string(val), - } - if Env.current.display_params: - ret.update(_describe_schema(name, val, 0, max_depth)) + "value": display_string(val, display_value=display_value), + }) - if any(_is_list_or_dict(type(val))): + if display_value and any(_is_list_or_dict(type(val))): ret["size"] = len(val) return ret @@ -170,9 +175,9 @@ def __init__(self, sigp): def __repr__(self): return "" % (self.name, self.kind) - def to_dict(self, value): + def to_dict(self, value, display_value=False): ret = {"kind": self.kind} - ret.update(describe_value(self.name, value)) + ret.update(describe_value(self.name, value, display_value=display_value)) return ret def _get_name_parts(filterable): @@ -247,7 +252,7 @@ def make_params(filterable): return [Param(p) for p in sig.parameters.values()] @staticmethod - def set_params(params, instance, args, kwargs): + def set_params(params, instance, args, kwargs, display_value=False): # pylint: disable=too-many-branches # Note that set_params expects args and kwargs as a tuple and # dict, respectively. It operates on them as collections, so @@ -295,7 +300,7 @@ def set_params(params, instance, args, kwargs): # If all the parameter types are handled, this # shouldn't ever happen... raise RuntimeError("Unknown parameter with desc %s" % (repr(p))) - ret.append(p.to_dict(value)) + ret.append(p.to_dict(value, display_value=display_value)) return ret @property @@ -405,8 +410,9 @@ def message_parameters(self): @message_parameters.setter def message_parameters(self, params): + display_params = Env.current.display_params for name, value in params.items(): - message_object = describe_value(name, value) + message_object = describe_value(name, value, display_value=display_params) self.message.append(message_object) @@ -497,13 +503,13 @@ def __init__(self, parent_id, elapsed): class FuncReturnEvent(ReturnEvent): __slots__ = ["return_value"] - def __init__(self, parent_id, elapsed, return_value): + def __init__(self, parent_id, elapsed, return_value, display_value=False): super().__init__(parent_id, elapsed) # Import here to prevent circular dependency # pylint: disable=import-outside-toplevel from _appmap.instrument import recording_disabled # noqa: F401 with recording_disabled(): - self.return_value = describe_value(None, return_value) + self.return_value = describe_value(None, return_value, display_value=display_value) class HttpResponseEvent(ReturnEvent): diff --git a/_appmap/instrument.py b/_appmap/instrument.py index 179e425c..784eca4c 100644 --- a/_appmap/instrument.py +++ b/_appmap/instrument.py @@ -70,7 +70,7 @@ def saved_shallow_rule(): _InstrumentedFn = namedtuple( - "_InstrumentedFn", "fn fntype instrumented_fn make_call_event params" + "_InstrumentedFn", "fn fntype instrumented_fn make_call_event params display_params" ) @@ -84,7 +84,9 @@ def call_instrumented(f, instance, args, kwargs): with recording_disabled(): logger.trace("%s args %s kwargs %s", f.fn, args, kwargs) - params = CallEvent.set_params(f.params, instance, args, kwargs) + params = CallEvent.set_params( + f.params, instance, args, kwargs, display_value=f.display_params + ) call_event = f.make_call_event(parameters=params) Recorder.add_event(call_event) call_event_id = call_event.id @@ -95,7 +97,8 @@ def call_instrumented(f, instance, args, kwargs): elapsed_time = time.time() - start_time return_event = event.FuncReturnEvent( - return_value=ret, parent_id=call_event_id, elapsed=elapsed_time + return_value=ret, parent_id=call_event_id, elapsed=elapsed_time, + display_value=f.display_params ) Recorder.add_event(return_event) return ret @@ -120,9 +123,16 @@ def instrument(filterable): """return an instrumented function""" logger.debug("hooking %s", filterable.fqname) + # note this has to happen before CallEvent.make, which clears this attribute + has_labels = hasattr(filterable.obj, "_appmap_labels") + make_call_event = event.CallEvent.make(filterable) params = CallEvent.make_params(filterable) + display_params = Env.current.display_params or ( + has_labels and Env.current.display_labeled_params + ) + # django depends on being able to find the cache_clear attribute # on functions. (You can see this by trying to map # https://github.com/chicagopython/chypi.org.) Make sure it gets @@ -132,7 +142,8 @@ def instrument(filterable): def instrumented_fn(wrapped, instance, args, kwargs): with saved_shallow_rule(): f = _InstrumentedFn( - wrapped, filterable.fntype, instrumented_fn, make_call_event, params + wrapped, filterable.fntype, instrumented_fn, make_call_event, params, + display_params ) return call_instrumented(f, instance, args, kwargs) diff --git a/_appmap/test/data/example_class.py b/_appmap/test/data/example_class.py index 4d7c2c93..e446efaf 100644 --- a/_appmap/test/data/example_class.py +++ b/_appmap/test/data/example_class.py @@ -56,6 +56,10 @@ def test_exception(self): def labeled_method(self): return "super important" + @appmap.labels("super", "important") + def labeled_method_with_param(self, p): + return p + @staticmethod @wrap_fn def wrapped_static_method(): diff --git a/_appmap/test/test_describe_value.py b/_appmap/test/test_describe_value.py index fe5706d3..24f90357 100644 --- a/_appmap/test/test_describe_value.py +++ b/_appmap/test/test_describe_value.py @@ -24,7 +24,7 @@ def value(self): return {"id": 1, "contents": "some text"} def test_one_level_schema(self, value): - actual = describe_value(None, value) + actual = describe_value(None, value, display_value=True) assert actual == DictIncluding( { "properties": [ @@ -34,6 +34,13 @@ def test_one_level_schema(self, value): } ) + def test_one_level_schema_display_false(self, value): + actual = describe_value(None, value, display_value=False) + assert "properties" not in actual + assert actual["class"] == "builtins.dict" + assert "builtins.dict object at" in actual["value"] + assert actual["object_id"] == id(value) + class TestNestedDictValue: @pytest.fixture @@ -41,7 +48,7 @@ def value(self): return {"page": {"page_number": 1, "page_size": 20, "total": 2383}} def test_two_level_schema(self, value): - actual = describe_value(None, value) + actual = describe_value(None, value, display_value=True) assert actual == DictIncluding( { "properties": [ @@ -60,7 +67,7 @@ def test_two_level_schema(self, value): def test_respects_max_depth(self, value): expected = {"properties": [{"name": "page", "class": "builtins.dict"}]} - actual = describe_value(None, value, max_depth=1) + actual = describe_value(None, value, max_depth=1, display_value=True) assert actual == DictIncluding(expected) @@ -70,7 +77,7 @@ def value(self): return [{"id": 1, "contents": "some text"}, {"id": 2}] def test_an_array_containing_schema(self, value): - actual = describe_value(None, value) + actual = describe_value(None, value, display_value=True) assert actual["class"] == "builtins.list" assert actual["items"][0] == DictIncluding( { @@ -88,6 +95,13 @@ def test_an_array_containing_schema(self, value): } ) + def test_an_array_display_false(self, value): + actual = describe_value(None, value, display_value=False) + assert "items" not in actual + assert actual["class"] == "builtins.list" + assert "builtins.list object at" in actual["value"] + assert actual["object_id"] == id(value) + class TestNestedArrays: @pytest.fixture @@ -95,7 +109,7 @@ def value(self): return [[["one"]]] def test_arrays_ignore_max_depth(self, value): - actual = describe_value(None, value, max_depth=1) + actual = describe_value(None, value, max_depth=1, display_value=True) expected = { "class": "builtins.list", "items": [ diff --git a/_appmap/test/test_events.py b/_appmap/test/test_events.py index bef98577..cbab31e8 100644 --- a/_appmap/test/test_events.py +++ b/_appmap/test/test_events.py @@ -52,6 +52,7 @@ def test_recursion_protection(self): # is working assert True + @pytest.mark.appmap_enabled(env={"APPMAP_DISPLAY_PARAMS": "true"}) def test_when_str_raises(self, mocker): r = appmap.Recording() with r: @@ -68,6 +69,7 @@ def test_when_str_raises(self, mocker): actual_value = r.events[0].parameters[0]["value"] assert expected_value == actual_value + @pytest.mark.appmap_enabled(env={"APPMAP_DISPLAY_PARAMS": "true"}) def test_when_both_raise(self, mocker): r = appmap.Recording() with r: @@ -117,6 +119,60 @@ def test_describe_return_value_recursion_protection(self): "return_self" ] + @pytest.mark.appmap_enabled(env={"APPMAP_DISPLAY_PARAMS": None}) + def test_labeled_params_displayed_by_default(self): + """When display_params is 'labeled' (default), + labeled functions should still have their params displayed via repr().""" + r = appmap.Recording() + with r: + from example_class import ExampleClass # pylint: disable=import-outside-toplevel + + result = ExampleClass().labeled_method_with_param("hello") + ExampleClass().instance_with_param("hello") + + assert result == "hello" + call_event = r.events[0] + # Parameter value should be the repr, not the opaque object string + assert call_event.parameters[0]["value"] == "'hello'" + # Return value should also be displayed + return_event = r.events[1] + assert return_event.return_value["value"] == "'hello'" + + # Unlabeled method should not have its params displayed, even in the same recording + call_event_unlabeled = r.events[2] + assert "object at" in call_event_unlabeled.parameters[0]["value"] + + @pytest.mark.appmap_enabled( + env={ + "APPMAP_DISPLAY_PARAMS": "false", + } + ) + def test_labeled_params_not_displayed_when_disabled(self): + """When display_params is off, labeled functions should NOT have their params displayed.""" + r = appmap.Recording() + with r: + from example_class import ExampleClass # pylint: disable=import-outside-toplevel + + ExampleClass().labeled_method_with_param("hello") + + call_event = r.events[0] + # Parameter value should be the opaque object string + assert "object at" in call_event.parameters[0]["value"] + + @pytest.mark.appmap_enabled(env={"APPMAP_DISPLAY_PARAMS": "labeled"}) + def test_unlabeled_params_not_displayed(self): + """When display_params is 'labeled', unlabeled functions should NOT + have their params displayed.""" + r = appmap.Recording() + with r: + from example_class import ExampleClass # pylint: disable=import-outside-toplevel + + ExampleClass().instance_with_param("hello") + + call_event = r.events[0] + # Parameter value should be the opaque object string + assert "object at" in call_event.parameters[0]["value"] + # There should be an exception return event generated even when the raised exception is a # BaseException. def test_exception_event_with_base_exception(self): diff --git a/_appmap/test/test_params.py b/_appmap/test/test_params.py index 26669861..7f6d3436 100644 --- a/_appmap/test/test_params.py +++ b/_appmap/test/test_params.py @@ -33,7 +33,9 @@ def prepare(cls, ffn): def wrapped_fn(_, instance, args, kwargs): return make_call_event( - parameters=CallEvent.set_params(params, instance, args, kwargs) + parameters=CallEvent.set_params( + params, instance, args, kwargs, display_value=True + ) ) return wrapped_fn diff --git a/_appmap/web_framework.py b/_appmap/web_framework.py index c6edb39b..086154b6 100644 --- a/_appmap/web_framework.py +++ b/_appmap/web_framework.py @@ -44,7 +44,7 @@ class TemplateEvent(Event): # pylint: disable=too-few-public-methods def __init__(self, path, instance=None): super().__init__("call") - self.receiver = describe_value(None, instance) + self.receiver = describe_value(None, instance, display_value=Env.current.display_params) self.path = root_relative_path(path) def to_dict(self, attrs=None): diff --git a/appmap/__init__.py b/appmap/__init__.py index a60e1af4..60a1ea7d 100644 --- a/appmap/__init__.py +++ b/appmap/__init__.py @@ -13,7 +13,7 @@ if _enabled is not None: # Use setdefault so tests can manage settings as necessary os.environ.setdefault("_APPMAP", _enabled) - _display_params = os.environ.get("APPMAP_DISPLAY_PARAMS", "false") + _display_params = os.environ.get("APPMAP_DISPLAY_PARAMS", "labeled") os.environ.setdefault("_APPMAP_DISPLAY_PARAMS", _display_params) from _appmap import generation # noqa: F401 @@ -58,7 +58,7 @@ def enabled(): os.environ.pop("_APPMAP_DISPLAY_PARAMS", None) else: os.environ.setdefault("_APPMAP", "false") - os.environ.setdefault("_APPMAP_DISPLAY_PARAMS", "false") + os.environ.setdefault("_APPMAP_DISPLAY_PARAMS", "labeled") if not _recording_exported: # Client code that imports appmap.Recording should run correctly