Conversation
github-actions
Bot
added
Ingestion
safe to test
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| return json.loads(content) | ||
| except json.JSONDecodeError as e: | ||
| logger.warning(f"Failed to parse as JSON despite content-type: {e}") | ||
| logger.error(f"Failed to parse as JSON despite content-type: {e}") |
There was a problem hiding this comment.
⚠️ Quality: Blanket warning→error promotion misclassifies non-critical failures
This PR promotes all logger.warning() calls to logger.error() without distinguishing between genuinely erroneous conditions and expected/graceful degradation scenarios. In many logging frameworks and monitoring setups, ERROR level triggers alerts and pages on-call engineers, while WARNING is for recoverable or expected-but-unusual situations.
Several of these changes are semantically incorrect:
-
parse_openapi_schema(parser.py:57,62) — When JSON parsing fails here, the code falls through to a generic JSON→YAML fallback (lines 64-80). This is normal control flow, not an error. Logging at ERROR on a successful fallback path will generate false alarm noise. -
test_server_connection(connection.py:146) — This is a test function designed to returnTrue/False. A failed connection is an expected outcome, not an error. (Also note the inconsistency:McpProtocolErroris nowerrorwhile the genericExceptionon line 149 stayswarning.) -
_test_connection_steps_automation_workflow(test_connections.py:138) — Test connection steps are expected to fail sometimes; the result is reported back to the user viaTestConnectionStepResult. Logging each failure as ERROR is misleading. -
find_database_service_by_hostname/find_messaging_service_by_brokers(kafkaconnect/metadata.py:234,296) — These returnNoneon failure and callers handle the absence gracefully. A missing service match is a warning, not an error. -
get_aws_bucket_region(s3/metadata.py:760) — Falls back to the configured region on line 761. This is graceful degradation.
Recommendation: Review each call site individually. Reserve logger.error() for conditions that indicate something is broken and needs attention (e.g., data corruption, unrecoverable API failures). Keep logger.warning() for expected-but-unusual situations where the code continues gracefully.
Suggested fix:
Audit each site individually. A helpful heuristic:
- ERROR: The operation failed AND the caller cannot recover
(e.g., data loss, broken pipeline)
- WARNING: The operation failed BUT the code degrades
gracefully (e.g., returns None, uses a fallback)
- DEBUG: Expected control-flow branches
Was this helpful? React with 👍 / 👎 | Reply gitar fix to apply this suggestion
There was a problem hiding this comment.
Pull request overview
Updates ingestion and data-quality modules to emit ERROR logs (instead of WARNING) for a variety of caught exceptions, aiming to improve error visibility in logs.
Changes:
- Switched many exception-path log statements from
logger.warning(...)tologger.error(...)across ingestion sources and utilities. - Switched multiple data-quality validation abort logs from warning to error.
Reviewed changes
Copilot reviewed 56 out of 56 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| ingestion/src/metadata/pii/scanners/ner_scanner.py | Promotes processing-exception logs to ERROR in the NER PII scanner. |
| ingestion/src/metadata/ingestion/source/storage/s3/metadata.py | Promotes S3 bucket region lookup failure logging to ERROR. |
| ingestion/src/metadata/ingestion/source/search/opensearch/parser.py | Promotes OpenSearch mapping parse failure logging to ERROR. |
| ingestion/src/metadata/ingestion/source/search/elasticsearch/parser.py | Promotes Elasticsearch mapping parse failure logging to ERROR. |
| ingestion/src/metadata/ingestion/source/pipeline/spline/utils.py | Promotes Spline parsing failure logging to ERROR. |
| ingestion/src/metadata/ingestion/source/pipeline/spline/metadata.py | Promotes datasource parsing failure logging to ERROR. |
| ingestion/src/metadata/ingestion/source/pipeline/kafkaconnect/metadata.py | Promotes multiple KafkaConnect ingestion error logs to ERROR. |
| ingestion/src/metadata/ingestion/source/pipeline/kafkaconnect/client.py | Promotes KafkaConnect client exception logs to ERROR. |
| ingestion/src/metadata/ingestion/source/pipeline/gluepipeline/metadata.py | Promotes GluePipeline S3 script download failure logging to ERROR. |
| ingestion/src/metadata/ingestion/source/pipeline/domopipeline/metadata.py | Promotes DomoPipeline source URL retrieval failure logging to ERROR. |
| ingestion/src/metadata/ingestion/source/pipeline/dbtcloud/client.py | Promotes dbt Cloud client exception logs to ERROR. |
| ingestion/src/metadata/ingestion/source/pipeline/airflow/api/client.py | Promotes Airflow API JSON parse failure logging to ERROR. |
| ingestion/src/metadata/ingestion/source/pipeline/airbyte/metadata.py | Promotes Airbyte timestamp parsing failure logging to ERROR. |
| ingestion/src/metadata/ingestion/source/messaging/pubsub/metadata.py | Promotes PubSub metadata/schema parsing failure logs to ERROR. |
| ingestion/src/metadata/ingestion/source/messaging/common_broker_source.py | Promotes schema-with-references parsing failure logging to ERROR. |
| ingestion/src/metadata/ingestion/source/mcp/connection.py | Promotes MCP connection failure logging to ERROR. |
| ingestion/src/metadata/ingestion/source/mcp/client.py | Promotes MCP client parsing/sending failure logs to ERROR. |
| ingestion/src/metadata/ingestion/source/drive/sftp/metadata.py | Promotes SFTP sample/schema extraction failure logging to ERROR. |
| ingestion/src/metadata/ingestion/source/database/lineage_source.py | Promotes lineage queue processing failure logs to ERROR. |
| ingestion/src/metadata/ingestion/source/database/external_table_lineage_mixin.py | Promotes external table lineage yield failure logging to ERROR. |
| ingestion/src/metadata/ingestion/source/database/common_db_source.py | Promotes SQLAlchemy engine dispose failure logging to ERROR. |
| ingestion/src/metadata/ingestion/source/dashboard/superset/db_source.py | Promotes Superset chart list fetch failure logging to ERROR. |
| ingestion/src/metadata/ingestion/source/dashboard/qliksense/client.py | Promotes QlikSense client fetch failure logs to ERROR. |
| ingestion/src/metadata/ingestion/source/dashboard/qlikcloud/client.py | Promotes QlikCloud client fetch failure logs to ERROR. |
| ingestion/src/metadata/ingestion/source/dashboard/microstrategy/client.py | Promotes MicroStrategy client fetch failure logs to ERROR. |
| ingestion/src/metadata/ingestion/source/dashboard/metabase/client.py | Promotes Metabase client fetch failure logs to ERROR. |
| ingestion/src/metadata/ingestion/source/api/rest/parser.py | Promotes OpenAPI schema parse failure logs to ERROR. |
| ingestion/src/metadata/ingestion/ometa/mixins/container_mixin.py | Promotes container sample parsing failure logging to ERROR. |
| ingestion/src/metadata/ingestion/connections/test_connections.py | Promotes automation workflow step failure logging to ERROR. |
| ingestion/src/metadata/ingestion/bulksink/metadata_usage.py | Promotes metadata usage publish failure logging to ERROR. |
| ingestion/src/metadata/data_quality/validations/table/sqlalchemy/tableDiff.py | Promotes Data Diff unsupported dialect logging to ERROR. |
| ingestion/src/metadata/data_quality/validations/table/base/tableRowInsertedCountToBeBetween.py | Promotes validation abort logging to ERROR. |
| ingestion/src/metadata/data_quality/validations/table/base/tableRowCountToEqual.py | Promotes validation abort logging to ERROR. |
| ingestion/src/metadata/data_quality/validations/table/base/tableRowCountToBeBetween.py | Promotes validation abort logging to ERROR. |
| ingestion/src/metadata/data_quality/validations/table/base/tableCustomSQLQuery.py | Promotes validation abort logging to ERROR. |
| ingestion/src/metadata/data_quality/validations/table/base/tableColumnToMatchSet.py | Promotes validation abort logging to ERROR. |
| ingestion/src/metadata/data_quality/validations/table/base/tableColumnNameToExist.py | Promotes validation abort logging to ERROR. |
| ingestion/src/metadata/data_quality/validations/table/base/tableColumnCountToEqual.py | Promotes validation abort logging to ERROR. |
| ingestion/src/metadata/data_quality/validations/table/base/tableColumnCountToBeBetween.py | Promotes validation abort logging to ERROR. |
| ingestion/src/metadata/data_quality/validations/column/base/columnValuesToNotMatchRegex.py | Promotes validation abort logging to ERROR. |
| ingestion/src/metadata/data_quality/validations/column/base/columnValuesToMatchRegex.py | Promotes validation abort logging to ERROR. |
| ingestion/src/metadata/data_quality/validations/column/base/columnValuesToBeUnique.py | Promotes validation abort logging to ERROR. |
| ingestion/src/metadata/data_quality/validations/column/base/columnValuesToBeNotNull.py | Promotes validation abort logging to ERROR. |
| ingestion/src/metadata/data_quality/validations/column/base/columnValuesToBeNotInSet.py | Promotes validation abort logging to ERROR. |
| ingestion/src/metadata/data_quality/validations/column/base/columnValuesToBeInSet.py | Promotes validation abort logging to ERROR. |
| ingestion/src/metadata/data_quality/validations/column/base/columnValuesToBeBetween.py | Promotes validation abort logging to ERROR. |
| ingestion/src/metadata/data_quality/validations/column/base/columnValuesToBeAtExpectedLocation.py | Promotes validation abort logging to ERROR. |
| ingestion/src/metadata/data_quality/validations/column/base/columnValuesSumToBeBetween.py | Promotes validation abort logging to ERROR. |
| ingestion/src/metadata/data_quality/validations/column/base/columnValuesMissingCount.py | Promotes validation abort logging to ERROR. |
| ingestion/src/metadata/data_quality/validations/column/base/columnValueStdDevToBeBetween.py | Promotes validation abort logging to ERROR. |
| ingestion/src/metadata/data_quality/validations/column/base/columnValueMinToBeBetween.py | Promotes validation abort logging to ERROR. |
| ingestion/src/metadata/data_quality/validations/column/base/columnValueMedianToBeBetween.py | Promotes validation abort logging to ERROR. |
| ingestion/src/metadata/data_quality/validations/column/base/columnValueMeanToBeBetween.py | Promotes validation abort logging to ERROR. |
| ingestion/src/metadata/data_quality/validations/column/base/columnValueMaxToBeBetween.py | Promotes validation abort logging to ERROR. |
| ingestion/src/metadata/data_quality/validations/column/base/columnValueLengthsToBeBetween.py | Promotes validation abort logging to ERROR. |
| ingestion/src/metadata/clients/domo_client.py | Promotes Domo client exception logs to ERROR. |
| except Exception as exc: | ||
| logger.debug(traceback.format_exc()) | ||
| logger.warning(f"Failed to close the api sesison due to [{exc}]") | ||
| logger.error(f"Failed to close the api sesison due to [{exc}]") |
There was a problem hiding this comment.
Typo in log message: "sesison" should be "session".
| except Exception as exc: | ||
| logger.debug(traceback.format_exc()) | ||
| logger.warning(f"Unable to get run info :{exc}") | ||
| logger.error(f"Unable to get run info :{exc}") |
There was a problem hiding this comment.
Minor formatting issue in the error message: there is an extra space before the colon ("info :{exc}"). Consider changing it to a consistent "info: {exc}" format for readability/searchability.
| except Exception as exc: | ||
| logger.debug(traceback.format_exc()) | ||
| logger.warning(f"Unable to get connector plugins {exc}") | ||
| logger.error(f"Unable to get connector plugins {exc}") |
There was a problem hiding this comment.
There are two consecutive spaces before the exception in this message ("plugins {exc}"). Tightening the formatting helps keep logs consistent and easier to grep.
| logger.error(f"Unknown error while processing {row} - {exc}") | ||
| logger.debug(traceback.format_exc()) |
There was a problem hiding this comment.
This error log includes the raw sample value (row), which can contain PII. Since this is the PII scanner, emitting sample data at ERROR level risks leaking sensitive data into centralized logs. Consider redacting the value (e.g., log row index/length/hash) or logging a generic message and rely on exception/traceback for debugging.
| except Exception: | ||
| logger.debug(traceback.format_exc()) | ||
| logger.warning(f"Unable to get the region for bucket: {bucket_name}") | ||
| logger.error(f"Unable to get the region for bucket: {bucket_name}") | ||
| return region or self.service_connection.awsConfig.awsRegion |
There was a problem hiding this comment.
This except Exception: branch logs at ERROR level but drops the exception details (no as exc) and only logs the bucket name. To make the error actionable, capture the exception and include it (or use logger.exception(...) / logger.error(..., exc_info=True)).
| logger.debug(traceback.format_exc()) | ||
| logger.warning(f"Unable to parse the index properties: {exc}") | ||
| logger.error(f"Unable to parse the index properties: {exc}") | ||
|
|
There was a problem hiding this comment.
The traceback is logged at DEBUG while the message is now ERROR. If ERROR logs are what get collected/alerted on, the stack trace will be missing. Consider logging the exception with traceback at the same level (e.g., logger.exception(...) or logger.error(..., exc_info=True)) and avoid the separate traceback.format_exc() call.
| except Exception as err: | ||
| logger.debug(traceback.format_exc()) | ||
| logger.warning(f"Failed to fetch chart list due to - {err}]") | ||
| logger.error(f"Failed to fetch chart list due to - {err}]") |
There was a problem hiding this comment.
The log message ends with an extra closing bracket (]), which makes the error output look malformed. Remove the trailing bracket or ensure brackets are paired consistently.
🟡 Playwright Results — all passed (19 flaky)✅ 3955 passed · ❌ 0 failed · 🟡 19 flaky · ⏭️ 86 skipped
🟡 19 flaky test(s) (passed on retry)
How to debug locally# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip # view trace |
|
The Python checkstyle failed. Please run You can install the pre-commit hooks with |
Code Review
|
| Compact |
|
Was this helpful? React with 👍 / 👎 | Gitar
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 57 out of 57 changed files in this pull request and generated 7 comments.
Comments suppressed due to low confidence (1)
ingestion/src/metadata/ingestion/source/mcp/connection.py:140
- This method now logs
McpProtocolErroras ERROR, but the genericexcept Exceptionbranch still logs as WARNING. Given the PR’s intent (“move warning logs to error logs” for caught exceptions), consider making the unexpected-exception branchlogger.error/logger.exceptionas well so unexpected connection failures are consistently surfaced as errors (and include traceback if possible).
| logger.debug(traceback.format_exc()) | ||
| logger.warning(f"Failed to close the api sesison due to [{exc}]") | ||
| logger.error(f"Failed to close the api sesison due to [{exc}]") | ||
|
|
There was a problem hiding this comment.
Typo in log message: sesison should be session.
| try: | ||
| self.engine.dispose() | ||
| except Exception as exc: # pylint: disable=broad-except | ||
| logger.warning(f"Failed to dispose engine: {exc}") | ||
| logger.error(f"Failed to dispose engine: {exc}") | ||
| self.engine = None |
There was a problem hiding this comment.
_release_engine now logs disposal failures at ERROR but drops the traceback (logger.error(f"... {exc}")). For troubleshooting connection/pool cleanup issues, include exception context via logger.exception(...) or logger.error(..., exc_info=True) so the stack trace is available at error level.
| try: | ||
| self.process_data(row=row, entities_score=entities_score) | ||
| except Exception as exc: | ||
| logger.warning(f"Unknown error while processing {row} - {exc}") | ||
| logger.error(f"Unknown error while processing {row} - {exc}") | ||
| logger.debug(traceback.format_exc()) |
There was a problem hiding this comment.
The exception handling logs an ERROR but the stack trace is still only emitted at DEBUG via traceback.format_exc(), which likely defeats the purpose of “capture errors” in typical INFO/ERROR deployments. Prefer logger.exception(...) (or logger.error(..., exc_info=True)) here so the traceback is attached to the error-level log, and you can drop the separate debug traceback log to avoid duplication.
| except Exception as exc: | ||
| logger.warning(f"Failed to parse jdbc url: {exc}") | ||
| logger.error(f"Failed to parse jdbc url: {exc}") | ||
| logger.error(traceback.format_exc()) | ||
|
|
There was a problem hiding this comment.
Same as above for parse_jdbc_url: emitting traceback.format_exc() as a separate ERROR line is noisy and drops structured exception info. Prefer logger.exception(...) (or logger.error(..., exc_info=True)) and remove the extra traceback log line.
| with patch("metadata.ingestion.source.mcp.client.logger") as mock_logger: | ||
| transport.send_notification("notifications/initialized", {}) | ||
| mock_logger.warning.assert_called_once() | ||
| assert "server down" in str(mock_logger.warning.call_args) | ||
| mock_logger.error.assert_called_once() | ||
| assert "server down" in str(mock_logger.error.call_args) | ||
|
|
There was a problem hiding this comment.
The test docstring/comment still says send_notification should log warnings but the assertion now expects logger.error. Update the docstring/comment to match the new behavior so the test remains self-descriptive.
| def parse_dbfs_path(path: str) -> Optional[str]: | ||
| try: | ||
| return path.split("/")[-1] | ||
| except Exception as exc: | ||
| logger.warning(f"Failed to parse dbfs: {exc}") | ||
| logger.error(f"Failed to parse dbfs: {exc}") | ||
| logger.error(traceback.format_exc()) | ||
| return None |
There was a problem hiding this comment.
parse_dbfs_path logs the traceback by stringifying traceback.format_exc() as a second ERROR line. This loses structured exception context and produces duplicate log lines. Prefer logger.exception(...) (or logger.error(..., exc_info=True)) so the traceback is included with the primary message, and remove the separate logger.error(traceback.format_exc()).
| except Exception: | ||
| logger.debug(traceback.format_exc()) | ||
| logger.warning(f"Unable to get the region for bucket: {bucket_name}") | ||
| logger.error(f"Unable to get the region for bucket: {bucket_name}") | ||
| return region or self.service_connection.awsConfig.awsRegion |
There was a problem hiding this comment.
In the exception path, the traceback is only logged at DEBUG, while the ERROR log omits exception context. If the goal is to improve observability of failures, use logger.exception(...) (or logger.error(..., exc_info=True)) so the traceback is captured at error level and you don’t rely on DEBUG logs being enabled.
Resolved conflicts in two files where main's PR #27728 swapped `logger.warning` → `logger.error` while Stage 2c added noqa markers: - ingestion/src/metadata/clients/domo_client.py:168 (TRY201 noqa preserved) - ingestion/src/metadata/ingestion/source/pipeline/spline/utils.py two log blocks (TRY400 noqa preserved on traceback.format_exc lines) Re-ran `ruff check --add-noqa` on the merged files; main's auto-merged warning→error swap introduced 94 new TRY400 violations across data-quality validators which are now grandfathered in line. Regenerated basedpyright baseline in a Linux Docker container so column positions match CI (Ubuntu) exactly. Local macOS arm64 will see column- shifted entries due to stub differences between macOS and Linux wheels; `--baselinemode=discard` mode tolerates the drift downstream. Pinned `pythonPlatform = "Linux"` in pyproject.toml so CI analysis is platform- stable across runners.
Per gitar-bot's review on PR #27774: 1. Main's PR #27728 promoted ~60 `logger.warning()` → `logger.error()` inside `except` blocks. Those changes landed on main with their own baseline updates. Our PR doesn't promote anything — the merge from origin/main brought those `error` calls along with their baseline entries. The bot interpreted the `# noqa: TRY400` we added next to those lines as us silencing the rule case-by-case. Cleaner: globally ignore TRY400 in pyproject.toml, with a comment explaining why the codebase's `logger.error(...)` + separate `logger.debug(traceback.format_exc())` pattern is intentional. Strip ~430 per-line `# noqa: TRY400` markers from source. 2. Document that `S101` in `per-file-ignores` is a forward-looking entry — flake8-bandit (`S`) is not yet selected, so the rule is no-op today; the entry stays so when `S` lands later, tests don't immediately error. Reverts the platform pin and Linux Docker–generated baseline. Keep main's baseline intact and let CI surface the exact column-shifted entries; the team will decide whether to fix in-place (revert format on affected files) or add per-line `# pyright: ignore` markers.
Describe your changes:
Fixes logging to capture errors
Type of change:
Checklist:
Fixes <issue-number>: <short explanation>Summary by Gitar
client.pyto correctly log response content types as warnings instead of errors.test_mcp_client.pyassertions to reflect the change from warning to error logging for server connection failures.This will update automatically on new commits.