chore(django): refactor cache wrapping #14370
Conversation
## 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)
This PR removes the internal Span._context property and ensures that Span.context is never None. The current memory utilization benchmarks are too simplistic and don't account for scenarios where a Span is initialized with a Context. Since it's not possible to generate a Span without a context using the public ddtrace APIs, the benchmarks need to be updated to reflect real world memory overhead. ## Checklist - [ ] 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)
Small performance wins by: 1) avoiding using `set_tag` or `set_tags`; use `set_tag_str` instead 2) avoiding using `set_metric` and set value directly on `_metrics` when possible ## 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)
### Description
This PR aims to validate that the `{% trans %}` template tag works
correctly when using **Jinja2 + Babel**, particularly in the context of
**IAST** (Interactive Application Security Testing).
We’ve added a test case to confirm that templates using the `{% trans
href=url_for('...') %}` pattern are rendered properly with Flask +
Jinja2 + Babel. This ensures that the IAST instrumentation handles this
scenario as expected.
### Context
In other frameworks like Pyramid, we’ve observed the use of similar
syntax:
```jinja2
{% trans href=request.route_path('search') %}
```
However, **IAST currently does not support Pyramid**, so we’re unable to
validate that use case. If you're using frameworks like `warehouse`
(built on Pyramid), we **recommend not enabling IAST**, as it won’t be
able to detect vulnerabilities reliably in that context.
For a full list of supported frameworks, refer to the [[official
documentation](https://docs.datadoghq.com/security/code_security/iast/setup/compatibility/python/#supported-frameworks)](https://docs.datadoghq.com/security/code_security/iast/setup/compatibility/python/#supported-frameworks).
Issue related:
#14217
---
Let me know if you want to tailor it more to a specific audience (e.g.
internal devs, open-source community, etc.).
## 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)
…ch test (#14344) The solution in PR #14328 was incomplete. This PR ensures all exceptions are cleared up at the end of each test. I'm not adding a release note because this is a continuation of #14328, which already has a release note. ## 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)
## 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) Signed-off-by: Juanjo Alvarez <[email protected]>
This PR adds improved tracking of tool results and definitions to the LLMObs VertexAI integration note: also includes some better test coverage for google genai note: although not adding support to generativeai since its deprecated, had to tweak tests to pass since vertex and gemini share the way they handle parts. ## 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)
…ising error (#14319) given a snippet like this ``` def divide(n1, n2): return n1/n2 def print_divide(n1, n2): print(divide(n1, n2)) def generate_capital_name_one_word(input_data: Dict[str, Any], config: Dict[str, Any]) -> str: print_divide(1, 0) output = oai_client.chat.completions.create( model=config["model"], messages=[ {"role": "system", "content": "You will respond only with the name of the capital of the country, nothing else."}, {"role": "user", "content": "What is the capital of France?"}, {"role": "assistant", "content": "Paris"}, {"role": "user", "content": input_data["question"]}], temperature=config["temperature"] ) return output.choices[0].message.content def exact_match(input_data, output_data, expected_output): raise ValueError("BBB") return expected_output == output_data def contains_answer(input_data, output_data, expected_output): return expected_output in output_data experiment = LLMObs.experiment( name="generate-one-word-capital-with-config", dataset=dataset, task=generate_capital_name_one_word, evaluators=[exact_match, contains_answer], config={"model": "gpt-4.1-nano", "temperature": 0}, description="a cool basic experiment with config", ) ``` we get an error message like so: ``` --------------------------------------------------------------------------- RuntimeError Traceback (most recent call last) Cell In[8], line 1 ----> 1 experiment.run(jobs=1,raise_errors=True) 3 experiment.url File ~/go/src/github.com/DataDog/llm-observability/preview/experiments/notebooks/.venv/lib/python3.12/site-packages/ddtrace/llmobs/_experiment.py:330, in Experiment.run(self, jobs, raise_errors, sample_size) 328 self._tags["experiment_id"] = str(experiment_id) 329 self._run_name = experiment_run_name --> 330 task_results = self._run_task(jobs, raise_errors, sample_size) 331 evaluations = self._run_evaluators(task_results, raise_errors=raise_errors) 332 experiment_results = self._merge_results(task_results, evaluations) File ~/go/src/github.com/DataDog/llm-observability/preview/experiments/notebooks/.venv/lib/python3.12/site-packages/ddtrace/llmobs/_experiment.py:413, in Experiment._run_task(self, jobs, raise_errors, sample_size) 411 err_msg = err_dict.get("message") if isinstance(err_dict, dict) else None 412 if raise_errors and err_msg: --> 413 raise RuntimeError("Error on record {}: {}".format(result["idx"], err_msg)) 414 self._llmobs_instance.flush() # Ensure spans get submitted in serverless environments 415 return task_results RuntimeError: Error on record 0: division by zero ``` after this change the error will look more verbose with the stack trace on the original error ``` --------------------------------------------------------------------------- RuntimeError Traceback (most recent call last) Cell In[12], line 1 ----> 1 experiment.run(jobs=1,raise_errors=True) 3 experiment.url File ~/go/src/github.com/DataDog/dd-trace-py/ddtrace/llmobs/_experiment.py:330, in Experiment.run(self, jobs, raise_errors, sample_size) 328 self._tags["experiment_id"] = str(experiment_id) 329 self._run_name = experiment_run_name --> 330 task_results = self._run_task(jobs, raise_errors, sample_size) 331 evaluations = self._run_evaluators(task_results, raise_errors=raise_errors) 332 experiment_results = self._merge_results(task_results, evaluations) File ~/go/src/github.com/DataDog/dd-trace-py/ddtrace/llmobs/_experiment.py:415, in Experiment._run_task(self, jobs, raise_errors, sample_size) 413 err_type = err_dict.get("type") if isinstance(err_dict, dict) else None 414 if raise_errors and err_msg: --> 415 raise RuntimeError( 416 "Error on record {}: {}\n{}\n{}".format(result["idx"], err_msg, err_type, err_stack) 417 ) 418 self._llmobs_instance.flush() # Ensure spans get submitted in serverless environments 419 return task_results RuntimeError: Error on record 0: division by zero builtins.ZeroDivisionError Traceback (most recent call last): File "/Users/gary.huang/go/src/github.com/DataDog/dd-trace-py/ddtrace/llmobs/_experiment.py", line 365, in _process_record output_data = self._task(input_data, self._config) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/var/folders/lv/k26fs55j1dl8p2jq5n8jwdnc0000gq/T/ipykernel_77428/3580111335.py", line 8, in generate_capital_name_one_word print_divide(1, 0) File "/var/folders/lv/k26fs55j1dl8p2jq5n8jwdnc0000gq/T/ipykernel_77428/3580111335.py", line 5, in print_divide print(divide(n1, n2)) ^^^^^^^^^^^^^^ File "/var/folders/lv/k26fs55j1dl8p2jq5n8jwdnc0000gq/T/ipykernel_77428/3580111335.py", line 2, in divide return n1/n2 ~~^~~ ZeroDivisionError: division by zero ``` ## 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)
…ache.double.wrapped
brettlangdon
added
the
changelog/no-changelog
|
I made an attempt to also remove the double wrapping like #14312 but Django cache backends don't have a shared base property for accessing the underlying client, so would require a bunch of custom logic per-backend. So while we could do that for the built-in backends for redis and memcached, any third party backends we likely won't support and you'd end up with double wrapping.... so I want to think about it further before trying to implement that. |
|
|
Bootstrap import analysisComparison of import times between this PR and base. SummaryThe average import time from this PR is: 273 ± 5 ms. The average import time from base is: 282 ± 4 ms. The import time difference between this PR and base is: -9.6 ± 0.2 ms. Import time breakdownThe following import paths have shrunk:
|
brettlangdon
force-pushed
the
LANGPLAT-642/django.cache.double.wrapped
branch
from
6a312a5 to
93c38b0
Compare
Performance SLOsCandidate: LANGPLAT-642/django.cache.double.wrapped (93c38b0) 🔵 No Baseline Data (24 suites)🔵 coreapiscenario - 12/12 (2 unstable)🔵 No baseline data available for this suite
|
This change refactors the cache instrumentation layer for Django to migrate to it's own module and uses the bytecode wrappers instead.
Checklist
Reviewer Checklist