fix: use json format directly if available and add information on debugging

[dbasunag]

…

Pull Request

Summary

Related Issues

• Fixes:
• JIRA:

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

• Documentation
  • Added a "Debugging test failures" guide showing how to enable human-readable test logs, example commands, sample formatted log lines, and a one-liner to convert existing machine-formatted log files into readable text.
• New Features
  • Tests now support a --readable-logs option to emit human-friendly log output during runs.

[github-actions]

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

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

[coderabbitai]

📝 Walkthrough

Walkthrough

Adds a “Debugging test failures” doc section and a pytest --readable-logs CLI flag; introduces human-readable logging mode (new formatter, toggle set_human_readable) and updates ThirdPartyJSONFormatter.format to pass-through already-JSON messages or wrap non-JSON messages into structured JSON. setup_logging gains a human_readable parameter.

Changes

Cohort / File(s)	Summary
Documentation docs/GETTING_STARTED.md	Added "Debugging test failures" section showing --readable-logs usage, sample human-readable log lines, and a sed/jq one-liner to convert existing pytest-tests.log JSON entries into readable text.
Pytest integration conftest.py	Added pytest CLI option --readable-logs and forwards selected value to setup_logging as human_readable during pytest_sessionstart.
Logging setup utilities/logger.py	Extended setup_logging(...) signature with human_readable: bool = False and now calls set_human_readable(enabled=human_readable) during initialization.
Logging formatter & runtime utilities/opendatahub_logger.py	Added set_human_readable(enabled: bool), new ThirdPartyHumanReadableFormatter, and logic to initialize/patch handlers for human-readable vs JSON output. ThirdPartyJSONFormatter.format now tries json.loads(record.getMessage()) and returns the original message if valid JSON; on parse failure it emits structured JSON including timestamp, logger, level, event, filename, lineno. StructlogWrapper._log routes through stdlib logging in human-readable mode and stringifies extra key=value pairs. _initialized reset when switching modes.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Security findings (actionable)

• Risk: human-readable logs and the new pytest flag may increase accidental exposure of sensitive data in logs (secrets, tokens). Action: review log statements and sanitize/avoid logging secrets. See CWE-532.
• Risk: passing through already-JSON messages without normalization could allow log injection or uncontrolled content in downstream systems. Action: validate/normalize message fields or escape control characters (consider CRLF neutralization). See CWE-117.
• Risk: dynamic patching of Logger handlers/formatters may cause duplicate handlers or misconfigured handlers if not idempotent. Action: ensure patches skip already-patched handlers and include tests for handler reconfiguration. See CWE-665.

🚥 Pre-merge checks | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Title check	⚠️ Warning	Title is partially related but misleading—it emphasizes 'use json format directly if available' when the main change is adding human-readable logging support via --readable-logs flag, which contradicts the stated intent.	Revise title to accurately reflect the primary change: add human-readable logging support (--readable-logs) while preserving JSON fallback behavior.
Description check	⚠️ Warning	Description uses only template placeholders with no substantive content—Summary, Related Issues, and Additional Requirements sections are unfilled comments.	Complete the Summary section with actual change rationale, populate Related Issues if applicable, and clarify testing and deployment requirements.

_{✏️ 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.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

utilities/opendatahub_logger.py (1)

117-124: lineno serialized as string instead of integer.

Line 123 converts lineno to str, but JSON logging conventions typically preserve numeric types. This may complicate downstream log aggregation/filtering expecting an integer.

Proposed fix

                 "event": msg,
                 "filename": record.pathname.split("/")[-1] if record.pathname else "",
-                "lineno": str(record.lineno),
+                "lineno": record.lineno,
             })

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@utilities/opendatahub_logger.py` around lines 117 - 124, The JSON formatter
is converting record.lineno to a string; change the serialization to keep it as
an integer by removing the str() wrapper so "lineno" is assigned record.lineno
(an int) in the dict returned by the formatter (refer to the JSON dict
construction using record.lineno and the surrounding keys like "timestamp",
"logger", and "level" to locate the code).

🤖 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/opendatahub_logger.py`:
- Line 116: The except clause in utilities/opendatahub_logger.py uses Python 2
syntax ("except json.JSONDecodeError, TypeError:") which is a SyntaxError in
Python 3; update the exception handling in that try/except to use a
parenthesized exception tuple (or separate except blocks) so it reads as
catching both json.JSONDecodeError and TypeError, ensuring the module imports
under Python 3.

---

Nitpick comments:
In `@utilities/opendatahub_logger.py`:
- Around line 117-124: The JSON formatter is converting record.lineno to a
string; change the serialization to keep it as an integer by removing the str()
wrapper so "lineno" is assigned record.lineno (an int) in the dict returned by
the formatter (refer to the JSON dict construction using record.lineno and the
surrounding keys like "timestamp", "logger", and "level" to locate the code).

---

ℹ️ Review info

⚙️ Run configuration

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

Review profile: CHILL

Plan: Pro

Run ID: 586b1f72-6203-42d8-a058-060d5263c2d4

📥 Commits

Reviewing files that changed from the base of the PR and between a84ef19 and 9d0af74.

📒 Files selected for processing (2)

• docs/GETTING_STARTED.md
• utilities/opendatahub_logger.py

[coderabbitai]

Actionable comments posted: 2

♻️ Duplicate comments (1)

utilities/opendatahub_logger.py (1)

116-116: ⚠️ Potential issue | 🟠 Major

Line 116 blocks the current lint toolchain.

This Python 3.14-only except A, B form may be valid for the runtime, but Flake8 7.3.0 is still reporting E999 on it in this PR. Either keep the parenthesized tuple here or upgrade the parser/linter stack before merging.

Expected result: the repo config will show whether the branch is intentionally on Python 3.14 while the lint toolchain still targets an older parser.

#!/bin/bash
set -euo pipefail

while IFS= read -r f; do
  printf '\n== %s ==\n' "$f"
  rg -n "requires-python|python_requires|flake8|ruff|external" "$f" || true
done < <(fd '^(pyproject\.toml|setup\.cfg|setup\.py|tox\.ini|\.pre-commit-config\.yaml)$')

printf '\n== Affected syntax ==\n'
sed -n '113,117p' utilities/opendatahub_logger.py

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@utilities/opendatahub_logger.py` at line 116, Change the invalid Python
3.14-only except syntax to a tuple form so the linter accepts it: replace the
line using "except json.JSONDecodeError, TypeError:" with "except
(json.JSONDecodeError, TypeError):" (keeping the existing exception handling
body intact) so the block catching json.JSONDecodeError and TypeError is
compatible with current Flake8/older parsers.

🤖 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 24-30: The setup_logging function currently uses a predictable
default log_file "/tmp/pytest-tests.log" which is insecure; change setup_logging
to avoid a fixed path by either requiring callers to pass an explicit log_file
or creating a secure temp file inside the function using
tempfile.mkstemp()/NamedTemporaryFile and using that file descriptor/path for
logging; update the signature of setup_logging (remove the unsafe default or set
log_file to None) and ensure the rest of the function (QueueListener setup and
any FileHandler creation) uses the securely-created path or raises a clear error
if None.

In `@utilities/opendatahub_logger.py`:
- Around line 111-124: ThirdPartyJSONFormatter.format currently returns any
syntactically valid JSON string unchanged and serializes only
record.getMessage(), which allows untrusted payloads to override provenance
fields and drops traceback/stack info; update ThirdPartyJSONFormatter.format
(and the other formatter override) to always build a JSON object that merges the
original message payload under a safe key (e.g. "event" or "message_payload")
instead of replacing provenance fields, and include record.exc_info (formatted
via logging.Formatter.formatException) and record.stack_info when present plus
canonical fields timestamp, logger, level, filename, lineno; ensure you parse
record.getMessage() only to include its content rather than using it as the full
output so exc_info/stack_info and provenance are preserved.

---

Duplicate comments:
In `@utilities/opendatahub_logger.py`:
- Line 116: Change the invalid Python 3.14-only except syntax to a tuple form so
the linter accepts it: replace the line using "except json.JSONDecodeError,
TypeError:" with "except (json.JSONDecodeError, TypeError):" (keeping the
existing exception handling body intact) so the block catching
json.JSONDecodeError and TypeError is compatible with current Flake8/older
parsers.

---

ℹ️ Review info

⚙️ Run configuration

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

Review profile: CHILL

Plan: Pro

Run ID: d07ea629-ae49-4e3f-96f6-9795a3a5a3a7

📥 Commits

Reviewing files that changed from the base of the PR and between 9d0af74 and c1d6122.

📒 Files selected for processing (4)

• conftest.py
• docs/GETTING_STARTED.md
• utilities/logger.py
• utilities/opendatahub_logger.py

✅ Files skipped from review due to trivial changes (1)

• docs/GETTING_STARTED.md

[fege]

/lgtm

[github-actions]

Status of building tag latest: success.
Status of pushing tag latest to image registry: success.