From 4beeccc975c0b00b490903eb0b5dd3f9ace5fdef Mon Sep 17 00:00:00 2001 From: Yun Kim <35776586+Yun-Kim@users.noreply.github.com> Date: Fri, 10 Jan 2025 11:29:38 -0500 Subject: [PATCH 1/4] chore(llmobs): fix tests to be less noisy locally (#11890) These tests rely on env vars that may already be present on dev envs and will result in noisy errors as a result. ## 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) --- tests/llmobs/test_llmobs_evaluator_runner.py | 12 ++---------- tests/llmobs/test_llmobs_ragas_evaluators.py | 15 +++------------ 2 files changed, 5 insertions(+), 22 deletions(-) diff --git a/tests/llmobs/test_llmobs_evaluator_runner.py b/tests/llmobs/test_llmobs_evaluator_runner.py index eb0be25c91..40c9fb5bd2 100644 --- a/tests/llmobs/test_llmobs_evaluator_runner.py +++ b/tests/llmobs/test_llmobs_evaluator_runner.py @@ -64,15 +64,7 @@ def test_evaluator_runner_on_exit(mock_writer_logs, run_python_code_in_subproces pypath = [os.path.dirname(os.path.dirname(os.path.dirname(os.path.dirname(__file__))))] if "PYTHONPATH" in env: pypath.append(env["PYTHONPATH"]) - env.update( - { - "DD_API_KEY": os.getenv("DD_API_KEY", "dummy-api-key"), - "DD_SITE": "datad0g.com", - "PYTHONPATH": ":".join(pypath), - "DD_LLMOBS_ML_APP": "unnamed-ml-app", - "_DD_LLMOBS_EVALUATOR_INTERVAL": "5", - } - ) + env.update({"PYTHONPATH": ":".join(pypath), "_DD_LLMOBS_EVALUATOR_INTERVAL": "5"}) out, err, status, pid = run_python_code_in_subprocess( """ import os @@ -87,7 +79,7 @@ def test_evaluator_runner_on_exit(mock_writer_logs, run_python_code_in_subproces ctx = logs_vcr.use_cassette("tests.llmobs.test_llmobs_evaluator_runner.send_score_metric.yaml") ctx.__enter__() atexit.register(lambda: ctx.__exit__()) -LLMObs.enable() +LLMObs.enable(api_key="dummy-api-key", site="datad0g.com", ml_app="unnamed-ml-app") LLMObs._instance._evaluator_runner.evaluators.append(DummyEvaluator(llmobs_service=LLMObs)) LLMObs._instance._evaluator_runner.start() LLMObs._instance._evaluator_runner.enqueue({"span_id": "123", "trace_id": "1234"}, None) diff --git a/tests/llmobs/test_llmobs_ragas_evaluators.py b/tests/llmobs/test_llmobs_ragas_evaluators.py index 9df6c39247..251b264204 100644 --- a/tests/llmobs/test_llmobs_ragas_evaluators.py +++ b/tests/llmobs/test_llmobs_ragas_evaluators.py @@ -206,14 +206,11 @@ def test_llmobs_with_faithfulness_emits_traces_and_evals_on_exit(mock_writer_log pypath.append(env["PYTHONPATH"]) env.update( { - "DD_API_KEY": os.getenv("DD_API_KEY", "dummy-api-key"), - "DD_SITE": "datad0g.com", "PYTHONPATH": ":".join(pypath), "OPENAI_API_KEY": os.getenv("OPENAI_API_KEY", "dummy-openai-api-key"), - "DD_LLMOBS_ML_APP": "unnamed-ml-app", "_DD_LLMOBS_EVALUATOR_INTERVAL": "5", "_DD_LLMOBS_EVALUATORS": "ragas_faithfulness", - "DD_LLMOBS_AGENTLESS_ENABLED": "1", + "DD_TRACE_ENABLED": "0", } ) out, err, status, pid = run_python_code_in_subprocess( @@ -232,14 +229,8 @@ def test_llmobs_with_faithfulness_emits_traces_and_evals_on_exit(mock_writer_log ) ctx.__enter__() atexit.register(lambda: ctx.__exit__()) -with mock.patch( - "ddtrace.internal.writer.HTTPWriter._send_payload", - return_value=Response( - status=200, - body="{}", - ), -): - LLMObs.enable() +with mock.patch("ddtrace.internal.writer.HTTPWriter._send_payload", return_value=Response(status=200, body="{}")): + LLMObs.enable(api_key="dummy-api-key", site="datad0g.com", ml_app="unnamed-ml-app", agentless_enabled=True) LLMObs._instance._evaluator_runner.enqueue(_llm_span_with_expected_ragas_inputs_in_messages(), None) """, env=env, From 8702cab2464d12c6b1664a38213166ce31fdd63b Mon Sep 17 00:00:00 2001 From: Yun Kim <35776586+Yun-Kim@users.noreply.github.com> Date: Fri, 10 Jan 2025 13:26:13 -0500 Subject: [PATCH 2/4] fix(llmobs): move listener hooks to enable instead of on init (#11889) Follow up on #11781 to fix a weird duplicate span writing issue with the new listener hook logic. Since we were registering these hooks on `LLMObs.__init__()` which also happens at startup (as we create a default LLMObs() instance) as well as on `LLMObs.enable()`, we were double registering these hooks, and the default LLMObsSpanWriter was still saved and called each time the tracer finished a span. A symptom of this issue is that if a user was to manually enable agentless mode, they would see noisy logs indicating a failure to send spans to the agent proxy endpoint (which is the default writer mode) even though they also submitted spans to the agentless endpoint succesfully. This fix resolves the issue by moving the hook registering to `LLMObs.enable()`, and adding corresponding logic to deregister the hooks on `_stop_service()`. This way we should only ever have one set of hooks registered per process. ## 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/llmobs/_llmobs.py | 14 ++++++++-- ...default-writer-hooks-5e456c2f7dfd4381.yaml | 4 +++ tests/llmobs/test_llmobs_service.py | 28 +++++++++++++++++++ 3 files changed, 43 insertions(+), 3 deletions(-) create mode 100644 releasenotes/notes/fix-llmobs-default-writer-hooks-5e456c2f7dfd4381.yaml diff --git a/ddtrace/llmobs/_llmobs.py b/ddtrace/llmobs/_llmobs.py index 91c49306b3..b4f1dc1b2f 100644 --- a/ddtrace/llmobs/_llmobs.py +++ b/ddtrace/llmobs/_llmobs.py @@ -111,9 +111,9 @@ def __init__(self, tracer=None): self._annotations = [] self._annotation_context_lock = forksafe.RLock() - # Register hooks for span events - core.on("trace.span_start", self._do_annotations) - core.on("trace.span_finish", self._on_span_finish) + def _on_span_start(self, span): + if self.enabled and span.span_type == SpanTypes.LLM: + self._do_annotations(span) def _on_span_finish(self, span): if self.enabled and span.span_type == SpanTypes.LLM: @@ -272,6 +272,10 @@ def _start_service(self) -> None: log.debug("Error starting evaluator runner") def _stop_service(self) -> None: + # Remove listener hooks for span events + core.reset_listeners("trace.span_start", self._on_span_start) + core.reset_listeners("trace.span_finish", self._on_span_finish) + try: self._evaluator_runner.stop() # flush remaining evaluation spans & evaluations @@ -366,6 +370,10 @@ def enable( cls.enabled = True cls._instance.start() + # Register hooks for span events + core.on("trace.span_start", cls._instance._on_span_start) + core.on("trace.span_finish", cls._instance._on_span_finish) + atexit.register(cls.disable) telemetry_writer.product_activated(TELEMETRY_APM_PRODUCT.LLMOBS, True) diff --git a/releasenotes/notes/fix-llmobs-default-writer-hooks-5e456c2f7dfd4381.yaml b/releasenotes/notes/fix-llmobs-default-writer-hooks-5e456c2f7dfd4381.yaml new file mode 100644 index 0000000000..702e2538b9 --- /dev/null +++ b/releasenotes/notes/fix-llmobs-default-writer-hooks-5e456c2f7dfd4381.yaml @@ -0,0 +1,4 @@ +--- +fixes: + - | + LLM Observability: Resolves an issue where enabling LLM Observability in agentless mode would result in traces also being sent to the agent proxy endpoint. \ No newline at end of file diff --git a/tests/llmobs/test_llmobs_service.py b/tests/llmobs/test_llmobs_service.py index 4c7f207066..dad6accdcf 100644 --- a/tests/llmobs/test_llmobs_service.py +++ b/tests/llmobs/test_llmobs_service.py @@ -1310,6 +1310,34 @@ def test_activate_distributed_headers_activates_context(llmobs, mock_llmobs_logs mock_activate.assert_called_once_with(dummy_context) +def test_listener_hooks_enqueue_correct_writer(run_python_code_in_subprocess): + """ + Regression test that ensures that listener hooks enqueue span events to the correct writer, + not the default writer created at startup. + """ + env = os.environ.copy() + pypath = [os.path.dirname(os.path.dirname(os.path.dirname(os.path.dirname(__file__))))] + if "PYTHONPATH" in env: + pypath.append(env["PYTHONPATH"]) + env.update({"PYTHONPATH": ":".join(pypath), "DD_TRACE_ENABLED": "0"}) + out, err, status, pid = run_python_code_in_subprocess( + """ +from ddtrace.llmobs import LLMObs + +LLMObs.enable(ml_app="repro-issue", agentless_enabled=True, api_key="foobar.baz", site="datad0g.com") +with LLMObs.agent("dummy"): + pass +""", + env=env, + ) + assert status == 0, err + assert out == b"" + agentless_writer_log = b"failed to send traces to intake at https://llmobs-intake.datad0g.com/api/v2/llmobs: HTTP error status 403, reason Forbidden\n" # noqa: E501 + agent_proxy_log = b"failed to send, dropping 1 traces to intake at http://localhost:8126/evp_proxy/v2/api/v2/llmobs after 5 retries" # noqa: E501 + assert err == agentless_writer_log + assert agent_proxy_log not in err + + def test_llmobs_fork_recreates_and_restarts_span_writer(): """Test that forking a process correctly recreates and restarts the LLMObsSpanWriter.""" with mock.patch("ddtrace.internal.writer.HTTPWriter._send_payload"): From d7927e6296679a3d7def5b31347262b7cce8b7dd Mon Sep 17 00:00:00 2001 From: "Gabriele N. Tornetta" Date: Fri, 10 Jan 2025 18:45:32 +0000 Subject: [PATCH 3/4] fix(er): include nonlocals in snapshots (#11894) We include nonlocal variables in snapshots to provide for better visibility into exception occurrences. ## 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/_safety.py | 9 +++++---- ...er-include-nonlocals-bbeecfbbbde35496.yaml | 4 ++++ tests/debugging/exception/test_replay.py | 20 +++++++++++++++++++ tests/debugging/test_safety.py | 5 ++++- 4 files changed, 33 insertions(+), 5 deletions(-) create mode 100644 releasenotes/notes/fix-er-include-nonlocals-bbeecfbbbde35496.yaml diff --git a/ddtrace/debugging/_safety.py b/ddtrace/debugging/_safety.py index 118deddef4..92b38ff6bd 100644 --- a/ddtrace/debugging/_safety.py +++ b/ddtrace/debugging/_safety.py @@ -1,5 +1,6 @@ from inspect import CO_VARARGS from inspect import CO_VARKEYWORDS +from itertools import chain from types import FrameType from typing import Any from typing import Dict @@ -23,11 +24,11 @@ def get_args(frame: FrameType) -> Iterator[Tuple[str, Any]]: def get_locals(frame: FrameType) -> Iterator[Tuple[str, Any]]: code = frame.f_code + _locals = frame.f_locals nargs = code.co_argcount + bool(code.co_flags & CO_VARARGS) + bool(code.co_flags & CO_VARKEYWORDS) - names = code.co_varnames[nargs:] - values = (frame.f_locals.get(name) for name in names) - - return zip(names, values) + return ( + (name, _locals.get(name)) for name in chain(code.co_varnames[nargs:], code.co_freevars, code.co_cellvars) + ) # include freevars and cellvars def get_globals(frame: FrameType) -> Iterator[Tuple[str, Any]]: diff --git a/releasenotes/notes/fix-er-include-nonlocals-bbeecfbbbde35496.yaml b/releasenotes/notes/fix-er-include-nonlocals-bbeecfbbbde35496.yaml new file mode 100644 index 0000000000..4d77fddb71 --- /dev/null +++ b/releasenotes/notes/fix-er-include-nonlocals-bbeecfbbbde35496.yaml @@ -0,0 +1,4 @@ +--- +fixes: + - | + exception replay: include missing nonlocal variables in snapshot log messages. diff --git a/tests/debugging/exception/test_replay.py b/tests/debugging/exception/test_replay.py index 9aae75dae4..8261bfb5b4 100644 --- a/tests/debugging/exception/test_replay.py +++ b/tests/debugging/exception/test_replay.py @@ -294,3 +294,23 @@ def c(foo=42): self.assert_span_count(6) # no new snapshots assert len(uploader.collector.queue) == 3 + + def test_debugger_exception_in_closure(self): + def b(): + with self.trace("b"): + nonloc = 4 + + def a(v): + if nonloc: + raise ValueError("hello", v) + + a(nonloc) + + with exception_replay() as uploader: + with with_rate_limiter(RateLimiter(limit_rate=1, raise_on_exceed=False)): + with pytest.raises(ValueError): + b() + + assert all( + s.line_capture["locals"]["nonloc"] == {"type": "int", "value": "4"} for s in uploader.collector.queue + ) diff --git a/tests/debugging/test_safety.py b/tests/debugging/test_safety.py index 3acb028892..cc44ca9ca1 100644 --- a/tests/debugging/test_safety.py +++ b/tests/debugging/test_safety.py @@ -15,7 +15,10 @@ def assert_args(args): assert set(dict(_safety.get_args(inspect.currentframe().f_back)).keys()) == args def assert_locals(_locals): - assert set(dict(_safety.get_locals(inspect.currentframe().f_back)).keys()) == _locals + assert set(dict(_safety.get_locals(inspect.currentframe().f_back)).keys()) == _locals | { + "assert_args", + "assert_locals", + } def assert_globals(_globals): assert set(dict(_safety.get_globals(inspect.currentframe().f_back)).keys()) == _globals From a58f139e24d78a66468dbc7f67ec42c2bdfad8ee Mon Sep 17 00:00:00 2001 From: Taegyun Kim Date: Fri, 10 Jan 2025 15:17:33 -0500 Subject: [PATCH 4/4] fix(profiling): remove `_self_acquired_at` after use (#11898) ## 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/profiling/collector/_lock.py | 1 + .../profiling-lock-acquired-at-remove-c8b5b96130a46ca8.yaml | 5 +++++ 2 files changed, 6 insertions(+) create mode 100644 releasenotes/notes/profiling-lock-acquired-at-remove-c8b5b96130a46ca8.yaml diff --git a/ddtrace/profiling/collector/_lock.py b/ddtrace/profiling/collector/_lock.py index ec62c5c0ee..ec0c438b2b 100644 --- a/ddtrace/profiling/collector/_lock.py +++ b/ddtrace/profiling/collector/_lock.py @@ -185,6 +185,7 @@ def _release(self, inner_func, *args, **kwargs): # _self_acquired_at is only set when the acquire was captured # if it's not set, we're not capturing the release start = self._self_acquired_at + del self._self_acquired_at try: return inner_func(*args, **kwargs) diff --git a/releasenotes/notes/profiling-lock-acquired-at-remove-c8b5b96130a46ca8.yaml b/releasenotes/notes/profiling-lock-acquired-at-remove-c8b5b96130a46ca8.yaml new file mode 100644 index 0000000000..fdb268dcd7 --- /dev/null +++ b/releasenotes/notes/profiling-lock-acquired-at-remove-c8b5b96130a46ca8.yaml @@ -0,0 +1,5 @@ +--- +fixes: + - | + profiling: resolves an issue where lock release would have been captured + with a wrong acquire timestamp