feat: unify error handling and logging - #657
Conversation
There was a problem hiding this comment.
Pull request overview
This PR migrates KPOps’ logging and expected-error handling to a unified, structured approach (via structlog), aiming to produce more helpful, service-scoped error output—especially for external-service failures (Kafka REST Proxy, Kafka Connect, Helm).
Changes:
- Introduces centralized structured logging utilities (
kpops/utils/logging.py) and migrates many modules fromloggingtostructlog. - Adds/standardizes external-service exception types (connection vs HTTP response errors) and updates Kafka REST Proxy / Kafka Connect / Helm wrappers accordingly.
- Updates and adds tests to assert structured log events (using
structlog.testing.capture_logs) and adjusts CLI entrypoint to consistently logKpopsExceptions and exit with code 1.
Reviewed changes
Copilot reviewed 68 out of 70 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| uv.lock | Adds structlog to the resolved lockfile. |
| pyproject.toml | Adds structlog dependency; switches console script entrypoint to kpops.cli.main:cli. |
| kpops/utils/logging.py | New central structlog configuration + exception logging helpers + context binding. |
| kpops/core/exception.py | Introduces KpopsException base with logged flag + ServiceException. |
| kpops/component_handlers/utils/exception.py | Adds reusable HTTP-response and connection error base classes for services. |
| kpops/component_handlers/topic/init.py | Adds Kafka REST Proxy service name constant. |
| kpops/component_handlers/topic/exception.py | Refactors topic/Kafka REST errors into service-scoped exception hierarchy. |
| kpops/component_handlers/topic/kafka_rest.py | Adds request/response debug hooks, transport→connection errors, structured logs, bound context. |
| kpops/component_handlers/topic/handler.py | Updates topic handler logging to structured fields (topic_name, counts, etc.). |
| kpops/component_handlers/kafka_connect/init.py | Adds Kafka Connect service name constant. |
| kpops/component_handlers/kafka_connect/exception.py | Refactors Kafka Connect errors into service-scoped exception hierarchy. |
| kpops/component_handlers/kafka_connect/kafka_connect_api.py | Adds structured request/response debug logs, transport→connection errors, bound context. |
| kpops/component_handlers/kafka_connect/kafka_connect_handler.py | Migrates handler logging to structured fields and separates “diff header” vs diff body logs. |
| kpops/component_handlers/kafka_connect/timeout.py | Migrates timeout logging to structured form. |
| kpops/component_handlers/helm_wrapper/init.py | Adds Helm service name constant. |
| kpops/component_handlers/helm_wrapper/exception.py | Introduces Helm service exception types (HelmError, etc.). |
| kpops/component_handlers/helm_wrapper/helm.py | Migrates to structlog; wraps operations with bound context; uses HelmError for stderr parsing. |
| kpops/component_handlers/helm_wrapper/helm_diff.py | Migrates HelmDiff logger type to structlog logger and structlog logging. |
| kpops/component_handlers/helm_wrapper/dry_run_handler.py | Migrates dry-run diff logging to structured fields. |
| kpops/component_handlers/kubernetes/utils.py | Migrates K8s utils logging to structured fields; downgrades critical→warning. |
| kpops/component_handlers/kubernetes/pvc_handler.py | Migrates PVC handler logging to structured fields (namespace/app/pvc_names). |
| kpops/component_handlers/schema_handler/schema_handler.py | Migrates schema handler logging to structured fields; refactors message formatting. |
| kpops/components/base_components/kafka_app.py | Migrates base component logger to structlog. |
| kpops/components/base_components/kafka_connector.py | Migrates base component logger to structlog. |
| kpops/components/base_components/kubernetes_app.py | Migrates base component logger to structlog. |
| kpops/components/base_components/helm_app.py | Migrates base component logger to structlog; structured Helm release existence logs. |
| kpops/components/base_components/base_defaults_component.py | Migrates defaults enrichment logs to structured fields. |
| kpops/components/streams_bootstrap/base.py | Migrates streams bootstrap logs to structured fields (component_name, release, etc.). |
| kpops/components/streams_bootstrap/streams/streams_app.py | Migrates validation fallback warning/debug to structured fields. |
| kpops/components/streams_bootstrap/producer/producer_app.py | Migrates validation fallback warning/debug to structured fields. |
| kpops/components/streams_bootstrap/consumer/consumer_app.py | Migrates validation fallback warning/debug to structured fields. |
| kpops/components/streams_bootstrap_v2/base.py | Migrates streams bootstrap v2 logs to structured fields. |
| kpops/components/streams_bootstrap_v2/streams/streams_app.py | Migrates validation fallback warning/debug to structured fields. |
| kpops/components/streams_bootstrap_v2/producer/producer_app.py | Migrates validation fallback warning/debug to structured fields. |
| kpops/utils/yaml.py | Migrates YAML loader debug log to structured fields. |
| kpops/utils/pydantic.py | Switches settings source logger type to structlog logger. |
| kpops/utils/gen_schema.py | Migrates schema generation logs to structured fields. |
| kpops/utils/docstring.py | Removes unused stdlib logger from docstring helpers. |
| kpops/pipeline/init.py | Migrates pipeline generator logger to structlog. |
| kpops/core/registry.py | Migrates registry debug logs to structured fields; removes typer.style formatting usage. |
| kpops/config/init.py | Migrates config logger to structlog. |
| kpops/api/logs.py | Removes legacy stdlib logging formatter/config module. |
| kpops/api/init.py | Uses centralized logging helpers; adds _run_component() wrapper that logs KpopsExceptions. |
| kpops/cli/main.py | Adds cli() entrypoint to catch/log KpopsExceptions and exit(1). |
| hooks/gen_docs/gen_docs_components.py | Migrates doc generation hook logging to structured fields. |
| tests/conftest.py | Switches test logging setup import; adjusts test component copying to allow existing dirs. |
| tests/utils/test_pydantic.py | Updates import path for SerializeAsOptional helpers. |
| tests/utils/test_logging.py | New tests for log_kpops_exception behavior and service-scoped logger naming. |
| tests/api/test_run_component.py | New tests for _run_component() exception logging and contextvars isolation. |
| tests/cli/test_main.py | New tests for CLI exit code and double-logging avoidance. |
| tests/pipeline/test_generate.py | Updates assertions to validate structured log events for pipeline selection/filtering. |
| tests/kubernetes/test_pvc_handler.py | Updates assertions to validate structured PVC deletion logs. |
| tests/components/test_kubernetes_app.py | Removes old log.info patch fixture (no longer used). |
| tests/components/test_kafka_sink_connector.py | Removes old log.info patch fixture (no longer used). |
| tests/components/test_helm_app.py | Updates destroy/reset/clean tests to assert structured logs. |
| tests/components/streams_bootstrap/test_streams_app.py | Updates dry-run and validation fallback tests to assert structured logs. |
| tests/components/streams_bootstrap/test_producer_app.py | Updates validation fallback tests + dry-run diff call assertions. |
| tests/components/streams_bootstrap/test_consumer_app.py | Updates dry-run and validation fallback tests to assert structured logs. |
| tests/components/streams_bootstrap_v2/test_streams_app.py | Updates dry-run and validation fallback tests to assert structured logs. |
| tests/components/streams_bootstrap_v2/test_producer_app.py | Updates validation fallback tests + dry-run diff call assertions. |
| tests/component_handlers/utils/test_exception.py | New tests for shared service exception base behavior (reason extraction, body parsing, etc.). |
| tests/component_handlers/utils/init.py | Package marker for utils tests. |
| tests/component_handlers/topic/test_topic_handler.py | Replaces patched stdlib logger calls with structured-log assertions. |
| tests/component_handlers/topic/test_kafka_rest.py | Updates to pytest_httpx2 request inspection; adds connection/umbrella exception tests; asserts structured logs. |
| tests/component_handlers/schema_handler/test_schema_handler.py | Replaces patched logger assertions with structured-log assertions; updates warning content/fields. |
| tests/component_handlers/kafka_connect/test_kafka_connect_handler.py | Replaces patched logger assertions with structured-log assertions; verifies structured “diff header/body” logging. |
| tests/component_handlers/kafka_connect/test_kafka_connect_api.py | Replaces caplog assertions with structured-log assertions; adds connection/umbrella exception tests. |
| tests/component_handlers/helm_wrapper/test_helm_wrapper.py | Updates exception class expectations; asserts structured warning logs for release-not-found. |
| tests/component_handlers/helm_wrapper/test_helm_diff.py | Migrates helm diff log assertion to structlog capture. |
| tests/component_handlers/helm_wrapper/test_dry_run_handler.py | Migrates dry-run handler tests to structlog capture + structured event assertions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| tokens = structlog.contextvars.bind_contextvars(**kw) | ||
| yield | ||
| structlog.contextvars.reset_contextvars(**tokens) |
There was a problem hiding this comment.
that's wrong. when an exception is raised, it propagates out of the generator at the yield expression. the reset (after yield) only runs for the success path, not the exception path. therefore, the context remains bound after the exception
There was a problem hiding this comment.
@disrupted yeah you're right. But that also means the context stays attached for exceptions that we recover from, right? (for example except TopicNotFoundException in __execute_topic_creation)
Do you think this could lead to context becoming cluttered and stale?
There was a problem hiding this comment.
@tobbber good point, the context could indeed leak into later exceptions. to fix it, contextvars is now always reset in the finally block. error context is stored directly inside KpopsException as dict.
close #157
before
after