Skip to content

feat: improved itk logging#977

Open
kdziedzic70 wants to merge 1 commit into1.0-devfrom
dziedzick/improved-itk-logging
Open

feat: improved itk logging#977
kdziedzic70 wants to merge 1 commit into1.0-devfrom
dziedzick/improved-itk-logging

Conversation

@kdziedzic70
Copy link
Copy Markdown

Description

New version of itk https://github.com/a2aproject/a2a-samples/releases/tag/itk-v.015-alpha improves log readabiulity for debugging by spliting the logs of individual tested agents into separate files if the ITK_LOG_LEVEL environmental variable is set to "DEBUG"

This PR integrates the change into python's sdk CI and updates the instruction on how to set debugging mode for tests

Thank you for opening a Pull Request!
Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Follow the CONTRIBUTING Guide.
  • Make your Pull Request title in the https://www.conventionalcommits.org/ specification.
    • Important Prefixes for release-please:
      • fix: which represents bug fixes, and correlates to a SemVer patch.
      • feat: represents a new feature, and correlates to a SemVer minor.
      • feat!:, or fix!:, refactor!:, etc., which represent a breaking change (indicated by the !) and will result in a SemVer major.
  • Ensure the tests and linter pass (Run bash scripts/format.sh from the repository root to format)
  • Appropriate docs were updated (if necessary)

Fixes #<issue_number_goes_here> 🦕

@kdziedzic70 kdziedzic70 requested a review from a team as a code owner April 16, 2026 11:08
@kdziedzic70 kdziedzic70 changed the title improved itk logging feat: improved itk logging Apr 16, 2026
@github-actions
Copy link
Copy Markdown

🧪 Code Coverage (vs 1.0-dev)

⬇️ Download Full Report

No coverage changes.

Generated by coverage-comment.yml

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces configurable logging levels and debug log persistence for the ITK service. Key changes include updating the .gitignore to exclude log files, documenting the new debugging workflow in the README.md, and modifying main.py and run_itk.sh to respect the ITK_LOG_LEVEL environment variable. In DEBUG mode, a local logs/ directory is created and mounted to the container. Review feedback suggests optimizing main.py by reusing the global log level variable and improving the readability of run_itk.sh by merging redundant conditional blocks.

Comment thread itk/main.py
grpc_port,
)

uvicorn_log_level = os.environ.get('ITK_LOG_LEVEL', 'INFO').lower()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

low

The environment variable ITK_LOG_LEVEL is already fetched and stored in the global variable log_level at the top of the module. You can reuse it here to avoid redundant lookups.

Suggested change
uvicorn_log_level = os.environ.get('ITK_LOG_LEVEL', 'INFO').lower()
uvicorn_log_level = log_level.lower()

Comment thread itk/run_itk.sh
Comment on lines +70 to +77
if [ "${ITK_LOG_LEVEL^^}" = "DEBUG" ]; then
mkdir -p "$ITK_DIR/logs"
fi

DOCKER_MOUNT_LOGS=""
if [ "${ITK_LOG_LEVEL^^}" = "DEBUG" ]; then
DOCKER_MOUNT_LOGS="-v $ITK_DIR/logs:/app/logs"
fi
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

low

These two conditional blocks check the same condition (ITK_LOG_LEVEL being DEBUG). They can be merged for better readability and to avoid redundant checks.

Suggested change
if [ "${ITK_LOG_LEVEL^^}" = "DEBUG" ]; then
mkdir -p "$ITK_DIR/logs"
fi
DOCKER_MOUNT_LOGS=""
if [ "${ITK_LOG_LEVEL^^}" = "DEBUG" ]; then
DOCKER_MOUNT_LOGS="-v $ITK_DIR/logs:/app/logs"
fi
DOCKER_MOUNT_LOGS=""
if [ "${ITK_LOG_LEVEL^^}" = "DEBUG" ]; then
mkdir -p "$ITK_DIR/logs"
DOCKER_MOUNT_LOGS="-v $ITK_DIR/logs:/app/logs"
fi

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant