Skip to content

feat: keep the console log text only and make pytest log json only#1297

Closed
dbasunag wants to merge 3 commits intoopendatahub-io:mainfrom
dbasunag:log_console_file
Closed

feat: keep the console log text only and make pytest log json only#1297
dbasunag wants to merge 3 commits intoopendatahub-io:mainfrom
dbasunag:log_console_file

Conversation

@dbasunag
Copy link
Copy Markdown
Collaborator

@dbasunag dbasunag commented Mar 25, 2026

Pull Request

Summary

Related Issues

  • Fixes:
  • JIRA:

Please review and indicate how it has been tested

  • Locally
  • Jenkins

Additional Requirements

  • If this PR introduces a new test image, did you create a PR to mirror it in disconnected environment?
  • If this PR introduces new marker(s)/adds a new component, was relevant ticket created to update relevant Jenkins job?

Summary by CodeRabbit

  • Refactor

    • Improved logging for multiprocessing so log records are transmitted intact and formatted at output.
    • Console and file outputs now use per-logger formatting so core and auxiliary logs are rendered differently.
    • File logs explicitly support human-readable and JSON modes.
  • Bug Fixes

    • Exception tracebacks are preserved when logs are queued, ensuring complete error details in outputs.

@dbasunag dbasunag requested a review from a team as a code owner March 25, 2026 14:09
@github-actions
Copy link
Copy Markdown

The following are automatically added/executed:

  • PR size label.
  • Run pre-commit
  • Run tox
  • Add PR author as the PR assignee
  • Build image based on the PR

Available user actions:

  • To mark a PR as WIP, add /wip in a comment. To remove it from the PR comment /wip cancel to the PR.
  • To block merging of a PR, add /hold in a comment. To un-block merging of PR comment /hold cancel.
  • To mark a PR as approved, add /lgtm in a comment. To remove, add /lgtm cancel.
    lgtm label removed on each new commit push.
  • To mark PR as verified comment /verified to the PR, to un-verify comment /verified cancel to the PR.
    verified label removed on each new commit push.
  • To Cherry-pick a merged PR /cherry-pick <target_branch_name> to the PR. If <target_branch_name> is valid,
    and the current PR is merged, a cherry-picked PR would be created and linked to the current PR.
  • To build and push image to quay, add /build-push-pr-image in a comment. This would create an image with tag
    pr-<pr_number> to quay repository. This image tag, however would be deleted on PR merge or close action.
Supported labels

{'/verified', '/hold', '/wip', '/build-push-pr-image', '/cherry-pick', '/lgtm'}

dbasunag and others added 2 commits March 25, 2026 10:17
Signed-off-by: Debarati Basu-Nag <dbasunag@redhat.com>

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Debarati Basu-Nag <dbasunag@redhat.com>

Co-Authored-By: Claude <noreply@anthropic.com>
@github-actions github-actions bot added size/l and removed size/m labels Mar 25, 2026
fege
fege previously approved these changes Mar 25, 2026
Copy link
Copy Markdown
Contributor

@fege fege left a comment

Choose a reason for hiding this comment

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

/lgtm

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 25, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)

Review profile: CHILL

Plan: Pro

Run ID: 860fc221-2912-4a26-9ef5-81aade17d9e7

📥 Commits

Reviewing files that changed from the base of the PR and between 4e8878c and 27ccaa1.

📒 Files selected for processing (1)
  • utilities/opendatahub_logger.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • utilities/opendatahub_logger.py

📝 Walkthrough

Walkthrough

Decouples formatting from queue transport: introduces PassthroughQueueHandler to enqueue unformatted LogRecords (converting exc_infoexc_text), adds DelegatingFormatter to pick output formatter by logger name, and removes third‑party logging integration in favor of structlog-only plain text rendering.

Changes

Cohort / File(s) Summary
Queue handler & formatters
utilities/logger.py
Added PassthroughQueueHandler (passes raw LogRecords through queues, converts exc_infoexc_text), added DelegatingFormatter (selects formatter by record.name), updated setup_logging() to use passthrough handlers and set file handler formatter based on human_readable. Removed per-queue formatter assignments.
Structlog renderer & cleanup
utilities/opendatahub_logger.py
Removed third-party handler monkeypatching and formatter enforcement; introduced _plain_text_renderer for structlog output; stopped forcing JSONOnlyFormatter onto logging handlers; adjusted exception/text emission behavior and human-readable kwarg rendering.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Actionable Issues

  • CWE-391: Converting record.exc_info to record.exc_text and then clearing exc_info may lead to loss of structured exception data needed by downstream handlers. If structured tracebacks are required, preserve or serialize structured exception data before clearing.
  • CWE-367 / Race Condition risk: Preparing the record for queueing assumes exc_info and related objects are stable; ensure no concurrent mutation can invalidate or partially serialize traceback objects during prepare/put. Consider copying required data defensively.
  • Information loss: Clearing record.exc_info prevents downstream reconstruction of exception chains and programmatic inspection. Document this behavior and validate it against consumers that expect exc_info.
  • Formatter routing robustness: DelegatingFormatter matches on record.name; absent explicit fallback handling, misnamed or dynamic logger names may receive an unintended formatter. Add an explicit default formatter fallback and/or defensive logging when no match is found.
  • Incomplete cleanup check: Verify ThirdPartyHumanReadableFormatter / ThirdPartyJSONFormatter are not referenced elsewhere (configs, other modules, or runtime patches). Remove unused imports and references to avoid dead code.
🚥 Pre-merge checks | ✅ 1 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The PR description uses the required template structure but the Summary section is completely empty with no actual details about the changes, objectives, or rationale filled in. Fill in the Summary section with a brief description of why these logging changes are needed and what problems they solve. Add GitHub issue or JIRA references if applicable.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: converting console logs to text-only format while making pytest logs JSON-only.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@utilities/logger.py`:
- Around line 33-39: PassthroughQueueHandler.prepare currently clears
record.exc_info after populating record.exc_text, but ThirdPartyJSONFormatter
and ThirdPartyHumanReadableFormatter only format record.getMessage() so
tracebacks get dropped; update PassthroughQueueHandler.prepare to append
record.exc_text (when present) onto the record message (e.g., combine into
record.msg or record.__dict__['message']) before setting record.exc_info = None,
and also ensure the alternate prepare block at the other location (lines
102–106) does the same; alternatively, modify ThirdPartyJSONFormatter.format and
ThirdPartyHumanReadableFormatter.format to include record.exc_text in their
output when present.

In `@utilities/opendatahub_logger.py`:
- Around line 163-172: The plain-text renderer and StructlogWrapper._log() are
leaking secrets because they use f"{k}={v}" which uses str() and bypasses
RedactedString.__repr__; change both sites to format values with repr() (e.g.,
use f"{k}={v!r}" or explicitly repr(v)) so RedactedString's redaction is honored
when building extras and log messages; update _plain_text_renderer (function
name _plain_text_renderer) and the StructlogWrapper._log method to use repr()
for each value when joining event_dict items.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)

Review profile: CHILL

Plan: Pro

Run ID: ae341550-b7a4-4c27-ab3c-f53acffd8eab

📥 Commits

Reviewing files that changed from the base of the PR and between 9ef3f9d and 4e8878c.

📒 Files selected for processing (2)
  • utilities/logger.py
  • utilities/opendatahub_logger.py

Signed-off-by: Debarati Basu-Nag <dbasunag@redhat.com>

Co-Authored-By: Claude <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@fege fege left a comment

Choose a reason for hiding this comment

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

/lgtm

@dbasunag
Copy link
Copy Markdown
Collaborator Author

@jgarciao has a better approach for use of structlog. I saw his commit and closing this one in favor of his upcoming patch. Thanks a lot @jgarciao !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants