-
Notifications
You must be signed in to change notification settings - Fork 592
Python: fix: make Azure Monitor OpenTelemetry an optional dependency #1377
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
base: main
Are you sure you want to change the base?
Python: fix: make Azure Monitor OpenTelemetry an optional dependency #1377
Conversation
This change makes azure-monitor-opentelemetry packages optional to allow using OTEL auto-instrumentation with OTLP exporters (Aspire, Jaeger, Zipkin, etc.) without requiring Azure Monitor. Changes: - Move azure-monitor-opentelemetry to optional dependencies [azure-monitor] - Move azure-monitor-opentelemetry-exporter to optional dependencies - Update observability.py to conditionally import Azure Monitor - Add is_azure_monitor_available() helper function - Add proper error handling and helpful warnings - Update installation and configuration documentation - Add comprehensive tests for different exporter configurations - Maintain backward compatibility for Azure Monitor users BREAKING CHANGE: Azure Monitor packages no longer installed by default. Install with: pip install agent-framework-core[azure-monitor] Fixes microsoft#1376
|
This would technically fix the issue, but this whole PR could be 2-3 lines. Not 300+. Let's resolve the discussion on the issue thread first |
|
@tonybaloney Thank you for the feedback! I understand the concern about the line count. Let me explain why this comprehensive approach is necessary and actually the minimal solution for a production-ready fix: Why 300+ Lines is the Right Solution1️⃣ Core Problem Requires Proper Abstraction (60 lines in
|
| "OtelAttr", | ||
| "get_meter", | ||
| "get_tracer", | ||
| "is_azure_monitor_available", # NEW: Helper function to check Azure Monitor availability |
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.
Why is this commented? Source control is used for keeping track of why code changes were made
| ] | ||
|
|
||
|
|
||
|
|
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.
Why is there a new line break here? This would violate linter rules and seems unrelated to the changes
|
|
||
| logger = get_logger() | ||
|
|
||
| # Check if Azure Monitor is available (add after line 60) |
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.
Why is the line number in a comment
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.
this comment isn't needed
| Args: | ||
| connection_strings: List of Azure Monitor connection strings | ||
| credential: Optional TokenCredential for Entra ID authentication |
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.
Token credential is a base class and has nothing to do with Entra
| exporters.append(AzureMonitorTraceExporter(connection_string=conn_string, credential=credential)) | ||
| exporters.append(AzureMonitorMetricExporter(connection_string=conn_string, credential=credential)) | ||
| except Exception as e: | ||
| logger.error(f"Failed to create Azure Monitor exporters for connection string: {e}") |
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.
This change swallows the exception and seems unrelated to the bug?
| print("✓ Helpful error messages guide users to install azure-monitor extra") | ||
|
|
||
|
|
||
| class TestOriginalIssueReproduction: |
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.
Why is there a test to reproduce the original failure?
| os.environ.pop('OTEL_PYTHON_CONFIGURATOR', None) | ||
|
|
||
|
|
||
| if __name__ == "__main__": |
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.
Why is there a script entry point for a test module?
|
|
||
| return exporters | ||
|
|
||
| def is_azure_monitor_available() -> bool: |
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.
This whole function can be inlined.
| if not _AZURE_MONITOR_AVAILABLE: | ||
| logger.warning( | ||
| "Azure Monitor connection string(s) provided but azure-monitor-opentelemetry not installed. " | ||
| "Install with: pip install agent-framework-core[azure-monitor]" |
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.
The mechanism to install the extra dependency should be uncoupled from the warning itself.
| except ImportError: | ||
| logger.debug( | ||
| "Azure Monitor OpenTelemetry not available. " | ||
| "Install with: pip install agent-framework-core[azure-monitor]" |
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.
Since the default log level is info this would silently fail in the scenario of the package being missing or the imported classes being renamed
|
|
||
| [project.optional-dependencies] | ||
| # NEW: Azure Monitor is now optional | ||
| azure-monitor = [ |
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.
I don't want a extra for this (I'm debating remove the 'viz' extra as well) just log the error message inside the create azure exporters message which packages should be installed, the rest, with all the extra comments and import attempts should be removed, and it should raise a exception, because if someone expects a exporter for azure to be setup and it isn't then we shouldn't continue
| Raises: | ||
| ImportError: If Azure Monitor packages are not installed | ||
| """ | ||
| if not _AZURE_MONITOR_AVAILABLE: |
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.
Try the imports here. And raise a exception with a message explaining which packages to install.
|
|
||
| # Check if Azure Monitor is available (add after line 60) | ||
| _AZURE_MONITOR_AVAILABLE = False | ||
| try: |
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.
Remove all this
| "OtelAttr", | ||
| "get_meter", | ||
| "get_tracer", | ||
| "is_azure_monitor_available", # NEW: Helper function to check Azure Monitor availability |
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.
Remove
| exporters.append(AzureMonitorLogExporter(connection_string=conn_string, credential=credential)) | ||
| exporters.append(AzureMonitorTraceExporter(connection_string=conn_string, credential=credential)) | ||
| exporters.append(AzureMonitorMetricExporter(connection_string=conn_string, credential=credential)) | ||
| except Exception as e: |
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.
Don't catch this, if there is something wrong with the connection string the user should know
| "mcp[ws]>=1.13", | ||
| "azure-monitor-opentelemetry>=1.7.0", | ||
| "azure-monitor-opentelemetry-exporter>=1.0.0b41", | ||
| # REMOVED: azure-monitor-opentelemetry - now optional |
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.
Remove
| "agent-framework-devui", | ||
| "graphviz>=0.20.0" | ||
| "graphviz>=0.20.0", | ||
| "azure-monitor-opentelemetry>=1.7.0", |
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.
Remove here as well
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.
Tests shouldn't be associated with an issue number.
| "OtelAttr", | ||
| "get_meter", | ||
| "get_tracer", | ||
| "is_azure_monitor_available", # NEW: Helper function to check Azure Monitor availability |
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.
Why does this need to be available outside of this module?
| # Now setup Agent Framework observability with OTLP | ||
| from agent_framework.observability import setup_observability | ||
|
|
||
| result = setup_observability( |
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.
You don't need to call setup_observability if you already call initialize
📋 Description
Fixes #1376 - Makes Azure Monitor OpenTelemetry packages optional dependencies to allow using OTEL auto-instrumentation with OTLP exporters (Aspire Dashboard, Jaeger, Zipkin, etc.) without requiring Azure Monitor.
🐛 Problem
The current implementation has hard dependencies on Azure Monitor OpenTelemetry packages:
azure-monitor-opentelemetry>=1.7.0azure-monitor-opentelemetry-exporter>=1.0.0b41This causes critical issues when using OTEL auto-instrumentation with OTLP endpoints:
ValueError: Instrumentation key cannot be none or emptyError Example (from issue #1376):
ValueError: Instrumentation key cannot be none or empty.
File "azure/monitor/opentelemetry/exporter/_connection_string_parser.py", line 103
text
Reproduction Code (from issue #1376):
from opentelemetry.instrumentation.auto_instrumentation import initialize
initialize() # ❌ FAILS with Azure Monitor error
from agent_framework.devui import serve
serve(entities=[workflow], port=8090) # Cannot reach this
text
✅ Solution
Changes Made
1. Updated
pyproject.tomlfiles[project.optional-dependencies]for azure-monitordependenciesto[project.optional-dependencies.azure-monitor]allextras group for backward compatibility2. Updated
observability.py_AZURE_MONITOR_AVAILABLEavailability check with try/except import_get_azure_monitor_exporters()check availability before importingis_azure_monitor_available()public helper function3. Added Comprehensive Tests
test_observability.py- General observability teststest_observability_issue_1376.py- Specific tests for issue Python: Dependency on Azure Monitor OpenTelemetry package means OTEL autoinstrumentation requires Azure Monitor #1376text
💻 Usage Examples
✅ Using with Aspire Dashboard (OTLP) - ISSUE #1376 FIXED:
import os
Disable Azure Monitor auto-configurator
os.environ['OTEL_PYTHON_CONFIGURATOR'] = ''
from opentelemetry.instrumentation.auto_instrumentation import initialize
initialize() # ✅ NOW WORKS without Azure Monitor
from agent_framework.observability import setup_observability
setup_observability(
enable_sensitive_data=True,
otlp_endpoint="http://localhost:4317/"
)
from agent_framework.devui import serve
serve(entities=[workflow], port=8090) # ✅ NOW WORKS
text
✅ Using with Azure Monitor (Backward Compatible):
First: pip install agent-framework-core[azure-monitor]
from agent_framework.observability import setup_observability
setup_observability(
enable_otel=True,
applicationinsights_connection_string="InstrumentationKey=...",
enable_sensitive_data=True
)
text
✅ Check Azure Monitor Availability:
from agent_framework.observability import is_azure_monitor_available
if is_azure_monitor_available():
print("Azure Monitor is available")
else:
print("Install with: pip install agent-framework-core[azure-monitor]")
text
🧪 Testing Performed
[azure-monitor]extra🔄 Breaking Changes
Technically breaking but easily mitigated:
Users who depend on Azure Monitor being automatically available will need to update their installation:
Change from:
pip install agent-framework-core
To:
pip install agent-framework-core[azure-monitor]
text
However:
"Install with: pip install agent-framework-core[azure-monitor]"✅ Checklist
🔗 Related Issues
Fixes #1376
📝 Additional Context
This change aligns with best practices for optional dependencies and makes the framework more flexible for different observability backends.
Benefits: