From 193c5546d8f3bcc768d042ac2d6f879e32c23772 Mon Sep 17 00:00:00 2001 From: Federico Mon Date: Sat, 12 Aug 2023 02:13:51 +0200 Subject: [PATCH 1/3] fix(httplib): avoid tracing requests made by trace writers (#6635) ## Description Avoids tracing http requests made by trace writers to datadog internal services (i.e. prevents the ci visibility writer from sending traced requests to internal services). ## Motivation - Prevents tracing internals from being traced. - Resolves flakiness in the httplib test suite with the agentless ci visibility writer ## Checklist - [x] Change(s) are motivated and described in the PR description. - [x] Testing strategy is described if automated tests are not included in the PR. - [x] Risk is outlined (performance impact, potential for breakage, maintainability, etc). - [x] Change is maintainable (easy to change, telemetry, documentation). - [x] [Library release note guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html) are followed. If no release note is required, add label `changelog/no-changelog`. - [x] Documentation is included (in-code, generated user docs, [public corp docs](https://github.com/DataDog/documentation/)). - [x] Backport labels are set (if [applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)) ## Reviewer Checklist - [x] Title is accurate. - [x] No unnecessary changes are introduced. - [x] Description motivates each change. - [x] Avoids breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes unless absolutely necessary. - [x] Testing strategy adequately addresses listed risk(s). - [x] Change is maintainable (easy to change, telemetry, documentation). - [x] Release note makes sense to a user of the library. - [x] Reviewer has explicitly acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment. - [x] 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) Co-authored-by: Munir Abdinur --- ddtrace/contrib/httplib/patch.py | 7 +++++++ ddtrace/internal/constants.py | 2 ++ ddtrace/internal/writer/writer.py | 8 +++++--- tests/ci_visibility/test_ci_visibility.py | 6 +++--- tests/contrib/httplib/test_httplib.py | 19 +++++++++++++++++++ tests/utils.py | 2 +- 6 files changed, 37 insertions(+), 7 deletions(-) diff --git a/ddtrace/contrib/httplib/patch.py b/ddtrace/contrib/httplib/patch.py index 656cf4383c5..4f23506faac 100644 --- a/ddtrace/contrib/httplib/patch.py +++ b/ddtrace/contrib/httplib/patch.py @@ -11,6 +11,7 @@ from .. import trace_utils from ...constants import ANALYTICS_SAMPLE_RATE_KEY from ...constants import SPAN_KIND +from ...internal.constants import _HTTPLIB_NO_TRACE_REQUEST from ...ext import SpanKind from ...ext import SpanTypes from ...internal.compat import PY2 @@ -183,9 +184,15 @@ def _wrap_putheader(func, instance, args, kwargs): def should_skip_request(pin, request): """Helper to determine if the provided request should be traced""" + if getattr(request, _HTTPLIB_NO_TRACE_REQUEST, False): + return True + if not pin or not pin.enabled(): return True + # httplib is used to send apm events (profiling,di, tracing, etc.) to the datadog agent + # Tracing these requests introduces a significant noise and instability in ddtrace tests. + # TO DO: Avoid tracing requests to APM internal services (ie: extend this functionality to agentless products). agent_url = pin.tracer.agent_trace_url if agent_url: parsed = parse.urlparse(agent_url) diff --git a/ddtrace/internal/constants.py b/ddtrace/internal/constants.py index 3a469f77018..d0ad5e05f25 100644 --- a/ddtrace/internal/constants.py +++ b/ddtrace/internal/constants.py @@ -66,3 +66,5 @@ FLASK_ENDPOINT = "flask.endpoint" FLASK_VIEW_ARGS = "flask.view_args" FLASK_URL_RULE = "flask.url_rule" + +_HTTPLIB_NO_TRACE_REQUEST = "_dd_no_trace" diff --git a/ddtrace/internal/writer/writer.py b/ddtrace/internal/writer/writer.py index 45d2265dc56..e5bdb45ef51 100644 --- a/ddtrace/internal/writer/writer.py +++ b/ddtrace/internal/writer/writer.py @@ -33,6 +33,7 @@ from .._encoding import BufferFull from .._encoding import BufferItemTooLarge from ..agent import get_connection +from ..constants import _HTTPLIB_NO_TRACE_REQUEST from ..encoding import JSONEncoderV2 from ..logger import get_logger from ..runtime import container @@ -281,14 +282,15 @@ def _reset_connection(self): self._conn.close() self._conn = None - def _put(self, data, headers, client): - # type: (bytes, Dict[str, str], WriterClientBase) -> Response + def _put(self, data, headers, client, no_trace): + # type: (bytes, Dict[str, str], WriterClientBase, bool) -> Response sw = StopWatch() sw.start() with self._conn_lck: if self._conn is None: log.debug("creating new intake connection to %s with timeout %d", self.intake_url, self._timeout) self._conn = get_connection(self._intake_url(client), self._timeout) + setattr(self._conn, _HTTPLIB_NO_TRACE_REQUEST, no_trace) try: log.debug("Sending request: %s %s %s", self.HTTP_METHOD, client.ENDPOINT, headers) self._conn.request( @@ -329,7 +331,7 @@ def _send_payload(self, payload, count, client): self._metrics_dist("http.requests") - response = self._put(payload, headers, client) + response = self._put(payload, headers, client, no_trace=True) if response.status >= 400: self._metrics_dist("http.errors", tags=("type:%s" % response.status,)) diff --git a/tests/ci_visibility/test_ci_visibility.py b/tests/ci_visibility/test_ci_visibility.py index 6e1fc28bda4..4359b71e376 100644 --- a/tests/ci_visibility/test_ci_visibility.py +++ b/tests/ci_visibility/test_ci_visibility.py @@ -438,7 +438,7 @@ def test_civisibilitywriter_coverage_agentless_url(): assert cov_client._intake_url == "https://citestcov-intake.datadoghq.com" with mock.patch("ddtrace.internal.writer.writer.get_connection") as _get_connection: - dummy_writer._put("", {}, cov_client) + dummy_writer._put("", {}, cov_client, no_trace=True) _get_connection.assert_called_once_with("https://citestcov-intake.datadoghq.com", 2.0) @@ -456,7 +456,7 @@ def test_civisibilitywriter_coverage_agentless_with_intake_url_param(): assert cov_client._intake_url == "https://citestcov-intake.datadoghq.com" with mock.patch("ddtrace.internal.writer.writer.get_connection") as _get_connection: - dummy_writer._put("", {}, cov_client) + dummy_writer._put("", {}, cov_client, no_trace=True) _get_connection.assert_called_once_with("https://citestcov-intake.datadoghq.com", 2.0) @@ -474,7 +474,7 @@ def test_civisibilitywriter_coverage_evp_proxy_url(): assert cov_client.ENDPOINT == "/evp_proxy/v2/api/v2/citestcov" with mock.patch("ddtrace.internal.writer.writer.get_connection") as _get_connection: - dummy_writer._put("", {}, cov_client) + dummy_writer._put("", {}, cov_client, no_trace=True) _get_connection.assert_called_once_with("http://localhost:8126", 2.0) diff --git a/tests/contrib/httplib/test_httplib.py b/tests/contrib/httplib/test_httplib.py index aba0d469e3c..6b4a9eb21dd 100644 --- a/tests/contrib/httplib/test_httplib.py +++ b/tests/contrib/httplib/test_httplib.py @@ -12,6 +12,7 @@ from ddtrace.internal.compat import PY2 from ddtrace.internal.compat import httplib from ddtrace.internal.compat import parse +from ddtrace.internal.constants import _HTTPLIB_NO_TRACE_REQUEST from ddtrace.internal.schema import DEFAULT_SPAN_SERVICE_NAME from ddtrace.pin import Pin from ddtrace.vendor import wrapt @@ -134,6 +135,24 @@ def test_should_skip_request(self): request = self.get_http_connection(parsed.hostname, parsed.port) pin = Pin.get_from(request) self.assertTrue(should_skip_request(pin, request)) + + def test_httplib_request_get_request_no_ddtrace(self): + """ + When making a GET request via httplib.HTTPConnection.request + while setting _dd_no_trace attr to True + we do not capture any spans + """ + self.tracer.enabled = True + conn = self.get_http_connection(SOCKET) + setattr(conn, _HTTPLIB_NO_TRACE_REQUEST, True) + with contextlib.closing(conn): + conn.request("GET", "/status/200") + resp = conn.getresponse() + self.assertEqual(self.to_str(resp.read()), "") + self.assertEqual(resp.status, 200) + + spans = self.pop_spans() + self.assertEqual(len(spans), 0) def test_httplib_request_get_request(self, query_string=""): """ diff --git a/tests/utils.py b/tests/utils.py index 9f3a1af32c6..e3e03bd9153 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -1192,7 +1192,7 @@ def flush_test_tracer_spans(writer): if encoded_traces is None: return headers = writer._get_finalized_headers(n_traces, client) - response = writer._put(encoded_traces, add_dd_env_variables_to_headers(headers), client) + response = writer._put(encoded_traces, add_dd_env_variables_to_headers(headers), client, no_trace=True) except Exception: return From 7d80bd570d5caf21c63d36bfe30f87da58da5a63 Mon Sep 17 00:00:00 2001 From: "Tahir H. Butt" Date: Sat, 12 Aug 2023 01:15:45 -0400 Subject: [PATCH 2/3] chore: migrate prechecks from riot (#6640) ## Checklist - [x] Change(s) are motivated and described in the PR description. - [x] Testing strategy is described if automated tests are not included in the PR. - [x] Risk is outlined (performance impact, potential for breakage, maintainability, etc). - [x] Change is maintainable (easy to change, telemetry, documentation). - [x] [Library release note guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html) are followed. If no release note is required, add label `changelog/no-changelog`. - [x] Documentation is included (in-code, generated user docs, [public corp docs](https://github.com/DataDog/documentation/)). - [x] Backport labels are set (if [applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)) ## Reviewer Checklist - [x] Title is accurate. - [x] No unnecessary changes are introduced. - [x] Description motivates each change. - [x] Avoids breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes unless absolutely necessary. - [x] Testing strategy adequately addresses listed risk(s). - [x] Change is maintainable (easy to change, telemetry, documentation). - [x] Release note makes sense to a user of the library. - [x] Reviewer has explicitly acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment. - [x] 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) Co-authored-by: Emmett Butler <723615+emmettbutler@users.noreply.github.com> --- .circleci/config.templ.yml | 1 - hatch.toml | 27 +++++++++++++++++++++++++++ riotfile.py | 10 ---------- scripts/gen_circleci_config.py | 4 ++-- 4 files changed, 29 insertions(+), 13 deletions(-) diff --git a/.circleci/config.templ.yml b/.circleci/config.templ.yml index 760b4967bf4..8322a0b9d89 100644 --- a/.circleci/config.templ.yml +++ b/.circleci/config.templ.yml @@ -243,7 +243,6 @@ jobs: executor: python310 steps: - checkout - - setup_riot - setup_hatch - run: name: "Spelling" diff --git a/hatch.toml b/hatch.toml index d1a730b7202..cd8f1b23fb1 100644 --- a/hatch.toml +++ b/hatch.toml @@ -83,3 +83,30 @@ build = [ env.CIRCLECI.type = [ { value = "virtual", if = ["true"] }, ] + + +[envs.slotscheck] +template = "slotscheck" +python = "3.10" +features = ["opentracing"] +extra-dependencies = [ + "slotscheck==0.17.0", +] + +[envs.slotscheck.scripts] +_ = [ + "python -m slotscheck -v ddtrace/", +] + + +[envs.scripts] +detached = true +python = "3.10" +extra-dependencies = [ + "packaging==23.1", +] + +[envs.scripts.scripts] +test = [ + "python -m doctest {args} scripts/get-target-milestone.py scripts/needs_testrun.py tests/suitespec.py", +] diff --git a/riotfile.py b/riotfile.py index aaec36f334e..763b066b944 100644 --- a/riotfile.py +++ b/riotfile.py @@ -103,16 +103,6 @@ def select_pys(min_version=MIN_PYTHON_VERSION, max_version=MAX_PYTHON_VERSION): "DD_CIVISIBILITY_ITR_ENABLED": "1", }, venvs=[ - Venv( - pys=["3"], - pkgs={"slotscheck": latest}, - venvs=[ - Venv( - name="slotscheck", - command="python -m slotscheck -v ddtrace/", - ), - ], - ), Venv( pys=["3"], name="scripts", diff --git a/scripts/gen_circleci_config.py b/scripts/gen_circleci_config.py index b39aab62327..b825cee55e9 100644 --- a/scripts/gen_circleci_config.py +++ b/scripts/gen_circleci_config.py @@ -71,12 +71,12 @@ def check(name: str, command: str, paths: t.Set[str]) -> None: ) check( name="Slots check", - command="riot -v run slotscheck", + command="hatch run slotscheck:_", paths={"ddtrace/*.py", "hatch.toml"}, ) check( name="Run scripts/*.py tests", - command="riot -v run -s scripts", + command="hatch run scripts:test", paths={"scripts/*.py"}, ) check( From 1b5f01a85fb503910212436c1baf4645711517b4 Mon Sep 17 00:00:00 2001 From: Romain Komorn <136473744+romainkomorndatadog@users.noreply.github.com> Date: Mon, 14 Aug 2023 13:10:44 +0100 Subject: [PATCH 3/3] chore(ci): add meta-testing to hatch (#6655) `riot` was removed from the pre-check CircleCI setup by https://github.com/DataDog/dd-trace-py/pull/6640/files#diff-8166c31d8ba62357b4a7cac4407d8744e60cf6b6ba32b09af939807db4c8f490L246 . This updates config to run the `meta-testing` check via `hatch` instead of via `riot`. ## Checklist - [x] Change(s) are motivated and described in the PR description. - [x] Testing strategy is described if automated tests are not included in the PR. - [x] Risk is outlined (performance impact, potential for breakage, maintainability, etc). - [x] Change is maintainable (easy to change, telemetry, documentation). - [x] [Library release note guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html) are followed. If no release note is required, add label `changelog/no-changelog`. - [x] Documentation is included (in-code, generated user docs, [public corp docs](https://github.com/DataDog/documentation/)). - [x] Backport labels are set (if [applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)) ## Reviewer Checklist - [x] Title is accurate. - [x] No unnecessary changes are introduced. - [x] Description motivates each change. - [x] Avoids breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes unless absolutely necessary. - [x] Testing strategy adequately addresses listed risk(s). - [x] Change is maintainable (easy to change, telemetry, documentation). - [x] Release note makes sense to a user of the library. - [x] Reviewer has explicitly acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment. - [x] 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) --- hatch.toml | 11 +++++++++++ scripts/gen_circleci_config.py | 2 +- 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/hatch.toml b/hatch.toml index cd8f1b23fb1..37bc88fc7ba 100644 --- a/hatch.toml +++ b/hatch.toml @@ -110,3 +110,14 @@ extra-dependencies = [ test = [ "python -m doctest {args} scripts/get-target-milestone.py scripts/needs_testrun.py tests/suitespec.py", ] + +[envs.meta-testing] +extra-dependencies = [ + "pytest", + "pytest-cov", + "hypothesis<6.45.1" +] +[envs.meta-testing.scripts] +meta-testing = [ + "pytest {args} tests/meta" +] diff --git a/scripts/gen_circleci_config.py b/scripts/gen_circleci_config.py index b825cee55e9..00f929a8010 100644 --- a/scripts/gen_circleci_config.py +++ b/scripts/gen_circleci_config.py @@ -81,7 +81,7 @@ def check(name: str, command: str, paths: t.Set[str]) -> None: ) check( name="Run conftest tests", - command="riot -v run meta-testing", + command="hatch run meta-testing:meta-testing", paths={"tests/*conftest.py", "tests/meta/*"}, ) check(