From 8e9edd3c18a7dcd96a6b0482981c12fca902a13c Mon Sep 17 00:00:00 2001 From: "Gabriele N. Tornetta" Date: Fri, 15 Nov 2024 23:09:50 +0000 Subject: [PATCH] chore(debugger): better no local error message (#11405) We improve the error message reported when an expression fails to resolve a reference to a local variable. ## 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 - [ ] 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/debugging/_expressions.py | 15 ++++++++++----- tests/debugging/test_debugger.py | 6 +++--- tests/debugging/test_debugger_span_decoration.py | 10 +++++++--- tests/debugging/test_expressions.py | 6 +++--- 4 files changed, 23 insertions(+), 14 deletions(-) diff --git a/ddtrace/debugging/_expressions.py b/ddtrace/debugging/_expressions.py index 5c50a97d12c..50028b9c6d2 100644 --- a/ddtrace/debugging/_expressions.py +++ b/ddtrace/debugging/_expressions.py @@ -86,6 +86,13 @@ def instanceof(value: Any, type_qname: str) -> bool: return False +def get_local(_locals: Mapping[str, Any], name: str) -> Any: + try: + return _locals[name] + except KeyError: + raise NameError(f"No such local variable: '{name}'") + + class DDCompiler: @classmethod def __getmember__(cls, o, a): @@ -235,11 +242,9 @@ def _compile_direct_operation(self, ast: DDASTType) -> Optional[List[Instr]]: if arg == "@it": return [Instr("LOAD_FAST", "_dd_it")] - return [ - Instr("LOAD_FAST", "_locals"), - Instr("LOAD_CONST", self.__ref__(arg)), - Instr("BINARY_SUBSCR"), - ] + return self._call_function( + get_local, [Instr("LOAD_FAST", "_locals")], [Instr("LOAD_CONST", self.__ref__(arg))] + ) return None diff --git a/tests/debugging/test_debugger.py b/tests/debugging/test_debugger.py index ad2771cfb71..2a0bdaf13d8 100644 --- a/tests/debugging/test_debugger.py +++ b/tests/debugging/test_debugger.py @@ -772,7 +772,7 @@ def test_debugger_condition_eval_error_get_reported_once(): evaluationErrors = snapshot["debugger"]["snapshot"]["evaluationErrors"] assert 1 == len(evaluationErrors) assert "foo == 42" == evaluationErrors[0]["expr"] - assert "'foo'" == evaluationErrors[0]["message"] + assert "No such local variable: 'foo'" == evaluationErrors[0]["message"] def test_debugger_function_probe_eval_on_entry(): @@ -881,6 +881,7 @@ def test_debugger_log_line_probe_generate_messages(): probe_id="foo", source_file="tests/submod/stuff.py", line=36, + rate=float("inf"), **compile_template( "hello world ", {"dsl": "foo", "json": {"ref": "foo"}}, @@ -899,8 +900,7 @@ def test_debugger_log_line_probe_generate_messages(): assert "hello world ERROR 456!" == msg2["message"], msg2 assert "foo" == msg1["debugger"]["snapshot"]["evaluationErrors"][0]["expr"], msg1 - # not amazing error message for a missing variable - assert "'foo'" == msg1["debugger"]["snapshot"]["evaluationErrors"][0]["message"], msg1 + assert "No such local variable: 'foo'" == msg1["debugger"]["snapshot"]["evaluationErrors"][0]["message"], msg1 assert not msg1["debugger"]["snapshot"]["captures"] diff --git a/tests/debugging/test_debugger_span_decoration.py b/tests/debugging/test_debugger_span_decoration.py index fd6e8d2aca7..5d7e51ee6a7 100644 --- a/tests/debugging/test_debugger_span_decoration.py +++ b/tests/debugging/test_debugger_span_decoration.py @@ -65,7 +65,7 @@ def test_debugger_span_decoration_probe_on_inner_function_active_span(self): assert span.get_tag("_dd.di.test.tag.probe_id") == "span-decoration" assert ( span.get_tag("_dd.di.test.bad.evaluation_error") - == "'Failed to evaluate expression \"test\": \\'notathing\\''" + == "'Failed to evaluate expression \"test\": No such local variable: \\'notathing\\''" ) assert not d.test_queue @@ -107,11 +107,15 @@ def test_debugger_span_decoration_probe_on_inner_function_active_span_unconditio assert span.get_tag("_dd.di.test.tag.probe_id") == "span-decoration" assert ( span.get_tag("_dd.di.test.bad.evaluation_error") - == "'Failed to evaluate expression \"test\": \\'notathing\\''" + == "'Failed to evaluate expression \"test\": No such local variable: \\'notathing\\''" ) (signal,) = d.test_queue - assert signal.errors == [EvaluationError(expr="test", message="Failed to evaluate condition: 'notathing'")] + assert signal.errors == [ + EvaluationError( + expr="test", message="Failed to evaluate condition: No such local variable: 'notathing'" + ) + ] (payload,) = d.uploader.wait_for_payloads() assert payload["message"] == "Condition evaluation errors for probe span-decoration" diff --git a/tests/debugging/test_expressions.py b/tests/debugging/test_expressions.py index 1ab4e539c04..f064a951893 100644 --- a/tests/debugging/test_expressions.py +++ b/tests/debugging/test_expressions.py @@ -50,7 +50,7 @@ def __getitem__(self, name): ({"len": {"getmember": [{"ref": "self"}, "collectionField"]}}, {"self": CustomObject("expr")}, 10), ({"len": {"getmember": [{"ref": "self"}, "_privateField"]}}, {"self": CustomObject("expr")}, len("private")), ({"len": {"getmember": [{"ref": "self"}, "bogusField"]}}, {"self": CustomObject("expr")}, AttributeError), - ({"len": {"ref": "payload"}}, {}, KeyError), + ({"len": {"ref": "payload"}}, {}, NameError), # Test plain references ({"ref": "hits"}, {"hits": 42}, 42), ({"getmember": [{"ref": "self"}, "name"]}, {"self": CustomObject("test-me")}, "test-me"), @@ -104,8 +104,8 @@ def __getitem__(self, name): (True, {}, True), ({"or": [{"ref": "bar"}, {"ref": "foo"}]}, {"bar": 42}, 42), ({"and": [{"ref": "bar"}, {"ref": "foo"}]}, {"bar": 0}, 0), - ({"or": [{"ref": "bar"}, {"ref": "foo"}]}, {"bar": 0}, KeyError), - ({"and": [{"ref": "bar"}, {"ref": "foo"}]}, {"bar": 42}, KeyError), + ({"or": [{"ref": "bar"}, {"ref": "foo"}]}, {"bar": 0}, NameError), + ({"and": [{"ref": "bar"}, {"ref": "foo"}]}, {"bar": 42}, NameError), ({"isDefined": "foobar"}, {"bar": 42}, False), ({"isDefined": "bar"}, {"bar": 42}, True), ({"instanceof": [{"ref": "bar"}, "int"]}, {"bar": 42}, True),