From 33b2499d7f615f40fcea098933dda4ca47d30c4b Mon Sep 17 00:00:00 2001 From: Munir Abdinur Date: Wed, 8 Jan 2025 12:36:11 -0500 Subject: [PATCH 1/4] chore(telemetry): make logs less noisy and more clear (#11853) Partially Resolves: https://github.com/DataDog/dd-trace-py/issues/10842 - Removes exception traceback from telemetry client logs. We should not generate a traceback everytime we fail to send telemetry payloads to the agent. This traceback is noisy and not actionable. - Updates telemetry client log message to clearly state that instrumentation telemetry failed to send and not user telemetry (ex: traces, logs, metrics). ## 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/internal/telemetry/writer.py | 14 ++++++++++---- tests/telemetry/test_writer.py | 2 +- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/ddtrace/internal/telemetry/writer.py b/ddtrace/internal/telemetry/writer.py index 71de6b03907..2be240c06fd 100644 --- a/ddtrace/internal/telemetry/writer.py +++ b/ddtrace/internal/telemetry/writer.py @@ -118,11 +118,17 @@ def send_event(self, request: Dict) -> Optional[httplib.HTTPResponse]: conn.request("POST", self._endpoint, rb_json, headers) resp = get_connection_response(conn) if resp.status < 300: - log.debug("sent %d in %.5fs to %s. response: %s", len(rb_json), sw.elapsed(), self.url, resp.status) + log.debug( + "Instrumentation Telemetry sent %d in %.5fs to %s. response: %s", + len(rb_json), + sw.elapsed(), + self.url, + resp.status, + ) else: - log.debug("failed to send telemetry to %s. response: %s", self.url, resp.status) - except Exception: - log.debug("failed to send telemetry to %s.", self.url, exc_info=True) + log.debug("Failed to send Instrumentation Telemetry to %s. response: %s", self.url, resp.status) + except Exception as e: + log.debug("Failed to send Instrumentation Telemetry to %s. Error: %s", self.url, str(e)) finally: if conn is not None: conn.close() diff --git a/tests/telemetry/test_writer.py b/tests/telemetry/test_writer.py index bcc3be9e38c..3b5ec7226af 100644 --- a/tests/telemetry/test_writer.py +++ b/tests/telemetry/test_writer.py @@ -638,7 +638,7 @@ def test_send_failing_request(mock_status, telemetry_writer): telemetry_writer.periodic(force_flush=True) # asserts unsuccessful status code was logged log.debug.assert_called_with( - "failed to send telemetry to %s. response: %s", + "Failed to send Instrumentation Telemetry to %s. response: %s", telemetry_writer._client.url, mock_status, ) From 86a367a4112eddce00591169056046136af3785e Mon Sep 17 00:00:00 2001 From: Munir Abdinur Date: Wed, 8 Jan 2025 12:46:26 -0500 Subject: [PATCH 2/4] chore: skip failing urlib3 ASM test (#11861) - There seems to be a bug in the ASM Standalone logic where the `_dd.p.appsec` tag is not being set on `urllib3` spans. Since this tag is not set x-datadog distributed tracing headers are not generated. ## 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/contrib/urllib3/test_urllib3.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tests/contrib/urllib3/test_urllib3.py b/tests/contrib/urllib3/test_urllib3.py index 2f0c447ee65..841e2c826ab 100644 --- a/tests/contrib/urllib3/test_urllib3.py +++ b/tests/contrib/urllib3/test_urllib3.py @@ -12,6 +12,7 @@ from ddtrace.ext import http from ddtrace.internal.schema import DEFAULT_SPAN_SERVICE_NAME from ddtrace.pin import Pin +from ddtrace.settings.asm import config as asm_config from tests.contrib.config import HTTPBIN_CONFIG from tests.opentracer.utils import init_tracer from tests.utils import TracerTestCase @@ -527,12 +528,16 @@ def test_distributed_tracing_disabled(self): timeout=mock.ANY, ) + @pytest.mark.skip(reason="urlib3 does not set the ASM Manual keep tag so x-datadog headers are not propagated") def test_distributed_tracing_apm_opt_out_true(self): """Tests distributed tracing headers are passed by default""" # Check that distributed tracing headers are passed down; raise an error rather than make the # request since we don't care about the response at all config.urllib3["distributed_tracing"] = True self.tracer.enabled = False + # Ensure the ASM SpanProcessor is set + self.tracer.configure(appsec_standalone_enabled=True, appsec_enabled=True) + assert asm_config._apm_opt_out with mock.patch( "urllib3.connectionpool.HTTPConnectionPool._make_request", side_effect=ValueError ) as m_make_request: @@ -580,6 +585,9 @@ def test_distributed_tracing_apm_opt_out_false(self): """Test with distributed tracing disabled does not propagate the headers""" config.urllib3["distributed_tracing"] = True self.tracer.enabled = False + # Ensure the ASM SpanProcessor is set. + self.tracer.configure(appsec_standalone_enabled=False, appsec_enabled=True) + assert not asm_config._apm_opt_out with mock.patch( "urllib3.connectionpool.HTTPConnectionPool._make_request", side_effect=ValueError ) as m_make_request: From 6c61f40f63afdda1bdb2fff72e9b98f8d4f35956 Mon Sep 17 00:00:00 2001 From: Brett Langdon Date: Wed, 8 Jan 2025 15:07:23 -0500 Subject: [PATCH 3/4] ci: fix flaky es search query (#11877) --- .../elasticsearch/test_elasticsearch.py | 30 +++++++++++++++---- 1 file changed, 24 insertions(+), 6 deletions(-) diff --git a/tests/contrib/elasticsearch/test_elasticsearch.py b/tests/contrib/elasticsearch/test_elasticsearch.py index b80b4486e71..4a480c550c8 100644 --- a/tests/contrib/elasticsearch/test_elasticsearch.py +++ b/tests/contrib/elasticsearch/test_elasticsearch.py @@ -1,6 +1,7 @@ import datetime from http.client import HTTPConnection from importlib import import_module +import json import time import pytest @@ -167,7 +168,12 @@ def test_elasticsearch(self): es.index(id=10, body={"name": "ten", "created": datetime.date(2016, 1, 1)}, **args) es.index(id=11, body={"name": "eleven", "created": datetime.date(2016, 2, 1)}, **args) es.index(id=12, body={"name": "twelve", "created": datetime.date(2016, 3, 1)}, **args) - result = es.search(sort=["name:desc"], size=100, body={"query": {"match_all": {}}}, **args) + result = es.search( + sort={"name": {"order": "desc", "unmapped_type": "keyword"}}, + size=100, + body={"query": {"match_all": {}}}, + **args, + ) assert len(result["hits"]["hits"]) == 3, result spans = self.get_spans() @@ -183,13 +189,25 @@ def test_elasticsearch(self): assert url.endswith("/_search") assert url == span.get_tag("elasticsearch.url") if elasticsearch.__version__ >= (8, 0, 0): - assert span.get_tag("elasticsearch.body").replace(" ", "") == '{"query":{"match_all":{}},"size":100}' - assert set(span.get_tag("elasticsearch.params").split("&")) == {"sort=name%3Adesc"} - assert set(span.get_tag(http.QUERY_STRING).split("&")) == {"sort=name%3Adesc"} + # Key order is not consistent, parse into dict to compare + body = json.loads(span.get_tag("elasticsearch.body")) + assert body == { + "query": {"match_all": {}}, + "sort": {"name": {"order": "desc", "unmapped_type": "keyword"}}, + "size": 100, + } + assert not span.get_tag("elasticsearch.params") + assert not span.get_tag(http.QUERY_STRING) else: assert span.get_tag("elasticsearch.body").replace(" ", "") == '{"query":{"match_all":{}}}' - assert set(span.get_tag("elasticsearch.params").split("&")) == {"sort=name%3Adesc", "size=100"} - assert set(span.get_tag(http.QUERY_STRING).split("&")) == {"sort=name%3Adesc", "size=100"} + assert set(span.get_tag("elasticsearch.params").split("&")) == { + "sort=%7B%27name%27%3A+%7B%27order%27%3A+%27desc%27%2C+%27unmapped_type%27%3A+%27keyword%27%7D%7D", + "size=100", + } + assert set(span.get_tag(http.QUERY_STRING).split("&")) == { + "sort=%7B%27name%27%3A+%7B%27order%27%3A+%27desc%27%2C+%27unmapped_type%27%3A+%27keyword%27%7D%7D", + "size=100", + } assert span.get_tag("component") == "elasticsearch" assert span.get_tag("span.kind") == "client" From 75bed24c32dd865e83353455d1c61cb5f6265f56 Mon Sep 17 00:00:00 2001 From: wantsui Date: Wed, 8 Jan 2025 16:45:49 -0500 Subject: [PATCH 4/4] chore: clarify usage and wording for current_root_span (#11764) Following up on https://github.com/DataDog/dd-trace-py/issues/9758, our current definition of current_root_span is a bit misleading. This attempts to clarify the definition since in a distributed trace, you can't actually use this to grab the root span. (The only thing it consistently returns is "local root"). ## 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/_trace/tracer.py | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/ddtrace/_trace/tracer.py b/ddtrace/_trace/tracer.py index f815e0f184e..02d5fed7626 100644 --- a/ddtrace/_trace/tracer.py +++ b/ddtrace/_trace/tracer.py @@ -940,18 +940,23 @@ def trace( ) def current_root_span(self) -> Optional[Span]: - """Returns the root span of the current execution. + """Returns the local root span of the current execution/process. - This is useful for attaching information related to the trace as a - whole without needing to add to child spans. + Note: This cannot be used to access the true root span of the trace + in a distributed tracing setup if the actual root span occurred in + another execution/process. + + This is useful for attaching information to the local root span + of the current execution/process, which is often also service + entry span. For example:: - # get the root span - root_span = tracer.current_root_span() + # get the local root span + local_root_span = tracer.current_root_span() # set the host just once on the root span - if root_span: - root_span.set_tag('host', '127.0.0.1') + if local_root_span: + local_root_span.set_tag('host', '127.0.0.1') """ span = self.current_span() if span is None: