Python: Fix ENABLE_SENSITIVE_DATA env var ignored when set after module import

[eavanvalkenburg]

Motivation and Context

When users call load_dotenv() after importing the agent_framework.observability module, environment variables like ENABLE_SENSITIVE_DATA and VS_CODE_EXTENSION_PORT were ignored because ObservabilitySettings was only read once at import time. This meant the module-level singleton retained stale values even after the environment changed.

Fixes #4119

Description

The root cause was that enable_instrumentation() and configure_otel_providers() only updated settings when explicit parameters were passed, falling back to the already-stale module-level OBSERVABILITY_SETTINGS singleton otherwise. The fix re-reads environment variables by constructing a fresh ObservabilitySettings() instance at call time, using those refreshed values as defaults when no explicit parameter is provided. Explicit parameters still take precedence over environment variables. Tests verify the re-read behavior for ENABLE_SENSITIVE_DATA, VS_CODE_EXTENSION_PORT, and that explicit arguments override env vars.

Contribution Checklist

• The code builds clean without any errors or warnings
• The PR follows the Contribution Guidelines
• All unit tests pass, and I have added new tests where possible
• Is this a breaking change? If yes, add "[BREAKING]" prefix to the title of the PR.

---

Note: PR autogenerated by eavanvalkenburg's agent

[markwallace-microsoft]

Python Test Coverage Report •

File	Stmts	Miss	Cover	Missing
packages/core/agent_framework
observability.py	693	25	96%	370–371, 398, 400–402, 405–407, 412–413, 419–420, 426–427, 717, 917–918, 1080, 1325–1326, 1572, 1754, 2138, 2140
TOTAL	24023	2634	89%

Python Unit Test Overview

Tests	Skipped	Failures	Errors	Time
5247	20 💤	0 ❌	0 🔥	1m 23s ⏱️

[Copilot]

Pull request overview

Fixes a bug in the Python core observability module where ENABLE_SENSITIVE_DATA (and related env-driven settings) could be ignored if environment variables were set after agent_framework.observability was imported (e.g., via load_dotenv()), by refreshing env-derived defaults at call time.

Changes:

• Update enable_instrumentation() to re-read ENABLE_SENSITIVE_DATA from the current environment when no explicit argument is provided.
• Update configure_otel_providers() to re-read env-derived defaults (notably sensitive data and VS Code port) when no explicit arguments are provided.
• Add regression tests covering env re-read behavior and explicit-parameter precedence.

Reviewed changes

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

File	Description
python/packages/core/agent_framework/observability.py	Refresh env-derived defaults at call time instead of relying solely on import-time singleton values.
python/packages/core/tests/core/test_observability.py	Add tests verifying env re-read behavior for sensitive data and VS Code extension port.

---

You can also share your feedback on Copilot code review. Take the survey.

[eavanvalkenburg]

Automated Code Review

Reviewers: 4 | Confidence: 88%

✓ Correctness

The changes correctly implement re-reading environment variables when enable_instrumentation() and configure_otel_providers() are called without explicit parameters, addressing the use case where load_dotenv() sets env vars after module import. The logic for explicit-parameter-overrides-env is sound, and the new tests cover the key scenarios. No correctness issues found.

✓ Security Reliability

The changes allow enable_instrumentation() and configure_otel_providers() to re-read environment variables at call time instead of relying only on import-time values, supporting load_dotenv() workflows. The security-sensitive enable_sensitive_data flag correctly preserves explicit-parameter-takes-precedence semantics. No injection risks, resource leaks, or unsafe deserialization found. One minor behavioral change—enable_console_exporters is now unconditionally refreshed from the environment in the else branch of configure_otel_providers, which was not previously updated there—is consistent with the env_file_path branch but lacks test coverage.

✗ Test Coverage

The new tests cover the main happy-path scenarios for the env-var re-read behaviour in both enable_instrumentation and configure_otel_providers. However, the newly added enable_console_exporters refresh line in configure_otel_providers has no test coverage at all, and there is no test verifying that enable_instrumentation(enable_sensitive_data=False) still takes precedence over a truthy env var (the symmetric explicit-override case for that function).

✗ Design Approach

The change fixes a real problem — OBSERVABILITY_SETTINGS is constructed at module import time (line 899), so env vars set later via load_dotenv() are invisible to it. The fix is a symptom-level workaround: it instantiates a fresh ObservabilitySettings() inside each public API function to re-read env vars. This works only for the two code paths covered, but ObservabilitySettings is a public global, so any caller accessing it directly still sees the stale values. Additionally, ObservabilitySettings() construction calls create_resource(), which builds an OTel Resource object that is immediately discarded; the fix uses heavy object creation as a mechanism for what is essentially an env-var read. The root cause — eager construction of the singleton at import time — is not addressed. A cleaner fix would be to make the env-var-backed fields lazy properties on ObservabilitySettings (reading from os.environ on access), or to delay singleton construction to first use. There is also a minor inconsistency: enable_instrumentation() refreshes enable_sensitive_data but not enable_console_exporters, whereas configure_otel_providers() refreshes both; if the user sets ENABLE_CONSOLE_EXPORTERS after import and only calls enable_instrumentation(), that flag is silently ignored.

Flagged Issues

• No test covers the new OBSERVABILITY_SETTINGS.enable_console_exporters = refreshed.enable_console_exporters line (~line 1077 in observability.py). If this line were deleted or regressed, no test would fail.
• The fix patches only the two public API code paths; OBSERVABILITY_SETTINGS remains a module-level public global whose env-var-backed fields are stale for any direct caller. The root cause—eager singleton construction at import time—is not addressed. Consider making these fields lazy properties that read os.environ on access, or lazily constructing the singleton on first use.
• Each call to enable_instrumentation() or configure_otel_providers() now instantiates a full ObservabilitySettings() whose constructor also calls create_resource(), building an OTel Resource that is immediately discarded. Settings parsing should be separated from resource creation so the refresh pattern does not carry unnecessary side effects.

Suggestions

• Add a test for enable_instrumentation(enable_sensitive_data=False) when ENABLE_SENSITIVE_DATA=true is in the environment, to verify the explicit parameter still wins (mirrors the existing test_configure_otel_providers_explicit_param_overrides_env but for the simpler function).
• In enable_instrumentation(), enable_console_exporters is not refreshed from the environment (only enable_sensitive_data is), whereas configure_otel_providers() refreshes both. If a user sets ENABLE_CONSOLE_EXPORTERS after import and calls only enable_instrumentation(), the env var is silently ignored. Either document this intentional asymmetry or refresh enable_console_exporters in both functions.
• The tests use importlib.reload(observability), which re-runs module-level code but can leave stale references in other already-imported modules. This fragility should be noted if the module gains cross-module references.

---

Automated review by eavanvalkenburg's agents

[eavanvalkenburg]

Automated Code Review

Reviewers: 4 | Confidence: 92%

✓ Correctness

The change correctly adds a re-read of ENABLE_CONSOLE_EXPORTERS from the environment inside enable_instrumentation(), mirroring the existing pattern for ENABLE_SENSITIVE_DATA. The new line in the existing test (delenv for ENABLE_CONSOLE_EXPORTERS) prevents test pollution from the new behavior. The new test is well-structured and follows the same pattern as the surrounding tests. No correctness issues found.

✓ Security Reliability

The change adds a single line to enable_instrumentation() that refreshes the enable_console_exporters setting from the environment, matching the existing pattern used for enable_sensitive_data. The helper _read_bool_env performs a safe os.getenv followed by a lowercase string comparison—no injection, deserialization, or resource-leak risk. The new test properly isolates environment variables via monkeypatch and validates the round-trip. The existing test gains a defensive delenv for the new variable. No security or reliability issues found.

✓ Test Coverage

The diff adds a one-line production fix to refresh enable_console_exporters from the environment inside enable_instrumentation(), plus a well-structured test that covers the primary scenario (env var absent at import, set before calling enable_instrumentation). The existing test_configure_otel_providers_reads_env_vs_code_port test also gets a minor cleanup to delete ENABLE_CONSOLE_EXPORTERS to avoid cross-contamination. The new test follows the same reload-and-assert pattern used by sibling tests and its assertions are meaningful.

✗ Design Approach

The fix unconditionally overwrites OBSERVABILITY_SETTINGS.enable_console_exporters from the environment every time enable_instrumentation() is called. This is problematic for two reasons. First, it is a leaky abstraction: enable_instrumentation() is documented as a function that only flips the instrumentation flag and does not configure exporters or providers; folding console-exporter state management into it bleeds a provider-configuration concern across a documented layer boundary. Second, configure_otel_providers() already correctly re-reads ENABLE_CONSOLE_EXPORTERS from the environment at call time (lines 1102–1106), so for the primary load_dotenv use case the fix is redundant on that path. On the enable_instrumentation()-only path (users who wire their own providers, e.g. Azure Monitor), the value written is never read by any exporter-creation code—enable_console_exporters is only consumed inside _configure(), which enable_instrumentation() never calls. The unconditional overwrite also introduces an asymmetry: enable_sensitive_data can be protected by passing it explicitly, but there is no corresponding enable_console_exporters parameter to enable_instrumentation(), so any value previously set programmatically (e.g., via a prior configure_otel_providers(enable_console_exporters=True) call) can be silently clobbered by a subsequent enable_instrumentation() call when the env var is absent.

Flagged Issues

• Unconditional overwrite in enable_instrumentation() silently resets enable_console_exporters to False whenever the env var is absent, clobbering any value previously set by configure_otel_providers(enable_console_exporters=True). This is a destructive side-effect on a function documented as only enabling the instrumentation flag.
• enable_console_exporters is only consumed inside _configure(), which enable_instrumentation() never calls. The refresh has no effect on the enable_instrumentation()-only code path and is redundant for the configure_otel_providers() path (which already re-reads the env var), so it does not solve a real problem but introduces regression risk.

Suggestions

• If the goal is to let enable_instrumentation() influence console exporters, add an explicit enable_console_exporters: bool | None = None parameter (mirroring enable_sensitive_data) with a conditional pattern: use the explicit value when provided, fall back to _read_bool_env otherwise. This preserves caller intent and maintains API symmetry. If the goal is solely to fix the load_dotenv timing issue, no change to enable_instrumentation() is needed—configure_otel_providers() already re-reads the env var at call time.
• Consider adding a reverse-direction test (set ENABLE_CONSOLE_EXPORTERS=true before reload, then delete it, call enable_instrumentation(), and assert the setting reverts to False) to verify the re-read works in both directions.
• Consider adding a test that verifies enable_console_exporters is refreshed even when enable_sensitive_data is explicitly passed, since the new line runs unconditionally regardless of that branch.

---

Automated review by eavanvalkenburg's agents

[eavanvalkenburg]

Automated Code Review

Reviewers: 4 | Confidence: 94%

✓ Correctness

The diff correctly removes the side-effect where enable_instrumentation() was unconditionally overwriting enable_console_exporters from the environment, which could clobber a value previously set by configure_otel_providers(). The responsibility for managing enable_console_exporters is properly scoped to configure_otel_providers(), which still reads the env var (lines 1100-1104). The four new tests thoroughly verify the fix: env var set after import is not picked up, prior configure_otel_providers value is preserved, explicit enable_sensitive_data param doesn't clobber, and env var removal after reload doesn't reset. No correctness issues found.

✓ Security Reliability

This diff removes a line from enable_instrumentation() that was unconditionally overwriting OBSERVABILITY_SETTINGS.enable_console_exporters by re-reading from the environment. This was a bug: it clobbered values previously set by configure_otel_providers(). The fix is correct—enable_console_exporters is an exporter concern managed exclusively by configure_otel_providers, which already handles both explicit parameters and env-var fallback (lines 1079-1104). The new tests thoroughly verify that enable_instrumentation no longer touches the console-exporters setting. No security or reliability issues found.

✓ Test Coverage

The diff removes a two-line side-effect from enable_instrumentation that was clobbering enable_console_exporters, and replaces one existing test with four well-targeted tests covering the new behavior. The original test was updated to assert the opposite outcome (value stays False instead of being set to True), and three new tests cover: clobbering after configure_otel_providers, clobbering when enable_sensitive_data is passed, and preservation when the env var is removed post-reload. Assertions are meaningful and specific. The test coverage for the behavioral change is thorough.

✓ Design Approach

The diff removes an unconditional re-read of ENABLE_CONSOLE_EXPORTERS from enable_instrumentation(), which previously clobbered any value set by configure_otel_providers. This is a well-motivated fix: enable_instrumentation() is documented as not configuring exporters or providers, yet the removed code was silently overwriting the exporter-level setting on every call, making it impossible to preserve a value set by configure_otel_providers(enable_console_exporters=True). The fix correctly narrows enable_instrumentation to its stated contract. The four new tests are clear and cover the key scenarios (env var post-reload, clobbering a prior configure call, combined with explicit sensitive_data param). No blocking design issues found.

Suggestions

• The first test (test_enable_instrumentation_does_not_touch_console_exporters) cleans up fewer env vars than the other three new tests (it doesn't clear ENABLE_SENSITIVE_DATA, VS_CODE_EXTENSION_PORT, or the OTEL_EXPORTER_OTLP_* vars). While this doesn't cause a correctness issue today, aligning the env cleanup across all four tests would make them more robust against future refactors.
• The same clobbering risk now exists for enable_sensitive_data: calling configure_otel_providers(enable_sensitive_data=True) followed by enable_instrumentation() (without the explicit param) resets it to the env-var value. This is pre-existing and outside this PR's scope, but worth a follow-up to make the two settings consistent in how they handle values already set by configure_otel_providers.

---

Automated review by eavanvalkenburg's agents