fix: logger info fix #318
fix: logger info fix #318
Conversation
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
dcs_core/integrations/databases/sybase.py (3)
72-77: Remove unnecessary f-strings (Ruff F541).These log messages don't interpolate any variables. Ruff flags them as F541. Drop the
fprefix.- logger.debug(f"Attempting FreeTDS connection") + logger.debug("Attempting FreeTDS connection") @@ - logger.info( - f"Successfully connected to Sybase using FreeTDS" - ) + logger.info("Successfully connected to Sybase using FreeTDS")
151-154: Unnecessary f-string in debug log (Ruff F541).No placeholders here either; remove the
f.- logger.debug( - f"Attempting connection ..." - ) + logger.debug("Attempting connection ...")
156-162: Reduce info-level connection details; move specifics to debug and avoid logging endpoint values.Current info log emits host/server and port details via
attempt['value'],port_config,ase_config. For PII/noise reduction, keep info concise and push details to debug. Omit sensitive endpoint values at debug, or include a masked version if you prefer.- logger.info( - f"Successfully connected to Sybase using: " - f"driver={driver}, " - f"{attempt['key']}={attempt['value']}, " - f"port_config={port_config}, " - f"ase_config={ase_config}" - ) + logger.info("Successfully connected to Sybase") + logger.debug( + "Connection details: driver={}, {}=<omitted>, port_config={}, ase_config={}", + driver, + attempt["key"], + port_config, + ase_config, + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
dcs_core/integrations/databases/sybase.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.12.2)
dcs_core/integrations/databases/sybase.py
73-73: f-string without any placeholders
Remove extraneous f prefix
(F541)
76-76: f-string without any placeholders
Remove extraneous f prefix
(F541)
153-153: f-string without any placeholders
Remove extraneous f prefix
(F541)
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
dcs_core/integrations/databases/sybase.py (2)
161-167: Fix unused exception variable and message casing; keep logs PII-safe.
eis unused (Ruff F841).- Capitalize “Sybase” for consistency.
Apply this diff:
- except Exception as e: - error_msg = ( - "Failed to connect to sybase." - ) + except Exception: + error_msg = "Failed to connect to Sybase." logger.debug(error_msg) errors.append(error_msg) continue
1-1: CI failures: commit Black formatting and fix lint errors insybase.pyPlease stage and commit the changes to
dcs_core/integrations/databases/sybase.pyand address the following ruff issues flagged by your pre-commit hooks:• Remove extraneous
fprefixes on logger calls (F541)
• Chain exceptions properly: useraise ... from eorfrom None(B904)
• Remove unused exception variablese(F841)
• Rename unused loop variableindex_nameto_index_name(B007)
• Simplify theif-elseinto a ternary fortype_condition(SIM108)
• Combine duplicateelifbranches forfigi/isinlength checks (SIM114)After applying these fixes, run:
pre-commit run -aand commit all resulting changes.
🧹 Nitpick comments (2)
dcs_core/integrations/databases/sybase.py (2)
80-82: Sanitize FreeTDS error log to avoid leaking exception details.Current message includes
str(e), which may contain host/DSN/other sensitive info. Prefer a generic error at error-level and, if needed, a non-sensitive hint at debug-level.Apply this diff:
- except Exception as e: - error_msg = f"Failed to connect to Sybase with FreeTDS: {str(e)}" - logger.error(error_msg) + except Exception as e: + error_msg = "Failed to connect to Sybase with FreeTDS." + logger.error(error_msg) + # Optional: minimal non-sensitive context for debugging + logger.debug("Exception type: {}", type(e).__name__) raise DataChecksDataSourcesConnectionError(message=error_msg)
169-172: Final aggregated error is noisy and low-signal; consider de-duplicating or adding non-sensitive context.Right now every attempt appends the same generic string, which produces a long, repetitive message. Either:
- include non-sensitive context (e.g., attempt key only, not values), or
- deduplicate before joining.
Example lightweight change (deduplicate only):
Apply this diff:
- raise DataChecksDataSourcesConnectionError( - message=f"Failed to connect to Sybase data source with driver {driver}: " - f"[{'; '.join(errors)}]" - ) + raise DataChecksDataSourcesConnectionError( + message=f"Failed to connect to Sybase data source with driver {driver}: " + f"[{'; '.join(sorted(set(errors)))}]" + )If you want, I can propose a safe per-attempt context string like:
"Failed to connect to Sybase (attempt_key=HOST|SERVER|SERVERNAME, port_mode=explicit|embedded, ase_params_present=true|false)"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
dcs_core/integrations/databases/sybase.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
dcs_core/integrations/databases/sybase.py (2)
dcs_core/integrations/databases/postgres.py (1)
connect(30-54)dcs_core/core/datasource/manager.py (1)
connect(52-63)
🪛 Ruff (0.12.2)
dcs_core/integrations/databases/sybase.py
73-73: f-string without any placeholders
Remove extraneous f prefix
(F541)
76-76: f-string without any placeholders
Remove extraneous f prefix
(F541)
153-153: f-string without any placeholders
Remove extraneous f prefix
(F541)
161-161: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
🪛 GitHub Actions: CI
dcs_core/integrations/databases/sybase.py
[error] 1-1: Black formatting changed dcs_core/integrations/databases/sybase.py during pre-commit; please commit the reformatted file.
🔇 Additional comments (1)
dcs_core/integrations/databases/sybase.py (1)
156-160: Good PII reduction on success logs.Only logging the driver (without connection details) is a solid improvement for avoiding credential/config leakage.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
dcs_core/integrations/databases/sybase.py (2)
77-80: Sanitize error logging to avoid leaking secrets from ODBC exceptions.
str(e)from pyodbc often contains parts of the connection string (including credentials). Avoid logging it or propagating it in the error message.Apply this diff to keep logs safe while retaining debuggability:
- except Exception as e: - error_msg = f"Failed to connect to Sybase with FreeTDS: {str(e)}" - logger.error(error_msg) - raise DataChecksDataSourcesConnectionError(message=error_msg) + except Exception as e: + # Avoid leaking credentials in logs; keep details at debug level only + logger.debug(f"FreeTDS connection failed with exception: {e!r}") + error_msg = "Failed to connect to Sybase with FreeTDS" + logger.error(error_msg) + # Preserve cause without exposing it in the message + raise DataChecksDataSourcesConnectionError(message=error_msg) from e
158-163: Use the caught exception (fix Ruff F841) and improve error signal without leaking secrets.Currently
eis unused (F841), and the collected errors are identical/low-signal. Log exception details at debug, keep public message generic, and standardize “Sybase” capitalization.- except Exception as e: - error_msg = ( - "Failed to connect to sybase." - ) - logger.debug(error_msg) - errors.append(error_msg) - continue + except Exception as e: + # Keep public/error-facing message generic to avoid leaking credentials + logger.debug(f"Connection attempt failed: {e!r}") + error_msg = f"Failed to connect to Sybase ({e.__class__.__name__})." + logger.debug(error_msg) + errors.append(error_msg) + continue
♻️ Duplicate comments (3)
dcs_core/integrations/databases/sybase.py (3)
73-73: Unnecessary f-string removed — good catch.This resolves Ruff F541 for this log line.
75-75: Unnecessary f-string removed — consistent with lint guidance.Keeps logs clean and avoids F541.
150-150: Unnecessary f-string removed in attempt log — thanks.Aligns with earlier lint recommendation and keeps log noise low.
🧹 Nitpick comments (2)
dcs_core/integrations/databases/sybase.py (2)
153-155: Nit: consolidate info log string.Slightly cleaner as a single string; alternatively use Loguru’s
{}formatting.Pick one of these:
- logger.info( - "Successfully connected to Sybase using: " - f"driver={driver}" - ) + logger.info(f"Successfully connected to Sybase using: driver={driver}")or
- logger.info( - "Successfully connected to Sybase using: " - f"driver={driver}" - ) + logger.info("Successfully connected to Sybase using: driver={}", driver)
165-168: Optional: de-duplicate accumulated error messages to reduce noise.If several attempts fail similarly, the final message becomes repetitive.
- raise DataChecksDataSourcesConnectionError( - message=f"Failed to connect to Sybase data source with driver {driver}: " - f"[{'; '.join(errors)}]" - ) + unique_errors = list(dict.fromkeys(errors)) # Preserve order while deduping + raise DataChecksDataSourcesConnectionError( + message=( + f"Failed to connect to Sybase data source with driver {driver}: " + f"[{'; '.join(unique_errors)}]" + ) + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
dcs_core/integrations/databases/sybase.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
dcs_core/integrations/databases/sybase.py (2)
dcs_core/integrations/databases/mssql.py (1)
connect(43-59)dcs_core/core/datasource/manager.py (1)
connect(52-63)
🪛 Ruff (0.12.2)
dcs_core/integrations/databases/sybase.py
157-157: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
🪛 GitHub Actions: CI
dcs_core/integrations/databases/sybase.py
[error] 155-156: Black formatting changed the file. Reformatted dcs_core/integrations/databases/sybase.py. Run 'pre-commit run --all-files' to apply and commit the changes.
🔇 Additional comments (1)
dcs_core/integrations/databases/sybase.py (1)
50-58: CI formatting failure indcs_core/integrations/databases/sybase.py
Please run your pre-commit hooks locally to apply Black formatting and unblock CI. For example:pre-commit run --all-files --show-diff-on-failure git add -A && git commit -m "chore: apply pre-commit formatters"Once formatted, push the updated file to resolve the CI error.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
dcs_core/integrations/databases/sybase.py (2)
153-155: Nit: Collapse the multi-line info log into a single string.Simplifies readability and avoids implicit concatenation.
- logger.info( - "Successfully connected to Sybase using: " - f"driver={driver}" - ) + logger.info(f"Successfully connected to Sybase using: driver={driver}")
150-150: Optional: Add minimal context to the attempt debug log (no PII).Helps when diagnosing which attempt permutation worked/failed without exposing host/user credentials.
- logger.debug("Attempting connection ...") + logger.debug( + f"Attempting connection (driver={driver}, key={attempt['key']}, ase_keys={list(ase_config.keys())})" + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
dcs_core/integrations/databases/sybase.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
dcs_core/integrations/databases/sybase.py (2)
dcs_core/integrations/databases/postgres.py (1)
connect(30-54)dcs_core/core/datasource/manager.py (1)
connect(52-63)
🪛 Ruff (0.12.2)
dcs_core/integrations/databases/sybase.py
157-157: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
🔇 Additional comments (1)
dcs_core/integrations/databases/sybase.py (1)
72-81: FreeTDS logging cleanup and error propagation look good.String interpolation removed where unnecessary; success/failure messages are concise, and errors are surfaced via DataChecksDataSourcesConnectionError.
| except Exception as e: | ||
| error_msg = ( | ||
| f"Failed with {attempt['key']}={attempt['value']}, " | ||
| f"port_config={port_config}, " | ||
| f"ase_config={ase_config}: {str(e)}" | ||
| ) | ||
| error_msg = "Failed to connect to sybase." | ||
| logger.debug(error_msg) | ||
| errors.append(error_msg) | ||
| continue |
There was a problem hiding this comment.
Fix unused exception variable (Ruff F841) and improve failure diagnostics.
Currently e is unused and every attempt appends the same generic error, reducing debuggability. Include a sanitized summary (exception class + first line) to both logs and aggregation; this also resolves F841.
- except Exception as e:
- error_msg = "Failed to connect to sybase."
- logger.debug(error_msg)
- errors.append(error_msg)
- continue
+ except Exception as e:
+ # Include exception class and first line only; avoids leaking connection params
+ err_summary = f"{type(e).__name__}: {str(e).splitlines()[0]}"
+ logger.debug(f"Connection attempt failed: {err_summary}")
+ errors.append(err_summary)
+ continue📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| except Exception as e: | |
| error_msg = ( | |
| f"Failed with {attempt['key']}={attempt['value']}, " | |
| f"port_config={port_config}, " | |
| f"ase_config={ase_config}: {str(e)}" | |
| ) | |
| error_msg = "Failed to connect to sybase." | |
| logger.debug(error_msg) | |
| errors.append(error_msg) | |
| continue | |
| except Exception as e: | |
| # Include exception class and first line only; avoids leaking connection params | |
| err_summary = f"{type(e).__name__}: {str(e).splitlines()[0]}" | |
| logger.debug(f"Connection attempt failed: {err_summary}") | |
| errors.append(err_summary) | |
| continue |
🧰 Tools
🪛 Ruff (0.12.2)
157-157: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
🤖 Prompt for AI Agents
In dcs_core/integrations/databases/sybase.py around lines 157 to 161, the except
block currently binds an unused variable `e` and appends a generic error
message; change it to use `e` by building a sanitized summary (exception class
name + first line of str(e)) and include that summary in the logger call and in
the errors list (e.g., combine a short contextual message with the summary).
This removes the unused-variable warning and provides better diagnostics while
preserving the loop continue behavior.
Fixes/Implements
Description
Summary Goes here.
Type of change
Delete irrelevant options.
How Has This Been Tested?
Summary by CodeRabbit