Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Copy of PR #435 - make start_session non blocking #23

Closed
wants to merge 24 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions agentops/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@

if "autogen" in sys.modules:
Client().configure(instrument_llm_calls=False)
Client()._initialize_autogen_logger()
Client().add_default_tags(["autogen"])

if "crewai" in sys.modules:
Comment on lines 22 to 28

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Entelligence AI Bot Icon Entelligence AI Bot v4

ℹ️ Performance Improvement

Initialize Autogen Logger for Enhanced Logging

The addition of _initialize_autogen_logger() ensures that the logging mechanism is set up correctly when the autogen module is present. This change enhances the logging capabilities, allowing for better tracking and debugging of operations related to the autogen module.

Commitable Code Suggestion:
Suggested change
if "autogen" in sys.modules:
Client().configure(instrument_llm_calls=False)
Client()._initialize_autogen_logger()
Client().add_default_tags(["autogen"])
if "crewai" in sys.modules:
+ Client()._initialize_autogen_logger()

Expand Down Expand Up @@ -72,7 +73,6 @@ def init(
Client().unsuppress_logs()
t = threading.Thread(target=check_agentops_update)
t.start()

if Client().is_initialized:
return logger.warning(
"AgentOps has already been initialized. If you are trying to start a session, call agentops.start_session() instead."
Expand Down Expand Up @@ -101,7 +101,6 @@ def init(
"auto_start_session is set to False - inherited_session_id will not be used to automatically start a session"
)
return Client().initialize()

Client().configure(auto_start_session=False)
Client().initialize()
return Client().start_session(inherited_session_id=inherited_session_id)
Expand Down
27 changes: 22 additions & 5 deletions agentops/client.py
Original file line number Diff line number Diff line change
Expand Up @@ -87,16 +87,13 @@ def initialize(self) -> Union[Session, None]:
return

self.unsuppress_logs()

if self._config.api_key is None:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment: Potential exposure of sensitive API keys.

Solution: Implement validation for API keys to ensure they are in the expected format and not empty before use.
!! Make sure the following suggestion is correct before committing it !!

Suggested change
if self._config.api_key is None:
if not self._config.api_key or not isinstance(self._config.api_key, str):

return logger.error(
"Could not initialize AgentOps client - API Key is missing."
+ "\n\t Find your API key at https://app.agentops.ai/settings/projects"
)

self._handle_unclean_exits()
self._initialize_partner_framework()

self._initialized = True

if self._config.instrument_llm_calls:
Comment on lines 94 to 99
Copy link

@github-actions github-actions bot Oct 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Entelligence AI Bot Icon Entelligence AI Bot v4

ℹ️ Logic Error

Potential Missing Initialization Logic

The removal of _initialize_partner_framework() without a replacement or alternative initialization logic could lead to missing setup steps for partner frameworks. Ensure that any necessary initialization previously handled by this method is now covered elsewhere in the code.

Commitable Code Suggestion:
Suggested change
)
self._handle_unclean_exits()
self._initialize_partner_framework()
self._initialized = True
if self._config.instrument_llm_calls:
def initialize(self) -> Union[Session, None]:
)
self._handle_unclean_exits()
- self._initialize_partner_framework()
-
self._initialized = True
if self._config.instrument_llm_calls:
<br>
</details>

Expand All @@ -116,7 +113,7 @@ def initialize(self) -> Union[Session, None]:

return session

def _initialize_partner_framework(self) -> None:
def _initialize_autogen_logger(self) -> None:
try:
import autogen
from .partners.autogen_logger import AutogenLogger
Expand Down Expand Up @@ -235,11 +232,30 @@ def start_session(
if tags is not None:
session_tags.update(tags)

def _start_session_callback(session: Session):
if session.is_running:
if len(self._pre_init_queue["agents"]) > 0:
for agent_args in self._pre_init_queue["agents"]:
session.create_agent(
name=agent_args["name"], agent_id=agent_args["agent_id"]
)
self._pre_init_queue["agents"] = []

logger.info(
colored(
f"\x1b[34mSession Replay: https://app.agentops.ai/drilldown?session_id={session.session_id}\x1b[0m",
"blue",
)
)
else:
self._sessions.remove(session)

session = Session(
session_id=session_id,
tags=list(session_tags),
host_env=get_host_env(self._config.env_data_opt_out),
config=self._config,
callback=_start_session_callback,
)

if not session.is_running:
Comment on lines 232 to 261

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Entelligence AI Bot Icon Entelligence AI Bot v4

ℹ️ Performance Improvement

Optimize Callback Usage for Session Initialization

The introduction of _start_session_callback improves session management by handling pre-initialized agents and logging session replay links. However, ensure that the callback is only set when necessary to avoid unnecessary overhead in session creation.

Commitable Code Suggestion:

@@ -235,11 +232,30 @@ def start_session( if tags is not None: session_tags.update(tags) + def _start_session_callback(session: Session): + if session.is_running: + if len(self._pre_init_queue["agents"]) > 0: + for agent_args in self._pre_init_queue["agents"]: + session.create_agent( + name=agent_args["name"], agent_id=agent_args["agent_id"] + ) + self._pre_init_queue["agents"] = [] + + logger.info( + colored( + f"\x1b[34mSession Replay: https://app.agentops.ai/drilldown?session_id={session.session_id}\x1b[0m", + "blue", + ) + ) + else: + self._sessions.remove(session) + session = Session( session_id=session_id, tags=list(session_tags), host_env=get_host_env(self._config.env_data_opt_out), config=self._config, + callback=_start_session_callback, ) if not session.is_running:

Expand Down Expand Up @@ -305,7 +321,8 @@ def create_agent(
self._pre_init_queue["agents"].append(
{"name": name, "agent_id": agent_id}
)
session.create_agent(name=name, agent_id=agent_id)
else:
session.create_agent(name=name, agent_id=agent_id)

return agent_id

Comment on lines 321 to 328

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Entelligence AI Bot Icon Entelligence AI Bot v4

ℹ️ Logic Error

Conditional Agent Creation Logic

The addition of an else block for session.create_agent() ensures that agents are only created when the session is running. Verify that this logic does not inadvertently skip agent creation in scenarios where it is required.

Commitable Code Suggestion:

@@ -305,7 +321,8 @@ def create_agent(
self._pre_init_queue["agents"].append(
{"name": name, "agent_id": agent_id}
)

  •        session.create_agent(name=name, agent_id=agent_id)
    
  •        else:
    
  •            session.create_agent(name=name, agent_id=agent_id)
    
       return agent_id
    

Expand Down
4 changes: 1 addition & 3 deletions agentops/host_env.py
Original file line number Diff line number Diff line change
Expand Up @@ -47,13 +47,11 @@ def get_sys_packages():


def get_installed_packages():

try:
return {
# TODO: test
# TODO: add to opt out
"Installed Packages": {
dist.metadata["Name"]: dist.version
dist.metadata.get("Name"): dist.metadata.get("Version")
for dist in importlib.metadata.distributions()
}
}
Comment on lines 47 to 57

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Entelligence AI Bot Icon Entelligence AI Bot v4

💻 Code Improvement

Use of get Method for Safe Metadata Access

The change from using direct dictionary access to using the get method for retrieving metadata values is a good practice. It prevents potential KeyError exceptions if the keys 'Name' or 'Version' are not present in the metadata. However, consider adding a default value to the get method to handle cases where the metadata might be missing these keys.

+                dist.metadata.get("Name", "Unknown"): dist.metadata.get("Version", "Unknown")
Commitable Code Suggestion:
Suggested change
def get_installed_packages():
try:
return {
# TODO: test
# TODO: add to opt out
"Installed Packages": {
dist.metadata["Name"]: dist.version
dist.metadata.get("Name"): dist.metadata.get("Version")
for dist in importlib.metadata.distributions()
}
}
dist.metadata.get("Name", "Unknown"): dist.metadata.get("Version", "Unknown")

Expand Down
2 changes: 0 additions & 2 deletions agentops/http_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -81,11 +81,9 @@ def post(

if jwt is not None:
JSON_HEADER["Authorization"] = f"Bearer {jwt}"

res = request_session.post(
url, data=payload, headers=JSON_HEADER, timeout=20
)

result.parse(res)
except requests.exceptions.Timeout:
result.code = 408
Comment on lines 81 to 89

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Entelligence AI Bot Icon Entelligence AI Bot v4

ℹ️ Error Handling

Add Logging for Timeout Exceptions

The current implementation catches a requests.exceptions.Timeout exception and sets the result code to 408. Consider adding logging for this exception to provide more context when a timeout occurs, which can aid in debugging and monitoring.

         except requests.exceptions.Timeout:
+            logging.error(f"Request to {url} timed out.")
             result.code = 408
Commitable Code Suggestion:
Suggested change
if jwt is not None:
JSON_HEADER["Authorization"] = f"Bearer {jwt}"
res = request_session.post(
url, data=payload, headers=JSON_HEADER, timeout=20
)
result.parse(res)
except requests.exceptions.Timeout:
result.code = 408
except requests.exceptions.Timeout:
logging.error(f"Request to {url} timed out.")
result.code = 408

Expand Down
33 changes: 22 additions & 11 deletions agentops/session.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
import time
from decimal import ROUND_HALF_UP, Decimal
from termcolor import colored
from typing import Optional, List, Union
from typing import Optional, List, Union, Callable
from uuid import UUID, uuid4
from datetime import datetime

Expand Down Expand Up @@ -40,6 +40,7 @@ def __init__(
config: Configuration,
tags: Optional[List[str]] = None,
host_env: Optional[dict] = None,
callback: Optional[Callable[["Session"], None]] = None,
):
self.end_timestamp = None
self.end_state: Optional[str] = None
Expand All @@ -60,17 +61,13 @@ def __init__(
"errors": 0,
"apis": 0,
}

self.is_running = False
active_sessions.append(self)
self.stop_flag = threading.Event()
self.thread = threading.Thread(target=self._run)
self.thread = threading.Thread(target=self._run, args=(callback,))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment: Unnecessary thread creation in session management.

Solution: Consider using a thread pool or asynchronous programming to manage sessions more efficiently.
!! Make sure the following suggestion is correct before committing it !!

Suggested change
self.thread = threading.Thread(target=self._run, args=(callback,))
self.thread = concurrent.futures.ThreadPoolExecutor().submit(self._run, callback)

self.thread.daemon = True
self.thread.start()

self.is_running = self._start_session()
if self.is_running == False:
self.stop_flag.set()
self.thread.join(timeout=1)

def set_video(self, video: str) -> None:
"""
Sets a url to the video recording of the session.
Comment on lines 61 to 73

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Entelligence AI Bot Icon Entelligence AI Bot v4

ℹ️ Logic Error

Ensure Session Stops if Initialization Fails

The removal of the initial session start check and immediate stop flag setting could lead to the session thread running even if the session fails to start. This could result in unnecessary resource usage and potential errors.

+        self.is_running = self._start_session()
+        if self.is_running == False:
+            self.stop_flag.set()
+            self.thread.join(timeout=1)
Commitable Code Suggestion:
Suggested change
"errors": 0,
"apis": 0,
}
self.is_running = False
active_sessions.append(self)
self.stop_flag = threading.Event()
self.thread = threading.Thread(target=self._run)
self.thread = threading.Thread(target=self._run, args=(callback,))
self.thread.daemon = True
self.thread.start()
self.is_running = self._start_session()
if self.is_running == False:
self.stop_flag.set()
self.thread.join(timeout=1)
def set_video(self, video: str) -> None:
"""
Sets a url to the video recording of the session.
self.is_running = self._start_session()
if self.is_running == False:
self.stop_flag.set()
self.thread.join(timeout=1)

Expand Down Expand Up @@ -274,16 +271,21 @@ def _start_session(self):
self.config.parent_key,
)
except ApiServerException as e:
return logger.error(f"Could not start session - {e}")
logger.error(f"Could not start session - {e}")
return False

logger.debug(res.body)

if res.code != 200:
logger.error(f"Could not start session - server error")
return False

jwt = res.body.get("jwt", None)
self.jwt = jwt
if jwt is None:
logger.error(
f"Could not start session - server could not authenticate your API Key"
)
return False

session_url = res.body.get(
Comment on lines 271 to 291

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Entelligence AI Bot Icon Entelligence AI Bot v4

🔒 Security Suggestion

Sanitize Error Messages in Logs

The error handling for session start has been improved, but ensure that sensitive information is not logged. Consider sanitizing error messages to prevent potential exposure of sensitive data.

Commitable Code Suggestion:

@@ -274,16 +271,21 @@ def _start_session(self):
self.config.parent_key,
)
except ApiServerException as e:

  •            return logger.error(f"Could not start session - {e}")
    
  •            logger.error(f"Could not start session - {e}")
    
  •            return False
    
           logger.debug(res.body)
    
           if res.code != 200:
    
  •            logger.error(f"Could not start session - server error")
               return False
    
           jwt = res.body.get("jwt", None)
           self.jwt = jwt
           if jwt is None:
    
  •            logger.error(
    
  •                f"Could not start session - server could not authenticate your API Key"
    
  •            )
               return False
    
           session_url = res.body.get(
    

Expand Down Expand Up @@ -359,10 +361,19 @@ def _flush_queue(self) -> None:
elif event_type == "apis":
self.event_counts["apis"] += 1

def _run(self) -> None:
def _run(self, callback: Optional[Callable[["Session"], None]] = None) -> None:
self.is_running = self._start_session()

if callback:
callback(self)

if self.is_running == False:
self.stop_flag.set()
self.thread.join(timeout=1)

while not self.stop_flag.is_set():
time.sleep(self.config.max_wait_time / 1000)
if self.queue:
if self.queue and self.jwt is not None:
self._flush_queue()

def create_agent(self, name, agent_id):
Comment on lines 361 to 379

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Entelligence AI Bot Icon Entelligence AI Bot v4

ℹ️ Performance Improvement

Ensure Non-blocking Callback Execution

The addition of a callback function in the _run method is beneficial for extensibility. However, ensure that the callback execution does not block the main thread, which could delay session operations.

Expand Down
6 changes: 0 additions & 6 deletions tests/test_agent.py
Original file line number Diff line number Diff line change
@@ -1,13 +1,9 @@
from unittest import TestCase

from agentops import track_agent
import agentops


class TrackAgentTests(TestCase):
def test_track_agent_with_class(self):
agentops.init()

@track_agent(name="agent_name")
class TestAgentClass:
t = "a"
Expand All @@ -19,8 +15,6 @@ class TestAgentClass:
self.assertIsNotNone(getattr(obj, "agent_ops_agent_id"))

def test_track_agent_with_class_name(self):
agentops.init()

@track_agent(name="agent_name")
class TestAgentClass:
t = "a"
Expand Down
2 changes: 1 addition & 1 deletion tests/test_canary.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ def setup_teardown():
agentops.end_all_sessions() # teardown part


@pytest.fixture
@pytest.fixture(autouse=True, scope="function")
def mock_req():
with requests_mock.Mocker() as m:
url = "https://api.agentops.ai"
Expand Down
2 changes: 1 addition & 1 deletion tests/test_events.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ def setup_teardown():
agentops.end_all_sessions() # teardown part


@pytest.fixture
@pytest.fixture(scope="function")
def mock_req():
with requests_mock.Mocker() as m:
url = "https://api.agentops.ai"
Expand Down
18 changes: 14 additions & 4 deletions tests/test_pre_init.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ def setup_teardown():


@contextlib.contextmanager
@pytest.fixture(autouse=True)
@pytest.fixture(autouse=True, scope="function")
def mock_req():
with requests_mock.Mocker() as m:
url = "https://api.agentops.ai"
Expand Down Expand Up @@ -48,10 +48,20 @@ def test_track_agent(self, mock_req):
assert len(mock_req.request_history) == 0

agentops.init(api_key=self.api_key)
time.sleep(1)

# Assert
# start session and create agent
assert len(mock_req.request_history) == 2
assert mock_req.last_request.headers["X-Agentops-Api-Key"] == self.api_key

agentops.end_session(end_state="Success")

# Wait for flush
time.sleep(1.5)

# 3 requests: create session, create agent, update session
assert len(mock_req.request_history) == 3

assert (
mock_req.request_history[-2].headers["X-Agentops-Api-Key"] == self.api_key
)

mock_req.reset()
2 changes: 1 addition & 1 deletion tests/test_record_action.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ def setup_teardown():


@contextlib.contextmanager
@pytest.fixture(autouse=True)
@pytest.fixture(autouse=True, scope="function")
def mock_req():
with requests_mock.Mocker() as m:
url = "https://api.agentops.ai"
Expand Down
2 changes: 1 addition & 1 deletion tests/test_record_tool.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ def setup_teardown():


@contextlib.contextmanager
@pytest.fixture(autouse=True)
@pytest.fixture(autouse=True, scope="function")
def mock_req():
with requests_mock.Mocker() as m:
url = "https://api.agentops.ai"
Expand Down
2 changes: 1 addition & 1 deletion tests/test_session.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ def setup_teardown():
agentops.end_all_sessions() # teardown part


@pytest.fixture
@pytest.fixture(scope="function")
def mock_req():
with requests_mock.Mocker() as m:
url = "https://api.agentops.ai"
Expand Down
2 changes: 1 addition & 1 deletion tests/test_teardown.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
from agentops import Client


@pytest.fixture
@pytest.fixture(scope="function")
def mock_req():
with requests_mock.Mocker() as m:
url = "https://api.agentops.ai"
Expand Down
Loading