Skip to content

Conversation

@rodrigobr-msft
Copy link
Contributor

This pull request introduces a small refactor to the configuration import path and adds a new placeholder for logging configuration. The main changes are grouped below:

Configuration import path update:

  • Changed the import of load_configuration_from_env in __init__.py to use the new config._load_configuration module path, improving code organization.

Logging setup:

  • Added a new configure_logging function in config/_configure_logging.py as a placeholder for future logging configuration.

Copilot AI review requested due to automatic review settings November 6, 2025 21:08
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the configuration loading module path and introduces a placeholder for logging configuration functionality. The changes prepare the codebase for future logging configuration support while reorganizing the configuration-related code into a dedicated config package.

  • Moved configuration loading to a new config package structure
  • Added a placeholder configure_logging function for future logging setup

Reviewed Changes

Copilot reviewed 2 out of 4 changed files in this pull request and generated 2 comments.

File Description
config/_configure_logging.py New file containing placeholder function for logging configuration
__init__.py Updated import path to reference configuration loading from the new config package

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@rodrigobr-msft rodrigobr-msft marked this pull request as ready for review November 11, 2025 16:19
@rodrigobr-msft rodrigobr-msft requested a review from a team as a code owner November 11, 2025 16:19
@rodrigobr-msft rodrigobr-msft linked an issue Nov 12, 2025 that may be closed by this pull request
@cleemullins
Copy link
Collaborator

@rodrigobr-msft The code here looks fine, but I'm not super-clear on why it's needed. Is there an issue that describes the problem, or is this just viewed as a fairly straightforward refactor?

@rodrigobr-msft
Copy link
Contributor Author

@rodrigobr-msft The code here looks fine, but I'm not super-clear on why it's needed. Is there an issue that describes the problem, or is this just viewed as a fairly straightforward refactor?

It's a desired feature as when debugging with customers, I have to give them 4 lines of code for them to copy and paste into their source code to enable logging. I was talking with a team that had a .NET config file, and they expected the same logging configuration found in .NET to work with Python. I think this is a good step towards parity in configuration and a small quality of life improvement for devs. I'm open to hearing other perspectives.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

…config/_configure_logging.py

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

libraries/microsoft-agents-activity/microsoft_agents/activity/config/_load_configuration.py:31

  • The function _configure_logging is called with side effects during configuration loading, but errors (e.g., invalid log level) could interrupt the configuration process. Consider handling logging configuration errors gracefully or documenting that invalid logging configuration will cause the entire configuration load to fail.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

rodrigobr-msft and others added 2 commits November 13, 2025 07:08
…config/_configure_logging.py

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings November 13, 2025 15:10
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

Comments suppressed due to low confidence (1)

libraries/microsoft-agents-activity/microsoft_agents/activity/config/_load_configuration.py:31

  • The logging configuration is called unconditionally within load_configuration_from_env, but if an invalid log level is provided in the configuration, a ValueError will be raised. This could cause the entire configuration loading to fail unexpectedly.

Consider wrapping the logging configuration in error handling to make the configuration loading more resilient:

try:
    _configure_logging(result.get("LOGGING", {}))
except ValueError as e:
    # Log the error or handle it appropriately
    # Could also re-raise if logging config failures should be fatal
    pass

Alternatively, document that logging configuration errors are intentionally fatal and will prevent application startup.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot AI review requested due to automatic review settings December 1, 2025 19:40
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (1)

libraries/microsoft-agents-activity/microsoft_agents/activity/config/_load_configuration.py:31

  • The load_configuration_from_env function now has a side effect by calling _configure_logging, which modifies global logging state. This is unexpected behavior for a function that appears to only parse and return configuration data.

Consider:

  1. Documenting this side effect in the docstring
  2. Or returning the logging configuration in the result dictionary and letting the caller decide when to apply it
  3. Or renaming the function to reflect that it does more than just loading configuration

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@rodrigobr-msft rodrigobr-msft enabled auto-merge (squash) December 8, 2025 17:53
@rodrigobr-msft rodrigobr-msft merged commit 5ce846f into main Dec 8, 2025
9 of 10 checks passed
@rodrigobr-msft rodrigobr-msft deleted the users/robrandao/logging-with-config branch December 8, 2025 17:54
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.

Allow logging to be configurable in the .env file

3 participants