mirror of
https://github.com/crewAIInc/crewAI.git
synced 2025-12-16 04:18:35 +00:00
Fix telemetry singleton pattern to respect dynamic environment variables (#2946)
* 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 <joao@crewai.com> * 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 <joao@crewai.com> * 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 <joao@crewai.com> --------- Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Co-authored-by: João <joao@crewai.com> Co-authored-by: Lorenze Jay <63378463+lorenzejay@users.noreply.github.com>
This commit is contained in:
committed by
GitHub
parent
739eb72fd0
commit
06c991d8c3
@@ -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):
|
||||
|
||||
@@ -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",
|
||||
[
|
||||
|
||||
@@ -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()
|
||||
|
||||
Reference in New Issue
Block a user