Agnosticize integrations HTTP error handling (batch 3 of 3)#24548
Conversation
|
0c5647f to
8d6f525
Compare
8677a25 to
633eb98
Compare
NouemanKHAL
left a comment
There was a problem hiding this comment.
LGTM for the code!
Blocker: changelog entries should either by a .added or omitted in this PR and only generated when merging to master as this is a dev change, we only want a changelog for the user-facing changes (when merging to master)
We'd also probably want ti have a single changelog entry for a migration to an agnostic HTTP layer, for now I see different changelog entries such as:
Migrate off requests-specific exceptions to the HTTP-agnostic exception library.Use library-agnostic HTTP exceptions when handling API request failures.Use library-agnostic HTTP exceptions when handling request timeouts.Use library-agnostic HTTP exceptions when a token request returns an unexpected status.
Which are a bit too specific for a user-facing changelog.
Narrow self.http catch sites to backend-agnostic http_exceptions and swap interception-path tests to the agnostic surface for: mesos_slave, n8n, nifi, nutanix, octopus_deploy. Keeps the existing n8n/nutanix changelog entries already present on the base.
Narrow self.http catch sites to backend-agnostic http_exceptions and swap interception-path tests to the agnostic surface for: proxmox, rabbitmq, sonarqube, sonatype_nexus, spark, squid, teamcity, traefik_mesh, yarn, and complete prefect.
…C review) The agnostic ResponseWrapper.json() raises a stdlib json.JSONDecodeError (ValueError), not an HTTPError, so a 200 with a non-JSON body escaped `except HTTPError` in check() and skipped the connect service check that master emitted (master caught it via the broad RequestException). Also catch json.JSONDecodeError.
Address non-blocking review nits on the new nifi test helpers: - Drop the leading underscore from the module-level helpers introduced in this PR (_dispatch, _mock_http_responses -> dispatch, mock_http_responses) per the repo naming convention for test-file helpers. Pre-existing underscore helpers are left untouched (out of scope). - Route test_overlapping_process_groups_no_duplicates through mock_http_responses instead of a hand-rolled side_effect, matching its siblings. Behavior is identical (same URL-substring dispatch).
db61d14 to
0d15c84
Compare
This PR merges into a feature branch, not master, so changelog entries aren't needed until the feature branch itself ships. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
cffde7e
into
mwdd146980/http-agnostic-protocol-surface
Validation ReportAll 21 validations passed. Show details
|
…24516) * feat(base): add agnostic HTTP protocol capabilities and auth/factory seam Freeze the backend-neutral HTTP surface in http_protocol.py and implement it on RequestsWrapper so consumers depend on the protocol, not requests: - Add connection close(), get_cookie(), and a trust_env property to HTTPClientProtocol/RequestsWrapper (capabilities expose behavior, never a backend object). - Add an agnostic auth hook (HTTPRequestAuth + OutgoingRequest) with a requests.auth.AuthBase adapter, so checks contribute headers/params without a requests-family auth type. - Add an overridable AgentCheck.create_http_wrapper() factory seam behind self.http, so the backend is swappable without touching consumers. Backend stays requests; changes are additive. Foundation for the httpx2 backend. * fix(base): harden get_cookie and annotate agnostic auth adapter Address code-review findings on the agnostic HTTP surface: - get_cookie: return default without eagerly building a session when none exists yet, and catch requests CookieConflictError so the documented value-or-default contract holds even with duplicate cookie names; fix the docstring. - Annotate the new _OutgoingRequest and _RequestsAuthHookAdapter helpers, matching the typed capabilities and protocols added in the same change. - Add regression tests: idempotent close after open, no-eager-session on cookie lookup, and conflict-returns-default. * fix(base): address review findings on the agnostic auth adapter - Trim the _RequestsAuthHookAdapter docstring to describe actual behavior: header edits apply via the shared dict, params are merged into the URL query, and the URL is read-only (finding 1). - Document that only per-call auth= is adapted for HTTPRequestAuth hooks; a hook set persistently via options['auth'] is unsupported (finding 4). - Revert the get_cookie hardening back to the minimal RequestsCookieJar.get delegation; that robustness was out of scope for a behavior-preserving foundation PR (finding 3). Keeps the new-code type annotations (finding 5) and the idempotent-close test (finding 7). * fix(base): honor get_cookie contract and document url read-only in protocol Round-2 review follow-ups on the agnostic HTTP surface: - get_cookie: catch requests CookieConflictError and return default so the method honors its own documented value-or-default contract. The method is new and the frozen protocol declares that contract, and the exception is requests-specific with no httpx2 equivalent, so leaving it unhandled would diverge per backend. Adds a cross-domain collision regression test. - OutgoingRequest protocol: document that url is inspection-only and writes are not guaranteed to be honored, in the frozen contract file where a second backend author will read it. * fix(base): adopt injected session trust_env in RequestsWrapper When a session is injected via session=, initialize the tracked _trust_env from that session's trust_env instead of hard-coding True, so the reported trust_env matches the session it wraps. getattr guards sessions that do not expose the attribute (e.g. autospec mocks). Adds a regression test for the injected-session case. Addresses round-2 review finding 2. * refactor(base): rename OutgoingRequest to HTTPRequest and create_http_wrapper to create_http_client Rename the agnostic auth-hook request view OutgoingRequest (and its concrete _OutgoingRequest) to HTTPRequest/_HTTPRequest, and the factory seam AgentCheck.create_http_wrapper to create_http_client, aligning with the HTTPClientProtocol return type. Behavior unchanged. * style(base): fix ruff import-order and B018 after the rename ruff isort reordered the renamed http_protocol imports (HTTPRequest before HTTPRequestAuth) in http.py and test_auth_hook.py, and the double-close test's bare 'http.session' expression is now an assertion to satisfy B018. * docs(base): document get_cookie ambiguous-collision fallback in contract The HTTPClientProtocol.get_cookie contract said only value or default; the concrete RequestsWrapper returns default on an ambiguous same-name cookie (CookieConflictError) but the frozen backend-neutral protocol gave no signal that a second backend must do the same. Align the contract so a future backend implements the edge case identically. * refactor(base): drop the HTTPRequestAuth hook seam Remove HTTPRequest/HTTPRequestAuth from the protocol surface and the _HTTPRequest/_RequestsAuthHookAdapter plumbing plus its per-call dispatch, and delete the seam-only test. The seam had a single intended consumer (voltdb), which now sends credentials as query params without it, so the frozen surface should not carry the hook. Net diff of this PR no longer introduces it. * docs(base): drop stale auth-hook re-export comment * fix(base): address review findings on protocol capabilities Soften the frozen-surface claim to provisional, disambiguate the options_method comment, use collections.abc.Iterator, trim a restating comment, cover the trust_env getattr fallback, and merge a weak cookie assertion into its stronger sibling test. * refactor(base): rename protocol classes to HTTPClient/HTTPResponse Drop the Protocol suffix now that these are the canonical backend-neutral HTTP surface names shared by the requests and future httpx backends. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(base): move MockHTTPResponse to datadog_checks_dev MockHTTPResponse is test-support code that was living under datadog_checks/base/utils/, so it shipped in the base wheel bundled with the Agent. Move it next to its sibling MockResponse in datadog_checks_dev/datadog_checks/dev/http.py, which is dev-only and never bundled. Import the HTTPStatusError dependency lazily inside raise_for_status() so importing dev.http does not pull datadog_checks_base at module load, matching the existing lazy-import convention in the dev pytest plugin (datadog_checks_dev does not declare base as a dependency). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(base): declare response attrs and get_peer_cert on protocol * feat(base): add ResponseWrapper.get_peer_cert capability * feat(dev): enforce HTTPResponse surface on MockHTTPResponse * fix(dev): enforce guard on public protocol surface only MockHTTPResponse's __getattr__/__setattr__ rejected any name not on the HTTPResponse protocol, including leading-underscore names. When wrapped by wrapt's ResponseWrapper, wrapt forwards private attribute sets (e.g. _ResponseWrapper__default_chunk_size) to the wrapped object, and the guard raised AttributeError on those, breaking the request path. A protocol is a public contract, so only public names should be checked against it; underscore-prefixed names pass through. * fix(dev): add history attribute to mock response * fix(base): parse kubelet pod list via response.json instead of raw * fix(http_check): read peer cert via get_peer_cert capability Swaps the .raw.connection.sock.getpeercert access for the HTTPResponse.get_peer_cert protocol member, so the check no longer depends on the requests-specific raw urllib3 response. * chore: add changelog entries for HTTP response surface (#24516) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(base): extend HTTP client protocol surface Declare ignore_tls_warning and persist_connections as writable client attributes, add a should_bypass_proxy capability, and document the per-request persist option on the verb methods. These cover the client touchpoints (items 2-5 of the surface audit) integrations reach for today, so they can move onto the agnostic protocol without reaching into RequestsWrapper internals. RequestsWrapper already carried the two attributes as slots and consumed persist in _request; only should_bypass_proxy is new, delegating to the existing module-level helper. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * chore(base): tighten HTTPResponse protocol comments Shorten multi-line comments to single lines and drop backticks. * chore(dev): tighten MockHTTPResponse guard comment Shorten the comment to one line and drop the semicolon. * chore(base): tighten HTTPClient protocol comments Shorten multi-line comments to single lines and drop backticks. * style: fix import order flagged by ruff --fix Run ddev test --fmt across the PR's changed files. * chore: renumber changelog entries from 24516 to 22676 Entries land on master through feature branch #22676, so the changelog filenames must reference that PR number. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Agnosticize integrations HTTP error handling (batch 1 of 3) (#24546) * Agnosticize integrations batch A1 (catch-narrowing + tests) Narrow self.http catch sites to backend-agnostic http_exceptions and swap interception-path tests to the agnostic surface for: activemq_xml, ambari, arangodb, citrix_hypervisor, cloud_foundry_api, consul. * Agnosticize integrations batch A2 (catch-narrowing + tests) Same agnosticization for: control_m, couch, couchbase, druid, ecs_fargate, elastic, envoy. * Preserve error handling after agnostic exception narrowing (batch A review) Address review findings on #24546 where narrowed catch sites changed behavior versus master: - couchbase: the agnostic ResponseWrapper.json() raises a stdlib json.JSONDecodeError (ValueError), not an HTTPError, so a 200 with a non-JSON body escaped `except HTTPError` and aborted the run. Master caught it via the broad RequestException. Also catch json.JSONDecodeError at the three sync-gateway/query-monitoring/index-stats sites. - activemq_xml: ConnectTimeout translates to HTTPTimeoutError (a sibling of HTTPConnectionError), so connect timeouts escaped `except HTTPConnectionError` and defeated suppress_errors. Catch HTTPTimeoutError as well. - druid: add explicit parametrize ids to the can-connect failure test. * chore: drop changelog entries for feature-branch merge This PR merges into a feature branch, not master, so changelog entries aren't needed until the feature branch itself ships. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> * Agnosticize integrations HTTP error handling (batch 2 of 3) (#24547) * Agnosticize integrations batch B (catch-narrowing + tests) Narrow self.http catch sites to backend-agnostic http_exceptions and swap interception-path tests to the agnostic surface for: fly_io, gitlab, harbor, hdfs_datanode, hdfs_namenode, ibm_db2, kubelet, kyototycoon, mapreduce, marathon, marklogic, mesos_master. * Catch connect timeouts in marklogic after agnostic narrowing (batch B review) ConnectTimeout translates to HTTPTimeoutError, a sibling of HTTPConnectionError, so it escaped `except (HTTPStatusError, HTTPConnectionError)` and skipped the CRITICAL connect service check that master emitted (master caught it via requests ConnectionError). Add HTTPTimeoutError to the catch. * Rename kubelet node-metrics test to match agnostic exception (batch 2 review) The test body raises HTTPStatusError, not a requests HTTPError, so rename test_report_node_metrics_kubernetes1_18_requests_httperror to ..._httpstatuserror. Test-only, no behavior change. * chore: drop changelog entries for feature-branch merge This PR merges into a feature branch, not master, so changelog entries aren't needed until the feature branch itself ships. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> * Agnosticize integrations HTTP error handling (batch 3 of 3) (#24548) * Agnosticize integrations batch C1 (catch-narrowing + tests) Narrow self.http catch sites to backend-agnostic http_exceptions and swap interception-path tests to the agnostic surface for: mesos_slave, n8n, nifi, nutanix, octopus_deploy. Keeps the existing n8n/nutanix changelog entries already present on the base. * Agnosticize integrations batch C2 (catch-narrowing + tests) Narrow self.http catch sites to backend-agnostic http_exceptions and swap interception-path tests to the agnostic surface for: proxmox, rabbitmq, sonarqube, sonatype_nexus, spark, squid, teamcity, traefik_mesh, yarn, and complete prefect. * Preserve malformed-JSON handling in sonarqube after narrowing (batch C review) The agnostic ResponseWrapper.json() raises a stdlib json.JSONDecodeError (ValueError), not an HTTPError, so a 200 with a non-JSON body escaped `except HTTPError` in check() and skipped the connect service check that master emitted (master caught it via the broad RequestException). Also catch json.JSONDecodeError. * Tidy nifi test helpers after agnostic migration (batch 3 review) Address non-blocking review nits on the new nifi test helpers: - Drop the leading underscore from the module-level helpers introduced in this PR (_dispatch, _mock_http_responses -> dispatch, mock_http_responses) per the repo naming convention for test-file helpers. Pre-existing underscore helpers are left untouched (out of scope). - Route test_overlapping_process_groups_no_duplicates through mock_http_responses instead of a hand-rolled side_effect, matching its siblings. Behavior is identical (same URL-substring dispatch). * chore: drop changelog entries for feature-branch merge This PR merges into a feature branch, not master, so changelog entries aren't needed until the feature branch itself ships. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> * Migrate openstack_controller to the backend-neutral HTTP protocol (#24594) * fix(openstack_controller): use backend-neutral HTTP protocol for keystone session Build the keystoneauth Session from the backend-neutral HTTPClient protocol surface (verify, cert, headers) instead of handing keystoneauth the underlying requests.Session. This removes the last openstack_controller coupling to the requests backend while preserving TLS config and the Nova and Ironic microversion headers on the SDK data-plane connection. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * chore(openstack_controller): add changelog entry for 24594 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Delete openstack_controller/changelog.d/24594.fixed --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> * fix: repair HTTP symbol references broken by branch refactors Two stragglers from this branch's own HTTP refactors were left with stale references and broke CI: - MockHTTPResponse moved from datadog_checks.base.utils.http_testing to datadog_checks.dev.http; update the four test files still importing the old path (sonatype_nexus, nifi, traefik_mesh, nutanix). - HTTPResponseProtocol was renamed to HTTPResponse in http_protocol; update the two production files still referencing the old name (nutanix at runtime, control_m under TYPE_CHECKING). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(http): address self-review gaps in agnostic surface Close protocol-compliance and hardening gaps surfaced by the automated self review, and finish decoupling http_check from requests internals. - Wrap ResponseWrapper.history so redirect items satisfy HTTPResponse and translate exceptions, instead of leaking raw requests objects. - Guard get_peer_cert against bare non-TLS sockets that lack getpeercert, so plain http:// requests return None rather than raise. - Route http_check through trust_env and close() instead of reaching into the raw session, removing its last requests-specific internals. - Drop the leading underscore from CaseInsensitiveDict and prune the redundant dunders from protocol_members(). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * chore(http): drop changelog entries until master-merge prep This PR merges into the httpx-migration-base feature branch, not master, so per-integration changelog entries are premature. Remove the entries added here and restore the base-branch entries this branch had deleted, leaving the PR changelog-neutral. * Migrate remaining integrations to the backend-neutral HTTP protocol (#24597) * fix(http): migrate remaining consumers to backend-neutral HTTP protocol Replace the last direct reaches into the requests Session across four integrations with the backend-neutral HTTPClient protocol: - avi_vantage: http.close() and http.get_cookie() - cisco_aci: http.close() - hpe_aruba_edgeconnect: http.get_cookie() and http.set_header() - http_check: http.trust_env setter and http.close() Behavior is preserved; only the access path changes. Updates the hpe_aruba_edgeconnect unit tests to drive the protocol methods. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> * Decouple integration tests from the requests library (#24602) * test(crio): use mock_http_response fixture * test(flink): use mock_http_response fixture * test(nginx_ingress_controller): use mock_http_response fixture * test(nginx): drive vts unit test through mock_http seam * test(silk): unit-test error_msg path via mock_http_response Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(http_check): suppress network via mock_http_response Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(nvidia_nim): drive unit test through mock_http seam * test(dell_powerflex): migrate fixtures to mock_http seam conftest.py's mock_http_call, mock_auth, and mock_http_get fixtures patched requests.Session directly, coupling tests to the requests backend. Route them through the auto-available mock_http fixture and agnostic MockHTTPResponse/HTTPStatusError instead. * test(dell_powerflex): use agnostic exceptions in unit tests Five tests patched requests.Session.get/post directly and imported requests.exceptions. Swap them onto the mock_http fixture and the agnostic http_exceptions module so the suite no longer depends on requests. --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> * Narrow HTTP exception catches in kube_* health checks (#24604) * fix(kube): catch agnostic HTTP exceptions in health checks * test(kube): cover non-status HTTP subtype in health checks The health-check catches were widened to the base agnostic HTTPError, but tests only raised HTTPStatusError. Add an HTTPConnectionError case per check so a non-status subtype is verified to still map to CRITICAL, pinning the widened contract against an accidental narrowing back to HTTPStatusError. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Add an HTTP client factory seam and route wrapper construction through it (#24622) * feat(base): add create_http_client factory seam Introduce a module-level create_http_client(instance, init_config, remapper, logger) in base.utils.http as the single place that names the HTTP backend, and have AgentCheck.create_http_client delegate to it with an optional instance override. This lets callers that build a client from a config other than the check's own instance (per-endpoint scraper configs, bespoke health handlers) share one seam, and lets non-check contexts reuse the module function directly. Callers keep the agnostic HTTPClient protocol as the return type, so the concrete backend stays confined to this factory. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(base): route mixin http handlers through the factory The OpenMetrics and Prometheus v1 scraper mixins built their per-endpoint RequestsWrapper directly. Route them through self.create_http_client so the framework has a single construction seam, and drop the now-unused RequestsWrapper imports. Behavior is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(http): route integration wrapper construction through the factory argocd, hpe_aruba_edgeconnect, and the five kube_* checks built their own RequestsWrapper for a per-endpoint or health handler. Route each through create_http_client (self.create_http_client for checks, the check's own for the argocd resources helper) and drop the RequestsWrapper imports. The kube_* conftests patched the removed RequestsWrapper symbol. Since the metrics path is already intercepted at get_http_handler, the health handler is create_http_client's only live caller under those fixtures, so retarget the patch to AgentCheck.create_http_client. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * style(base): condense create_http_client docstring to one line Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(base): pin factory contract and fix review nits Address review feedback on the create_http_client seam: - Type the module function's instance param as dict | None, matching the delegating method that can pass self.instance (None) into the or {} guard. - Soften the module docstring, which overclaimed a single repo-wide seam while vsphere still constructs a wrapper directly. - Fix the override example in test_factory_override_swaps_client to keep the instance parameter, so it does not model a Liskov-incompatible override. - Add a test that get_http_handler routes through create_http_client, pinning the new coupling that overriding the factory now governs the scraper handler. - Add a test that an explicit empty-dict instance override is honored and does not fall back to the check's own instance. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(base): route mixin handlers through the module-level factory The OpenMetrics/Prometheus mixins called self.create_http_client, which only exists on AgentCheck. PrometheusScraper composes PrometheusScraperMixin without being an AgentCheck (it holds a check reference and uses its own init_config/HTTP_CONFIG_REMAPPER/log), so the mixin raised AttributeError and the Prometheus integration test failed. Route both mixins through the module-level create_http_client with explicit config, matching the original RequestsWrapper arguments exactly. This is backend-agnostic and does not assume the mixin is mixed into a check. Update the coverage test to pin the module-level seam instead of the method. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: NouemanKHAL <noueman.khalikine@datadoghq.com>
What does this PR do?
Removes backend-specific
requestsreferences from the final set of integrations in this sweep, leaving each on the HTTP-agnostic surface only. Per integration it narrowsself.httpcatch sites todatadog_checks.base.utils.http_exceptions.*(no dual-catch), swaps barefrom requests import ...to agnostic names (includingrequests.Responsetype hints toHTTPResponseProtocol), drops now-unusedrequestsimports, and updates the interception-path tests (agnostic exceptions,MockHTTPResponse, and themock_http/mock_openmetrics_httpfixtures).Integrations covered (batch 3 of 3): mesos_slave, n8n, nifi, nutanix, octopus_deploy, prefect, proxmox, rabbitmq, sonarqube, sonatype_nexus, spark, squid, teamcity, traefik_mesh, yarn.
The backend stays
requests. Behavior is unchanged. This PR is based directly on #24516 for now. When #24516 is approved, it will be merged into #22676, and then this PR will be stacked on #22676.Motivation
Checks and their tests should depend only on the agnostic HTTP surface (
http_exceptions.*), never onrequests-specific APIs, so the backend can later be swapped without touching consumers.Verification
requestsimports/attribute use.http_exceptions.*. No(RequestException, HTTPError)dual-catch remains.ddev test --lintis green across the touched integrations.Review checklist (to be filled by reviewers)
qa/requiredif this PR needs QA validation, orqa/skip-qaif it does not. Exactly one of the two is required.backport/<branch-name>label to the PR and it will automatically open a backport PR once this one is merged