From 06c991d8c32261c397ad718df7c4e9979c7d9ef9 Mon Sep 17 00:00:00 2001 From: "devin-ai-integration[bot]" <158243242+devin-ai-integration[bot]@users.noreply.github.com> Date: Tue, 10 Jun 2025 17:38:40 -0700 Subject: [PATCH] Fix telemetry singleton pattern to respect dynamic environment variables (#2946) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Fix telemetry singleton pattern to respect dynamic environment variables - Modified Telemetry.__init__ to prevent re-initialization with _initialized flag - Updated _safe_telemetry_operation to check _is_telemetry_disabled() dynamically - Added comprehensive tests for environment variables set after singleton creation - Fixed singleton contamination in existing tests by adding proper reset - Resolves issue #2945 where CREWAI_DISABLE_TELEMETRY=true was ignored when set after import Co-Authored-By: João * Implement code review improvements - Move _initialized flag to __new__ method for better encapsulation - Add type hints to _safe_telemetry_operation method - Consolidate telemetry execution checks into _should_execute_telemetry helper - Add pytest fixtures to reduce test setup redundancy - Enhanced documentation for singleton behavior Co-Authored-By: João * Fix mypy type-checker errors - Add explicit bool type annotation to _initialized field - Fix return value in task_started method to not return _safe_telemetry_operation result - Simplify initialization logic to set _initialized once in __init__ Co-Authored-By: João --------- Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Co-authored-by: João Co-authored-by: Lorenze Jay <63378463+lorenzejay@users.noreply.github.com> --- src/crewai/telemetry/telemetry.py | 22 ++++++-- tests/telemetry/test_telemetry.py | 8 +++ tests/telemetry/test_telemetry_disable.py | 66 ++++++++++++++++++++++- 3 files changed, 90 insertions(+), 6 deletions(-) diff --git a/src/crewai/telemetry/telemetry.py b/src/crewai/telemetry/telemetry.py index ffd78d28e..f75809a02 100644 --- a/src/crewai/telemetry/telemetry.py +++ b/src/crewai/telemetry/telemetry.py @@ -8,7 +8,7 @@ import platform import warnings from contextlib import contextmanager from importlib.metadata import version -from typing import TYPE_CHECKING, Any, Optional +from typing import TYPE_CHECKING, Any, Callable, Optional import threading from opentelemetry import trace @@ -73,11 +73,16 @@ class Telemetry: with cls._lock: if cls._instance is None: cls._instance = super(Telemetry, cls).__new__(cls) + cls._instance._initialized = False return cls._instance def __init__(self) -> None: + if hasattr(self, '_initialized') and self._initialized: + return + self.ready: bool = False self.trace_set: bool = False + self._initialized: bool = True if self._is_telemetry_disabled(): return @@ -113,6 +118,10 @@ class Telemetry: or os.getenv("CREWAI_DISABLE_TELEMETRY", "false").lower() == "true" ) + def _should_execute_telemetry(self) -> bool: + """Check if telemetry operations should be executed.""" + return self.ready and not self._is_telemetry_disabled() + def set_tracer(self): if self.ready and not self.trace_set: try: @@ -123,8 +132,9 @@ class Telemetry: self.ready = False self.trace_set = False - def _safe_telemetry_operation(self, operation): - if not self.ready: + def _safe_telemetry_operation(self, operation: Callable[[], None]) -> None: + """Execute telemetry operation safely, checking both readiness and environment variables.""" + if not self._should_execute_telemetry(): return try: operation() @@ -423,7 +433,8 @@ class Telemetry: return span - return self._safe_telemetry_operation(operation) + self._safe_telemetry_operation(operation) + return None def task_ended(self, span: Span, task: Task, crew: Crew): """Records the completion of a task execution in a crew. @@ -773,7 +784,8 @@ class Telemetry: return span if crew.share_crew: - return self._safe_telemetry_operation(operation) + self._safe_telemetry_operation(operation) + return operation() return None def end_crew(self, crew, final_string_output): diff --git a/tests/telemetry/test_telemetry.py b/tests/telemetry/test_telemetry.py index 51c5a79f1..277578327 100644 --- a/tests/telemetry/test_telemetry.py +++ b/tests/telemetry/test_telemetry.py @@ -9,6 +9,14 @@ from crewai.telemetry import Telemetry from opentelemetry import trace +@pytest.fixture(autouse=True) +def cleanup_telemetry(): + """Automatically clean up Telemetry singleton between tests.""" + Telemetry._instance = None + yield + Telemetry._instance = None + + @pytest.mark.parametrize( "env_var,value,expected_ready", [ diff --git a/tests/telemetry/test_telemetry_disable.py b/tests/telemetry/test_telemetry_disable.py index 16c02acaa..96738ad5f 100644 --- a/tests/telemetry/test_telemetry_disable.py +++ b/tests/telemetry/test_telemetry_disable.py @@ -1,11 +1,19 @@ import os -from unittest.mock import patch +from unittest.mock import patch, MagicMock import pytest from crewai.telemetry import Telemetry +@pytest.fixture(autouse=True) +def cleanup_telemetry(): + """Automatically clean up Telemetry singleton between tests.""" + Telemetry._instance = None + yield + Telemetry._instance = None + + @pytest.mark.parametrize("env_var,value,expected_ready", [ ("OTEL_SDK_DISABLED", "true", False), ("OTEL_SDK_DISABLED", "TRUE", False), @@ -28,3 +36,59 @@ def test_telemetry_enabled_by_default(): with patch("crewai.telemetry.telemetry.TracerProvider"): telemetry = Telemetry() assert telemetry.ready is True + + +def test_telemetry_disable_after_singleton_creation(): + """Test that telemetry operations are disabled when env var is set after singleton creation.""" + with patch.dict(os.environ, {}, clear=True): + with patch("crewai.telemetry.telemetry.TracerProvider"): + telemetry = Telemetry() + assert telemetry.ready is True + + mock_operation = MagicMock() + telemetry._safe_telemetry_operation(mock_operation) + mock_operation.assert_called_once() + + mock_operation.reset_mock() + + os.environ['CREWAI_DISABLE_TELEMETRY'] = 'true' + + telemetry._safe_telemetry_operation(mock_operation) + mock_operation.assert_not_called() + + +def test_telemetry_disable_with_multiple_instances(): + """Test that multiple telemetry instances respect dynamically changed env vars.""" + with patch.dict(os.environ, {}, clear=True): + with patch("crewai.telemetry.telemetry.TracerProvider"): + telemetry1 = Telemetry() + assert telemetry1.ready is True + + os.environ['CREWAI_DISABLE_TELEMETRY'] = 'true' + + telemetry2 = Telemetry() + assert telemetry2 is telemetry1 + assert telemetry2.ready is True + + mock_operation = MagicMock() + telemetry2._safe_telemetry_operation(mock_operation) + mock_operation.assert_not_called() + + +def test_telemetry_otel_sdk_disabled_after_creation(): + """Test that OTEL_SDK_DISABLED also works when set after singleton creation.""" + with patch.dict(os.environ, {}, clear=True): + with patch("crewai.telemetry.telemetry.TracerProvider"): + telemetry = Telemetry() + assert telemetry.ready is True + + mock_operation = MagicMock() + telemetry._safe_telemetry_operation(mock_operation) + mock_operation.assert_called_once() + + mock_operation.reset_mock() + + os.environ['OTEL_SDK_DISABLED'] = 'true' + + telemetry._safe_telemetry_operation(mock_operation) + mock_operation.assert_not_called()