Azure: add the ability to set log level for any loggers in the EventHub connector#2027
Azure: add the ability to set log level for any loggers in the EventHub connector#2027
Conversation
Reviewer's GuideAdds configurable logging support to the Azure EventHub connector via environment variables and bumps the Azure integration version to 2.9.3 with corresponding changelog and manifest updates. Class diagram for Azure EventHub connector logging utilitiesclassDiagram
class AzureEventsHubConfiguration {
}
class LoggingModule {
+set_log_level(loggers, log_level) void
+set_log_level_from_env() void
}
class AzureEventHubConnectorModule {
+set_log_level_from_env_on_import() void
}
AzureEventHubConnectorModule ..> LoggingModule : uses
LoggingModule ..> AzureEventsHubConfiguration : used_with
Flow diagram for Azure EventHub connector log level initialization from environmentflowchart TD
A[Connector module import] --> B[Call set_log_level_from_env]
B --> C{AZURE_LOG_LEVEL set?}
C -- No --> D[Return without changes]
C -- Yes --> E[Initialize loggers as None]
E --> F{AZURE_LOGGERS set?}
F -- Yes --> G[Parse AZURE_LOGGERS into set of names]
F -- No --> H[Use all loggers from logging.root.manager.loggerDict keys]
G --> I[Call set_log_level]
H --> I[Call set_log_level]
I --> J[Iterate over each logger name]
J --> K[Get logger by name and set level to AZURE_LOG_LEVEL]
K --> L[Connector continues with configured logging]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
set_log_level_from_envhelper assumesAZURE_LOG_LEVELis a valid integer and will raise if it isn't; consider validating the value (e.g., supporting symbolic levels likeDEBUG/INFO) and failing gracefully instead of throwing at import time. - Calling
set_log_level_from_env()at module import time makes logger configuration a side effect of importingazure_eventhub; consider moving this into an explicit initialization path so that import order and partial environments don't unexpectedly affect logging. - When parsing
AZURE_LOGGERS, a trailing comma or extra commas will produce empty logger names that end up configuring the root logger; consider filtering out empty names before passing them toset_log_level.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `set_log_level_from_env` helper assumes `AZURE_LOG_LEVEL` is a valid integer and will raise if it isn't; consider validating the value (e.g., supporting symbolic levels like `DEBUG`/`INFO`) and failing gracefully instead of throwing at import time.
- Calling `set_log_level_from_env()` at module import time makes logger configuration a side effect of importing `azure_eventhub`; consider moving this into an explicit initialization path so that import order and partial environments don't unexpectedly affect logging.
- When parsing `AZURE_LOGGERS`, a trailing comma or extra commas will produce empty logger names that end up configuring the root logger; consider filtering out empty names before passing them to `set_log_level`.
## Individual Comments
### Comment 1
<location> `Azure/connectors/logging.py:38` </location>
<code_context>
+ loggers = set(logging.root.manager.loggerDict.keys())
+
+ # Set the log level for the specified loggers.
+ set_log_level(loggers, int(os.environ.get("AZURE_LOG_LEVEL")))
</code_context>
<issue_to_address>
**issue:** Guard against invalid AZURE_LOG_LEVEL values to avoid crashing on import.
Calling `int(os.environ.get("AZURE_LOG_LEVEL"))` will raise `ValueError` if the env var is set to a non-integer (including empty string), which will break import since this runs at import time. Wrap this in a `try`/`except` for `ValueError` (and possibly `TypeError`) and fall back to a default log level or ignore the override when parsing fails.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Azure/connectors/logging.py
Outdated
| loggers = set(logging.root.manager.loggerDict.keys()) | ||
|
|
||
| # Set the log level for the specified loggers. | ||
| set_log_level(loggers, int(os.environ.get("AZURE_LOG_LEVEL"))) |
There was a problem hiding this comment.
issue: Guard against invalid AZURE_LOG_LEVEL values to avoid crashing on import.
Calling int(os.environ.get("AZURE_LOG_LEVEL")) will raise ValueError if the env var is set to a non-integer (including empty string), which will break import since this runs at import time. Wrap this in a try/except for ValueError (and possibly TypeError) and fall back to a default log level or ignore the override when parsing fails.
There was a problem hiding this comment.
Pull request overview
This PR adds an environment-variable-based mechanism to control Python logger levels for the Azure EventHub connector, and bumps the Azure module version accordingly.
Changes:
- Bump Azure module version to
2.9.3and add a changelog entry. - Add
Azure/connectors/logging.pyhelper to set logger levels fromAZURE_LOG_LEVEL/AZURE_LOGGERS. - Invoke the env-based log-level initialization from the EventHub connector module.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
Azure/manifest.json |
Bumps module version to 2.9.3 (and minor JSON formatting). |
Azure/connectors/logging.py |
Introduces env-driven logger level configuration helpers. |
Azure/connectors/azure_eventhub.py |
Calls the new env-driven log-level initializer. |
Azure/CHANGELOG.md |
Documents release 2.9.3 and the added logging configurability. |
| loggers = None | ||
|
|
||
| # If AZURE_LOGGERS is set, use the specified loggers. Otherwise, use all loggers. | ||
| if os.environ.get("AZURE_LOGGERS") is not None: | ||
| loggers = set(os.environ.get("AZURE_LOGGERS").split(",")) | ||
|
|
||
| # If AZURE_LOGGERS is not set, use all loggers. | ||
| if loggers is None: |
There was a problem hiding this comment.
loggers = None is later reassigned to a set, which is a type inconsistency and will trip strict type-checking; annotate it as set[str] | None (or avoid the sentinel by initializing to an empty set). Also, when AZURE_LOGGERS is an empty string, split(",") yields "" and ends up modifying the root logger unintentionally—filter out empty names.
| loggers = None | |
| # If AZURE_LOGGERS is set, use the specified loggers. Otherwise, use all loggers. | |
| if os.environ.get("AZURE_LOGGERS") is not None: | |
| loggers = set(os.environ.get("AZURE_LOGGERS").split(",")) | |
| # If AZURE_LOGGERS is not set, use all loggers. | |
| if loggers is None: | |
| loggers_env = os.environ.get("AZURE_LOGGERS") | |
| # If AZURE_LOGGERS is set and non-empty, use the specified loggers (excluding empty names). | |
| if loggers_env: | |
| loggers: set[str] = {name.strip() for name in loggers_env.split(",") if name.strip()} | |
| else: | |
| # If AZURE_LOGGERS is not set or empty, use all loggers. |
| from .logging import set_log_level_from_env | ||
| from .metrics import EVENTS_LAG, FORWARD_EVENTS_DURATION, INCOMING_MESSAGES, MESSAGES_AGE, OUTCOMING_EVENTS | ||
|
|
||
| # Initialize log level from environment variables | ||
| set_log_level_from_env() | ||
|
|
There was a problem hiding this comment.
Calling set_log_level_from_env() at module import introduces a global side effect and can make simply importing this module fail depending on environment variables. Prefer invoking this during connector startup (e.g., in AzureEventsHubTrigger.__init__/run) after logging is configured, so behavior is deterministic and easier to test.
| # Initialize log level from environment variables | ||
| set_log_level_from_env() | ||
|
|
There was a problem hiding this comment.
This new env-driven logging behavior isn’t covered by tests. Since Azure/tests/connector/test_azure_eventhub.py already exists, add unit tests that verify: (1) valid values change the expected logger levels, (2) AZURE_LOGGERS selection works, and (3) invalid AZURE_LOG_LEVEL values don’t crash startup.
Azure/connectors/logging.py
Outdated
| loggers = set(logging.root.manager.loggerDict.keys()) | ||
|
|
||
| # Set the log level for the specified loggers. | ||
| set_log_level(loggers, int(os.environ.get("AZURE_LOG_LEVEL"))) |
There was a problem hiding this comment.
int(os.environ.get("AZURE_LOG_LEVEL")) will raise ValueError for common inputs like "INFO"/"DEBUG" and will crash the connector (currently at import time). Consider accepting standard level names (e.g., via logging.getLevelName/logging._nameToLevel) and handling invalid values gracefully (fallback or ignore) instead of throwing.
mchupeau-sk
left a comment
mchupeau-sk
left a comment
There was a problem hiding this comment.
Maybe apply copilot suggestion ? LGFM
Add the ability to set log level for any loggers in the Azure EventHub connector with some environment variables.
Summary by Sourcery
Allow configuring Azure EventHub connector logging levels via environment variables and bump the Azure integration version.
New Features:
Enhancements:
Build:
Documentation: