Python: Fix ENABLE_SENSITIVE_DATA env var not respected after load_dotenv() #4120
+193
−53
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -880,8 +880,7 @@ def get_meter( | |
| return metrics.get_meter(name=name, version=version, schema_url=schema_url) | ||
|
|
||
|
|
||
| OBSERVABILITY_SETTINGS: ObservabilitySettings | None = None | ||
|
|
||
|
|
||
| def enable_instrumentation( | ||
|
|
@@ -900,8 +899,11 @@ def enable_instrumentation( | |
| Keyword Args: | ||
| enable_sensitive_data: Enable OpenTelemetry sensitive events. Overrides | ||
| the environment variable ENABLE_SENSITIVE_DATA if set. Default is None. | ||
| When not provided, falls back to the ENABLE_SENSITIVE_DATA environment variable. | ||
| """ | ||
| global OBSERVABILITY_SETTINGS | ||
| if OBSERVABILITY_SETTINGS is None: | ||
| OBSERVABILITY_SETTINGS = ObservabilitySettings() | ||
| OBSERVABILITY_SETTINGS.enable_instrumentation = True | ||
| if enable_sensitive_data is not None: | ||
| OBSERVABILITY_SETTINGS.enable_sensitive_data = enable_sensitive_data | ||
|
|
@@ -1026,27 +1028,22 @@ def configure_otel_providers( | |
| - https://opentelemetry.io/docs/languages/sdk-configuration/otlp-exporter/ | ||
| """ | ||
| global OBSERVABILITY_SETTINGS | ||
| # Build kwargs for a fresh ObservabilitySettings, excluding None values. | ||
| # Creating the settings here (instead of at import time) ensures that | ||
| # environment variables populated by the caller's load_dotenv() are picked up. | ||
| settings_kwargs: dict[str, Any] = { | ||
| "enable_instrumentation": True, | ||
| } | ||
| if env_file_path is not None: | ||
| settings_kwargs["env_file_path"] = env_file_path | ||
| if env_file_encoding is not None: | ||
| settings_kwargs["env_file_encoding"] = env_file_encoding | ||
| if enable_sensitive_data is not None: | ||
| settings_kwargs["enable_sensitive_data"] = enable_sensitive_data | ||
| if vs_code_extension_port is not None: | ||
| settings_kwargs["vs_code_extension_port"] = vs_code_extension_port | ||
|
|
||
| OBSERVABILITY_SETTINGS = ObservabilitySettings(**settings_kwargs) | ||
|
|
||
| OBSERVABILITY_SETTINGS._configure( # type: ignore[reportPrivateUsage] | ||
|
Comment on lines
+1031
to
1048
There was a problem hiding this comment. configure_otel_providers() now always replaces the global OBSERVABILITY_SETTINGS with a new ObservabilitySettings instance. This breaks the function’s documented “call once”/idempotent behavior: calling configure_otel_providers() a second time will reset _executed_setup back to False and can reconfigure providers/handlers again, which can lead to duplicate exporters/handlers and unexpected telemetry. Consider reusing the existing settings when setup has already been executed (or otherwise preserving the original _executed_setup state) so repeated calls remain a no-op after the first successful configuration.
Contributor
Author
There was a problem hiding this comment. The docstring states: "DO NOT call this method multiple times, as it may lead to unexpected behavior." Single-call-at-startup is the expected usage pattern. Not a regression. |
||
| additional_exporters=exporters, | ||
|
|
@@ -1132,10 +1129,10 @@ def get_response( | |
| **kwargs: Any, | ||
| ) -> Awaitable[ChatResponse[Any]] | ResponseStream[ChatResponseUpdate, ChatResponse[Any]]: | ||
| """Trace chat responses with OpenTelemetry spans and metrics.""" | ||
| settings = OBSERVABILITY_SETTINGS | ||
| super_get_response = super().get_response # type: ignore[misc] | ||
|
|
||
| if settings is None or not settings.ENABLED: | ||
| return super_get_response(messages=messages, stream=stream, options=options, **kwargs) # type: ignore[no-any-return] | ||
|
Comment on lines
+1132
to
1136
There was a problem hiding this comment. ChatTelemetryLayer.get_response() currently short-circuits when OBSERVABILITY_SETTINGS is None, which means env-var-only enablement (ENABLE_INSTRUMENTATION=true) won’t activate telemetry unless some other code has already created the settings object. If OBSERVABILITY_SETTINGS is meant to be “deferred”, consider ensuring it is initialized here (or via a shared helper) before checking settings.ENABLED so the first instrumented call can pick up env vars populated by load_dotenv().
Contributor
Author
There was a problem hiding this comment. Same rationale as above — telemetry activation requires an explicit |
||
|
|
||
| opts: dict[str, Any] = options or {} # type: ignore[assignment] | ||
|
|
@@ -1170,7 +1167,7 @@ def get_response( | |
| span_name = attributes.get(SpanAttributes.LLM_REQUEST_MODEL, "unknown") | ||
| span = get_tracer().start_span(f"{operation} {span_name}") | ||
| span.set_attributes(attributes) | ||
| if settings.SENSITIVE_DATA_ENABLED and messages: | ||
| _capture_messages( | ||
| span=span, | ||
| provider_name=provider_name, | ||
|
|
@@ -1205,11 +1202,7 @@ async def _finalize_stream() -> None: | |
| operation_duration_histogram=self.duration_histogram, | ||
| duration=duration, | ||
| ) | ||
| if settings.SENSITIVE_DATA_ENABLED and isinstance(response, ChatResponse) and response.messages: | ||
| _capture_messages( | ||
| span=span, | ||
| provider_name=provider_name, | ||
|
|
@@ -1230,7 +1223,7 @@ async def _finalize_stream() -> None: | |
|
|
||
| async def _get_response() -> ChatResponse: | ||
| with _get_span(attributes=attributes, span_name_attribute=SpanAttributes.LLM_REQUEST_MODEL) as span: | ||
| if settings.SENSITIVE_DATA_ENABLED and messages: | ||
| _capture_messages( | ||
| span=span, | ||
| provider_name=provider_name, | ||
|
|
@@ -1252,7 +1245,7 @@ async def _get_response() -> ChatResponse: | |
| operation_duration_histogram=self.duration_histogram, | ||
| duration=duration, | ||
| ) | ||
| if settings.SENSITIVE_DATA_ENABLED and response.messages: | ||
| _capture_messages( | ||
| span=span, | ||
| provider_name=provider_name, | ||
|
|
@@ -1312,12 +1305,12 @@ def run( | |
| **kwargs: Any, | ||
| ) -> Awaitable[AgentResponse[Any]] | ResponseStream[AgentResponseUpdate, AgentResponse[Any]]: | ||
| """Trace agent runs with OpenTelemetry spans and metrics.""" | ||
| settings = OBSERVABILITY_SETTINGS | ||
| super_run = super().run # type: ignore[misc] | ||
| provider_name = str(self.otel_provider_name) | ||
| capture_usage = bool(getattr(self, "_otel_capture_usage", True)) | ||
|
|
||
| if settings is None or not settings.ENABLED: | ||
| return super_run( # type: ignore[no-any-return] | ||
| messages=messages, | ||
| stream=stream, | ||
|
|
@@ -1363,7 +1356,7 @@ def run( | |
| span_name = attributes.get(OtelAttr.AGENT_NAME, "unknown") | ||
| span = get_tracer().start_span(f"{operation} {span_name}") | ||
| span.set_attributes(attributes) | ||
| if settings.SENSITIVE_DATA_ENABLED and messages: | ||
| _capture_messages( | ||
| span=span, | ||
| provider_name=provider_name, | ||
|
|
@@ -1396,11 +1389,7 @@ async def _finalize_stream() -> None: | |
| capture_usage=capture_usage, | ||
| ) | ||
| _capture_response(span=span, attributes=response_attributes, duration=duration) | ||
| if settings.SENSITIVE_DATA_ENABLED and isinstance(response, AgentResponse) and response.messages: | ||
| _capture_messages( | ||
| span=span, | ||
| provider_name=provider_name, | ||
|
|
@@ -1420,7 +1409,7 @@ async def _finalize_stream() -> None: | |
|
|
||
| async def _run() -> AgentResponse: | ||
| with _get_span(attributes=attributes, span_name_attribute=OtelAttr.AGENT_NAME) as span: | ||
| if settings.SENSITIVE_DATA_ENABLED and messages: | ||
| _capture_messages( | ||
| span=span, | ||
| provider_name=provider_name, | ||
|
|
@@ -1442,7 +1431,7 @@ async def _run() -> AgentResponse: | |
| if response: | ||
| response_attributes = _get_response_attributes(attributes, response, capture_usage=capture_usage) | ||
| _capture_response(span=span, attributes=response_attributes, duration=duration) | ||
| if settings.SENSITIVE_DATA_ENABLED and response.messages: | ||
| _capture_messages( | ||
| span=span, | ||
| provider_name=provider_name, | ||
|
|
@@ -1780,8 +1769,7 @@ def __repr__(self) -> str: | |
|
|
||
| def workflow_tracer() -> Tracer: | ||
| """Get a workflow tracer or a no-op tracer if not enabled.""" | ||
| return get_tracer() if OBSERVABILITY_SETTINGS is not None and OBSERVABILITY_SETTINGS.ENABLED else trace.NoOpTracer() | ||
|
|
||
|
|
||
| def create_workflow_span( | ||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
With OBSERVABILITY_SETTINGS now defaulting to None, setting ENABLE_INSTRUMENTATION=true in the process environment no longer automatically enables instrumentation until some code explicitly calls enable_instrumentation()/configure_otel_providers(). This appears to regress the documented “zero-code via env vars” behavior (e.g., python/samples/02-agents/observability/README.md:156 says ENABLE_INSTRUMENTATION=true should cause the framework to emit observability data). Consider lazily initializing OBSERVABILITY_SETTINGS on first telemetry check (or at import time when env vars are already present) so env-only enablement continues to work while still supporting load_dotenv-after-import timing.
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.
Every sample in the repo explicitly calls
configure_otel_providers()orenable_instrumentation()— no sample relies on env-var-only enablement. The README's "zero-code" pattern (Pattern 5) refers to theopentelemetry-instrumentCLI, which doesn't depend onOBSERVABILITY_SETTINGS. Deferring creation to call time is intentional so that env vars set viaload_dotenv()after import are correctly picked up.