Skip to content

Commit

Permalink
chore(ci-visibility): capture timeout errors (#6352)
Browse files Browse the repository at this point in the history
CI Visibility: Capture timeout errors in requests to the backend

## 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)
  • Loading branch information
gnufede authored Jul 16, 2023
1 parent 37b05ab commit f7ffe6d
Show file tree
Hide file tree
Showing 3 changed files with 56 additions and 3 deletions.
17 changes: 14 additions & 3 deletions ddtrace/internal/ci_visibility/recorder.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
from ddtrace.internal.agent import get_trace_url
from ddtrace.internal.ci_visibility.filters import TraceCiVisibilityFilter
from ddtrace.internal.compat import JSONDecodeError
from ddtrace.internal.compat import TimeoutError
from ddtrace.internal.compat import parse
from ddtrace.internal.logger import get_logger
from ddtrace.internal.service import Service
Expand All @@ -41,6 +42,7 @@
log = get_logger(__name__)

TEST_SKIPPING_LEVEL = "suite"
DEFAULT_TIMEOUT = 15


def _extract_repository_name_from_url(repository_url):
Expand All @@ -61,7 +63,7 @@ def _get_git_repo():
def _do_request(method, url, payload, headers):
# type: (str, str, str, Dict) -> Response
try:
conn = get_connection(url)
conn = get_connection(url, timeout=DEFAULT_TIMEOUT)
log.debug("Sending request: %s %s %s %s", method, url, payload, headers)
conn.request("POST", url, payload, headers)
resp = compat.get_connection_response(conn)
Expand Down Expand Up @@ -172,7 +174,11 @@ def _check_enabled_features(self):
},
}
}
response = _do_request("POST", url, json.dumps(payload), _headers)
try:
response = _do_request("POST", url, json.dumps(payload), _headers)
except TimeoutError:
log.warning("Request timeout while fetching enabled features")
return False, False
try:
parsed = json.loads(response.body)
except JSONDecodeError:
Expand Down Expand Up @@ -260,7 +266,12 @@ def _fetch_tests_to_skip(self):
log.warning("Cannot make requests to skippable endpoint if mode is not agentless or evp proxy")
return

response = _do_request("POST", url, json.dumps(payload), _headers)
try:
response = _do_request("POST", url, json.dumps(payload), _headers)
except TimeoutError:
log.warning("Request timeout while fetching skippable tests")
self._test_suites_to_skip = []
return

if response.status >= 400:
log.warning("Test skips request responded with status %d", response.status)
Expand Down
7 changes: 7 additions & 0 deletions ddtrace/internal/compat.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
"Queue",
"stringify",
"StringIO",
"TimeoutError",
"urlencode",
"parse",
"reraise",
Expand All @@ -45,6 +46,12 @@
PY2 = sys.version_info[0] == 2
PY3 = sys.version_info[0] == 3

try:
from builtin import TimeoutError
except ImportError:
# Purposely shadowing a missing python builtin
from socket import timeout as TimeoutError # noqa: A001

if not PY2:
long = int

Expand Down
35 changes: 35 additions & 0 deletions tests/ci_visibility/test_ci_visibility.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
from ddtrace.internal.ci_visibility.git_client import CIVisibilityGitClient
from ddtrace.internal.ci_visibility.git_client import CIVisibilityGitClientSerializerV1
from ddtrace.internal.ci_visibility.recorder import _extract_repository_name_from_url
from ddtrace.internal.compat import TimeoutError
from ddtrace.internal.utils.http import Response
from ddtrace.span import Span
from tests.utils import DummyCIVisibilityWriter
Expand Down Expand Up @@ -99,6 +100,40 @@ def test_ci_visibility_service_enable_with_app_key_and_itr_disabled(_do_request)
CIVisibility.disable()


@mock.patch("ddtrace.internal.ci_visibility.recorder._do_request", side_effect=TimeoutError)
def test_ci_visibility_service_settings_timeout(_do_request):
with override_env(
dict(
DD_API_KEY="foobar.baz",
DD_APP_KEY="foobar",
DD_CIVISIBILITY_AGENTLESS_ENABLED="1",
DD_CIVISIBILITY_ITR_ENABLED="1",
)
):
ddtrace.internal.ci_visibility.recorder.ddconfig = ddtrace.settings.Config()
CIVisibility.enable(service="test-service")
assert CIVisibility._instance._code_coverage_enabled_by_api is False
assert CIVisibility._instance._test_skipping_enabled_by_api is False
CIVisibility.disable()


@mock.patch("ddtrace.internal.ci_visibility.recorder.CIVisibility._check_enabled_features", return_value=(True, True))
@mock.patch("ddtrace.internal.ci_visibility.recorder._do_request", side_effect=TimeoutError)
def test_ci_visibility_service_skippable_timeout(_do_request, _check_enabled_features):
with override_env(
dict(
DD_API_KEY="foobar.baz",
DD_APP_KEY="foobar",
DD_CIVISIBILITY_AGENTLESS_ENABLED="1",
DD_CIVISIBILITY_ITR_ENABLED="1",
)
):
ddtrace.internal.ci_visibility.recorder.ddconfig = ddtrace.settings.Config()
CIVisibility.enable(service="test-service")
assert CIVisibility._instance._test_suites_to_skip == []
CIVisibility.disable()


@mock.patch("ddtrace.internal.ci_visibility.recorder._do_request")
def test_ci_visibility_service_enable_with_app_key_and_itr_enabled(_do_request):
with override_env(
Expand Down

0 comments on commit f7ffe6d

Please sign in to comment.