[BREAKING] Python: Enable instrumentation by default#5865
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Updates the Python Agent Framework and samples to reflect that instrumentation is now enabled by default, and shifts programmatic opt-in to a new enable_sensitive_telemetry() API for sensitive payload capture.
Changes:
- Make
ENABLE_INSTRUMENTATIONdefault to enabled (opt-out viafalse) and update docs/samples accordingly. - Replace most uses of
enable_instrumentation()withenable_sensitive_telemetry()for sensitive-data opt-in. - Update DevUI and Foundry integration points to reflect the new defaults/entry points.
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| python/samples/README.md | Updates env var table to reference observability settings instead of enable_instrumentation() |
| python/samples/04-hosting/foundry-hosted-agents/responses/07_observability/agent.yaml | Removes ENABLE_INSTRUMENTATION from sample agent config; keeps sensitive-data opt-in |
| python/samples/04-hosting/foundry-hosted-agents/responses/07_observability/agent.manifest.yaml | Removes ENABLE_INSTRUMENTATION from manifest sample |
| python/samples/04-hosting/foundry-hosted-agents/responses/07_observability/README.md | Documents instrumentation default-on and opt-out |
| python/samples/04-hosting/foundry-hosted-agents/responses/07_observability/.env.example | Removes ENABLE_INSTRUMENTATION example line |
| python/samples/03-workflows/observability/executor_io_observation.py | Wording update to reflect default instrumentation |
| python/samples/02-agents/observability/advanced_manual_setup_console_output.py | Switches to enable_sensitive_telemetry() for sensitive data capture |
| python/samples/02-agents/observability/README.md | Updates guidance/code samples to default-on instrumentation + sensitive opt-in |
| python/samples/02-agents/observability/.env.example | Documents opt-out via ENABLE_INSTRUMENTATION=false |
| python/packages/lab/lightning/agent_framework_lab_lightning/init.py | Removes explicit enable_instrumentation() call from tracer init |
| python/packages/foundry/tests/foundry/test_foundry_chat_client.py | Updates mocks to enable_sensitive_telemetry() |
| python/packages/foundry/tests/foundry/test_foundry_agent.py | Updates mocks to enable_sensitive_telemetry() |
| python/packages/foundry/agent_framework_foundry/_chat_client.py | Calls enable_sensitive_telemetry() only when sensitive data is requested |
| python/packages/foundry/agent_framework_foundry/_agent.py | Calls enable_sensitive_telemetry() only when sensitive data is requested |
| python/packages/devui/frontend/src/components/layout/deployment-modal.tsx | Updates docker-compose snippet to reflect default-on instrumentation |
| python/packages/devui/agent_framework_devui/_server.py | Treats instrumentation as enabled unless explicitly set to false |
| python/packages/devui/agent_framework_devui/_executor.py | Updates comment to reflect new defaults/entry point |
| python/packages/devui/agent_framework_devui/init.py | Removes instrumentation_enabled parameter and related logic |
| python/packages/core/tests/core/test_observability.py | Updates tests to cover the new enable_sensitive_telemetry() behavior |
| python/packages/core/agent_framework/observability.py | Defaults instrumentation to enabled; replaces enable_instrumentation() with enable_sensitive_telemetry() |
| python/CODING_STANDARD.md | Updates import example to enable_sensitive_telemetry |
| python/.github/skills/python-development/SKILL.md | Updates skills doc sample import to enable_sensitive_telemetry |
| python/.env.example | Updates env example to reflect default-on instrumentation |
There was a problem hiding this comment.
Automated Code Review
Reviewers: 4 | Confidence: 90%
✗ Correctness
The PR correctly implements the 'enable instrumentation by default' change in ObservabilitySettings and renames enable_instrumentation to enable_sensitive_telemetry. However, beyond the already-flaged issues in the review thread, the configure_azure_monitor() methods in both _agent.py and _chat_client.py have a security-relevant behavioral regression: the old code always actively set enable_sensitive_data to False by default (overriding the env var), whereas the new code is a no-op when enable_sensitive_data=False, meaning ENABLE_SENSITIVE_DATA=true in the environment will no longer be overidden, potentially leaking sensitive telemetry data in production.
✓ Security Reliability
The PR correctly changes the default for
enable_instrumentationfrom False to True, with sound env-var parsing logic. Sensitive data capture remains opt-in. All existing references toenable_instrumentationoutside the diff are either pytest fixtures or attribute-level monkeypatches onOBSERVABILITY_SETTINGS(not calls to the removed function), so they won't break. The six existing unresolved review comments already cover the significant concerns: the breaking removal ofenable_instrumentation()from the public API, theconfigure_azure_monitorregression whenenable_sensitive_data=False, the DevUI boolean parsing inconsistency, the deployment modal comment mismatch, and the DevUIserve()parameter removal. No additional security or reliability defects were found.
✓ Test Coverage
The PR renames
enable_instrumentation()toenable_sensitive_telemetry()and changes the default forenable_instrumentationfromFalsetoTrue. Test renames and adaptations are correct, but there are two notable test coverage gaps: (1) no test verifies the new defaultenable_instrumentation=Truewhen theENABLE_INSTRUMENTATIONenv var is unset — this is the core behavioral change of the PR, and (2) neither foundry test file covers theconfigure_azure_monitor(enable_sensitive_data=False)path, leaving the newif enable_sensitive_data:guard untested for the false branch.
✗ Design Approach
The main design issue is in the Agent Lightning integration: this PR removes the explicit instrumentation enablement from
AgentFrameworkTracer.init(), which means that a user who hasENABLE_INSTRUMENTATION=falsewill no longer get Agent Framework spans through the Lightning tracer even though that tracer is documented as the component that enables Agent Framework observability for Lightning.
Flagged Issues
- configure_azure_monitor() no longer overrides ENABLE_SENSITIVE_DATA=true from the environment when called with the default enable_sensitive_data=False. The old code always called enable_instrumentation(enable_sensitive_data=False), which forcefully set OBSERVABILITY_SETTINGS.enable_sensitive_data=False. The new
if enable_sensitive_data:guard skips the call entirely, leaving env-sourced enable_sensitive_data=True in effect. This is a security regression: sensitive data (prompts, tool arguments) may leak in production when users expect configure_azure_monitor() to suppress it. - AgentFrameworkTracer no longer guarantees Agent Framework telemetry is enabled for the Lightning integration. With ENABLE_INSTRUMENTATION=false, observability gates become no-ops, which contradicts the Lightning tracer's documented contract to enable OpenTelemetry observability for Agent-lightning and the README guidance that instantiating AgentFrameworkTracer() sends telemetry to Lightning.
Automated review by TaoChenOSU's agents
Motivation and Context
Closes #5749
Description
Changes required to make the code base consistent throughout.
Contribution Checklist