-
Notifications
You must be signed in to change notification settings - Fork 56
Support log configuration in the .env file #230
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this 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
configpackage structure - Added a placeholder
configure_loggingfunction 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.
libraries/microsoft-agents-activity/microsoft_agents/activity/config/_configure_logging.py
Outdated
Show resolved
Hide resolved
libraries/microsoft-agents-activity/microsoft_agents/activity/config/_configure_logging.py
Show resolved
Hide resolved
There was a problem hiding this 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.
libraries/microsoft-agents-activity/microsoft_agents/activity/config/_configure_logging.py
Show resolved
Hide resolved
libraries/microsoft-agents-activity/microsoft_agents/activity/config/_configure_logging.py
Outdated
Show resolved
Hide resolved
libraries/microsoft-agents-activity/microsoft_agents/activity/config/_configure_logging.py
Outdated
Show resolved
Hide resolved
|
@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. |
There was a problem hiding this 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.
libraries/microsoft-agents-activity/microsoft_agents/activity/config/_configure_logging.py
Show resolved
Hide resolved
libraries/microsoft-agents-activity/microsoft_agents/activity/config/_configure_logging.py
Outdated
Show resolved
Hide resolved
libraries/microsoft-agents-activity/microsoft_agents/activity/config/_configure_logging.py
Outdated
Show resolved
Hide resolved
…config/_configure_logging.py Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this 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_loggingis 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.
libraries/microsoft-agents-activity/microsoft_agents/activity/config/_configure_logging.py
Outdated
Show resolved
Hide resolved
…into users/robrandao/logging-with-config
…config/_configure_logging.py Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this 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, aValueErrorwill 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
passAlternatively, 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.
libraries/microsoft-agents-activity/microsoft_agents/activity/config/_configure_logging.py
Show resolved
Hide resolved
libraries/microsoft-agents-activity/microsoft_agents/activity/config/_configure_logging.py
Show resolved
Hide resolved
libraries/microsoft-agents-activity/microsoft_agents/activity/config/_configure_logging.py
Show resolved
Hide resolved
libraries/microsoft-agents-activity/microsoft_agents/activity/config/_configure_logging.py
Show resolved
Hide resolved
There was a problem hiding this 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.
libraries/microsoft-agents-activity/microsoft_agents/activity/config/_configure_logging.py
Show resolved
Hide resolved
libraries/microsoft-agents-activity/microsoft_agents/activity/config/_configure_logging.py
Show resolved
Hide resolved
libraries/microsoft-agents-activity/microsoft_agents/activity/config/_configure_logging.py
Show resolved
Hide resolved
…into users/robrandao/logging-with-config
…com/microsoft/Agents-for-python into users/robrandao/logging-with-config
There was a problem hiding this 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_envfunction 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:
- Documenting this side effect in the docstring
- Or returning the logging configuration in the result dictionary and letting the caller decide when to apply it
- 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.
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:
load_configuration_from_envin__init__.pyto use the newconfig._load_configurationmodule path, improving code organization.Logging setup:
configure_loggingfunction inconfig/_configure_logging.pyas a placeholder for future logging configuration.