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 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