Skip to content

refactor(kafka-connect): improve request logging and dry-run handling - #652

Open
disrupted wants to merge 26 commits into
mainfrom
refactor-kafka-connect
Open

refactor(kafka-connect): improve request logging and dry-run handling#652
disrupted wants to merge 26 commits into
mainfrom
refactor-kafka-connect

Conversation

@disrupted

@disrupted disrupted commented Jul 29, 2026

Copy link
Copy Markdown
Member

use httpx event hooks to clean up logging
unified KafkaConnect._request() method to handle both real execution and dry-run preview

@disrupted
disrupted changed the base branch from main to kafka-connect-reset July 29, 2026 07:32
@disrupted
disrupted marked this pull request as ready for review July 29, 2026 08:01
@tobbber
tobbber requested a review from Copilot July 29, 2026 12:31

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors the Kafka Connect REST client to use httpx event hooks for request/response debug logging, and unifies “real” vs “dry-run” behavior through a single request path. It also updates the KafkaConnectHandler and related tests to pass dry_run through to Kafka Connect operations consistently.

Changes:

  • Rename/refactor the Kafka Connect REST wrapper to KafkaConnect, adding a unified request() method with dry_run support.
  • Add httpx request/response event hooks to centralize debug logging.
  • Update handler logic and tests to propagate dry_run into create/update/pause/resume/stop/delete/reset flows.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
kpops/component_handlers/kafka_connect/kafka_connect_api.py Introduces KafkaConnect, central request() method, and httpx event-hook-based logging with dry_run support.
kpops/component_handlers/kafka_connect/kafka_connect_handler.py Refactors handler flow to use KafkaConnect methods and propagate dry_run across operations.
tests/component_handlers/kafka_connect/test_kafka_connect_api.py Updates API tests to use pytest_httpx request inspection and adds logging/dry-run behavior tests.
tests/component_handlers/kafka_connect/test_kafka_connect_handler.py Updates handler tests to reflect new dry_run plumbing and reset/delete behavior.
Comments suppressed due to low confidence (4)

kpops/component_handlers/kafka_connect/kafka_connect_api.py:65

  • _log_response() always calls await response.aread() and attempts JSON parsing, even when DEBUG logging is disabled. This forces buffering the full response body in memory for every request (including large responses) and can interfere with streaming usage. Guard the body read/logging behind log.isEnabledFor(logging.DEBUG) so normal INFO-level runs don't pay this cost.
    kpops/component_handlers/kafka_connect/kafka_connect_api.py:57
  • _log_request() decodes request.content unconditionally when it exists, even if DEBUG logging is disabled. This adds overhead on every request and can raise UnicodeDecodeError for non-UTF8 payloads. Consider guarding on log.isEnabledFor(logging.DEBUG) and decoding with errors="replace" (or falling back to repr(...)).
    tests/component_handlers/kafka_connect/test_kafka_connect_api.py:721
  • Same as above: this test collects all DEBUG records in caplog, which can break if unrelated DEBUG logs are emitted. Filter by record.name == "KafkaConnectAPI" to make the assertions deterministic.
    tests/component_handlers/kafka_connect/test_kafka_connect_api.py:632
  • The DEBUG log assertions collect all DEBUG records in caplog, which can make the test flaky if any other library emits DEBUG logs. Filter by the expected logger name (e.g. KafkaConnectAPI) so the test only asserts on KafkaConnect's hook output.

This issue also appears on line 719 of the same file.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Base automatically changed from kafka-connect-reset to main July 29, 2026 12:46
@disrupted
disrupted requested a review from tobbber July 29, 2026 13:17
@disrupted disrupted added the component/handlers Related to component handlers label Jul 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component/handlers Related to component handlers

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants