Skip to content

Commit

Permalink
chore(debugger): better no local error message (#11405)
Browse files Browse the repository at this point in the history
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)
  • Loading branch information
P403n1x87 authored Nov 15, 2024
1 parent ecd0ab1 commit 8e9edd3
Show file tree
Hide file tree
Showing 4 changed files with 23 additions and 14 deletions.
15 changes: 10 additions & 5 deletions ddtrace/debugging/_expressions.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down Expand Up @@ -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

Expand Down
6 changes: 3 additions & 3 deletions tests/debugging/test_debugger.py
Original file line number Diff line number Diff line change
Expand Up @@ -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():
Expand Down Expand Up @@ -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"}},
Expand All @@ -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"]

Expand Down
10 changes: 7 additions & 3 deletions tests/debugging/test_debugger_span_decoration.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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"
Expand Down
6 changes: 3 additions & 3 deletions tests/debugging/test_expressions.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"),
Expand Down Expand Up @@ -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),
Expand Down

0 comments on commit 8e9edd3

Please sign in to comment.