Copy of PR #435 - make start_session non blocking #6
Conversation
Co-authored-by: Howard Gil <[email protected]>
🔍 Code Review Summary❗ Attention Required: This push has potential issues. 🚨 Overview
🚨 Critical Issuessecurity (2 issues)1. Use of potentially unsafe API key handling.📁 File: agentops/session.py 💡 Solution: Current Code: logger.error(f"Could not start session - server could not authenticate your API Key")Suggested Code: logger.error("Could not start session - server could not authenticate your API Key")2. Potential performance bottleneck due to thread join timeout.📁 File: agentops/session.py 💡 Solution: Current Code: self.thread.join(timeout=1)Suggested Code: self.thread.join(timeout=self.config.max_wait_time)
Useful Commands
|
![kaizen-bot[bot]](https://avatars.githubusercontent.com/in/480502?s=60&v=4){kind=small}
| 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" |
There was a problem hiding this comment.
Comment: Use of potentially unsafe API key handling.
Solution: Implement a mechanism to mask or omit sensitive information from logs.
!! Make sure the following suggestion is correct before committing it !!
| f"Could not start session - server could not authenticate your API Key" | |
| logger.error("Could not start session - server could not authenticate your API Key") |
|
|
||
| if self.is_running == False: | ||
| self.stop_flag.set() | ||
| self.thread.join(timeout=1) |
There was a problem hiding this comment.
Comment: Potential performance bottleneck due to thread join timeout.
Solution: Consider implementing a more robust mechanism for thread management, such as using a thread pool or increasing the timeout based on expected workload.
!! Make sure the following suggestion is correct before committing it !!
| self.thread.join(timeout=1) | |
| self.thread.join(timeout=self.config.max_wait_time) |
|
Summary
|
|
WalkthroughThis update refines the AgentOps library by enhancing session management and logging capabilities. Key changes include the introduction of a callback mechanism for session initialization, improved error handling during session start, and the removal of redundant code. The update also modifies test fixtures to ensure automatic and scoped execution, enhancing test reliability. Additionally, the Changes
|
|
Files selected (13)
Files ignored (0)InstructionsEmoji Descriptions:
Interact with the Bot:
Available Commands:
Tips for Using @Entelligence.AI Effectively:
Need More Help?
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| "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. |
There was a problem hiding this comment.
💻 Code Efficiency 🟡
Simplify Condition Check
The condition if self.is_running == False: can be simplified to if not self.is_running: for better readability.
| "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. | |
| if not self.is_running: |
|
|
||
|
|
||
| @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" |
There was a problem hiding this comment.
💻 Code reliability 🟡
Mocking Completeness
Ensure that the requests_mock.Mocker context manager is properly handling all expected scenarios in the tests. If there are any edge cases or specific responses that need to be mocked, they should be explicitly defined to avoid flaky tests.
| @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" | |
| No code suggestion necessary. |
💻 Code maintainability 🟡
Fixture Scope Change
The change to the pytest fixture to include a scope of 'function' is a good practice as it ensures that the fixture is invoked for each test function. However, ensure that this change does not introduce any unintended side effects in tests that may rely on the previous behavior of the fixture.
| @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" | |
| No code suggestion necessary. |
| 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" |
There was a problem hiding this comment.
💻 Code maintainability 🟡
Review Fixture Scope Change
The change to the pytest fixture scope from default to 'function' is a good practice for ensuring that each test runs in isolation. However, ensure that this change does not introduce any unintended side effects in tests that rely on shared state. Review the tests to confirm that they are not dependent on the previous fixture behavior.
| 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" | |
| N/A |
📜 Guidelines:
N/A
💻 Code efficiency 🟡
Profile Test Performance
Using a scoped fixture can lead to increased test execution time if the setup is resource-intensive. Consider profiling the tests to ensure that the performance remains acceptable with the new fixture scope.
| 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" | |
| N/A |
📜 Guidelines:
N/A
|
|
||
|
|
||
| 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() | ||
| } | ||
| } |
There was a problem hiding this comment.
💻 Code maintainability 🟡
Add Error Handling for Metadata Retrieval
The use of dist.metadata.get('Name') and dist.metadata.get('Version') is a good improvement for handling missing keys. However, consider adding error handling to manage cases where the metadata might not be available, which could lead to unexpected behavior.
| 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() | |
| } | |
| } | |
| try: | |
| name = dist.metadata.get('Name') | |
| version = dist.metadata.get('Version') | |
| if name and version: | |
| installed_packages[name] = version | |
| except Exception as e: | |
| # Handle or log the error |
💻 Code efficiency 🟡
Optimize Distribution Metadata Retrieval
The current implementation retrieves all distributions and their metadata in a single pass. If the number of distributions is large, this could lead to performance issues. Consider implementing pagination or lazy loading if applicable.
| 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() | |
| } | |
| } | |
| # Consider implementing pagination or lazy loading here |
| 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" |
There was a problem hiding this comment.
💻 Code reliability 🟡
Fixture Isolation Concern
Using autouse=True for the fixture may lead to tests that rely on the mock not being isolated. If a test fails due to the mock being applied when it shouldn't be, it could lead to confusion. Consider using explicit fixture application for clarity.
| 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" | |
| @pytest.fixture(scope="function") |
💻 Code maintainability 🟡
Autouse Fixture Risk
The mock_req fixture is now set to autouse, which means it will be applied to all tests in the module. This could lead to unintended side effects if any tests do not require this mock. Consider explicitly applying the fixture only to tests that need it to avoid potential issues.
| 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" | |
| @pytest.fixture(scope="function") |
| 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: |
There was a problem hiding this comment.
ℹ️ Performance Improvement 🟡
Optimize Callback Function Definition
The callback function _start_session_callback is defined inside the start_session method. This could lead to unnecessary redefinition of the function every time start_session is called. Consider moving it outside to improve performance.
| 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: | |
| def _start_session_callback(session: Session): | |
| # Callback logic here |
|
|
||
|
|
||
| @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" |
There was a problem hiding this comment.
💻 Code reliability 🟡
Verify Mock Coverage
Ensure that the requests_mock.Mocker() is properly handling all expected requests in the tests. If any requests are missed, it could lead to false positives in test results.
| @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" | |
| No code suggestion necessary. |
💻 Code maintainability 🟡
Fixture Scope Change
The test fixture mock_req is now scoped to 'function', which is a good practice for ensuring that tests do not interfere with each other. However, ensure that this change does not lead to any unintended side effects in tests that rely on the previous behavior of the fixture being scoped to the module level.
| @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" | |
| No code suggestion necessary. |
| from agentops import Client | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| @pytest.fixture(scope="function") | ||
| def mock_req(): | ||
| with requests_mock.Mocker() as m: | ||
| url = "https://api.agentops.ai" |
There was a problem hiding this comment.
💻 Code reliability 🟡
Add Assertions to Mocked Requests
Consider adding assertions in the tests that utilize the mock_req fixture to ensure that the mocked requests are being called as expected. This will enhance the reliability of your tests.
| from agentops import Client | |
| @pytest.fixture | |
| @pytest.fixture(scope="function") | |
| def mock_req(): | |
| with requests_mock.Mocker() as m: | |
| url = "https://api.agentops.ai" | |
| assert m.call_count > 0 |
💻 Code maintainability 🟡
Fixture Scope Change
The change to the pytest fixture scope from default to 'function' is a good practice for ensuring that each test runs in isolation. However, ensure that this change does not introduce any unintended side effects in tests that may rely on shared state across tests.
| from agentops import Client | |
| @pytest.fixture | |
| @pytest.fixture(scope="function") | |
| def mock_req(): | |
| with requests_mock.Mocker() as m: | |
| url = "https://api.agentops.ai" | |
| No code suggestion necessary. |
| 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" |
There was a problem hiding this comment.
💻 Code maintainability 🟡
Review Fixture Scope Change
The change to the pytest fixture scope from default to 'function' is a good practice for ensuring that each test runs in isolation. However, ensure that this change does not introduce any unintended side effects in tests that rely on shared state. Review all tests using this fixture to confirm they function correctly with the new scope.
| 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" | |
| N/A |
📜 Guidelines:
N/A
💻 Code efficiency 🟡
Optimize Fixture Performance
Using a scoped fixture can lead to increased setup and teardown overhead if the fixture is resource-intensive. If the mock_req fixture is heavy, consider optimizing it or using a more efficient setup to reduce test execution time.
| 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" | |
| N/A |
📜 Guidelines:
N/A
|
|
||
| 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 |
There was a problem hiding this comment.
ℹ️ Performance Improvement 🟡
Consider Configurable Timeout
The timeout value of 20 seconds may be too long for certain applications, potentially leading to poor user experience. Consider making this value configurable or reducing it to a more reasonable default.
| 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 | |
| timeout_value = get_configured_timeout() # Example function | |
| res = request_session.post( | |
| url, data=payload, headers=JSON_HEADER, timeout=timeout_value | |
| ) |
|
Summary
|
|
WalkthroughThis update refines the AgentOps library by enhancing session management and logging capabilities. Key changes include the introduction of a callback mechanism for session initialization, improved error handling during session start, and the removal of redundant code. The update also modifies test fixtures to ensure automatic and scoped execution, enhancing test reliability. Additionally, the Changes
|
|
Files selected (13)
Files ignored (0)InstructionsEmoji Descriptions:
Interact with the Bot:
Available Commands:
Tips for Using @Entelligence.AI Effectively:
Need More Help?
|
|
|
||
|
|
||
| @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" |
There was a problem hiding this comment.
💻 Code maintainability 🟡
Fixture Scope Adjustment
The change to the pytest fixture to include scope='function' is a good practice as it ensures that the fixture is executed for each test function, improving isolation. However, ensure that this change does not introduce performance overhead if the fixture is resource-intensive.
|
|
||
|
|
||
| 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() | ||
| } | ||
| } |
There was a problem hiding this comment.
💻 Code efficiency 🟡
Handle Missing Metadata Gracefully
The change from direct indexing to using get for accessing dist.metadata is a good improvement for handling potential missing keys. However, ensure that the absence of 'Name' or 'Version' does not lead to unexpected behavior in the application. Consider adding a fallback or logging mechanism to handle cases where these keys are not present.
| 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", "N/A") |
|
|
||
|
|
||
| @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" |
There was a problem hiding this comment.
💻 Code maintainability 🟡
Review Fixture Scope Change
The change to the pytest fixture scope from the default to 'function' is a good practice for ensuring that tests are isolated and do not share state. However, ensure that this change does not introduce any unintended side effects in tests that rely on shared state across multiple tests.
| 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" |
There was a problem hiding this comment.
💻 Code reliability 🟡
Mock Configuration
Ensure that the mock request in the mock_req fixture is properly configured to handle all expected scenarios. If the tests rely on specific responses from the mocked API, consider defining those responses explicitly to avoid false positives in tests.
| 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" | |
| m.get(url, json={'key': 'value'}, status_code=200) |
💻 Code maintainability 🟡
Fixture Scope Optimization
Changing the fixture scope to 'function' can lead to increased overhead if the setup is expensive. If the setup is lightweight, this change is fine, but if it involves significant resources, consider using a broader scope to optimize test execution time.
| 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" | |
| @pytest.fixture(scope="module") |
| 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" |
There was a problem hiding this comment.
💻 Code reliability 🟡
Verify Mocking Completeness
Ensure that the requests_mock.Mocker() is properly handling all expected request scenarios in the tests. If any requests are not mocked, it could lead to flaky tests that fail intermittently.
💻 Code maintainability 🟡
Review Fixture Autouse Behavior
The change to the pytest fixture to use autouse=True may lead to unexpected behavior if the fixture is not needed for every test function. Ensure that all tests that rely on this fixture are properly isolated and do not inadvertently depend on the fixture's side effects.
| 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" | |
| @pytest.fixture(autouse=True, scope="function") |
| from agentops import Client | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| @pytest.fixture(scope="function") | ||
| def mock_req(): | ||
| with requests_mock.Mocker() as m: | ||
| url = "https://api.agentops.ai" |
There was a problem hiding this comment.
💻 Code reliability 🟡
Mock Isolation
Ensure that the mock request is properly isolated and does not interfere with other tests. Consider using a more specific scope or additional teardown logic if necessary.
| 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: |
There was a problem hiding this comment.
ℹ️ Performance Improvement 🟡
Optimize Callback Definition
The callback _start_session_callback is defined within the start_session method. This could lead to unnecessary redefinitions of the callback function every time start_session is called. Consider moving the callback definition outside of the method to improve performance and clarity.
| 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: | |
| def _start_session_callback(session: Session): | |
| if session.is_running: | |
| # existing logic | |
| else: | |
| self._sessions.remove(session) | |
| def start_session(...): | |
| callback=_start_session_callback, |
| 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" |
There was a problem hiding this comment.
💻 Code maintainability 🟡
Parameterize Mock URL
Ensure that the mock request URL is configurable or parameterized to avoid hardcoding. This will improve the flexibility of the tests and allow for easier updates in the future.
| 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" | |
| url = os.getenv('MOCK_URL', 'https://api.agentops.ai') |
💻 Code maintainability 🟡
Fixture Scope Optimization
Changing the fixture scope to 'function' can lead to increased overhead if the setup is expensive. If the setup is lightweight, this is fine, but if it involves significant resources, consider using a broader scope to optimize test execution time.
| 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" | |
| @pytest.fixture(scope="module") |
|
|
||
| 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 |
There was a problem hiding this comment.
ℹ️ Performance Improvement 🟡
Configurable Timeout for POST Request
The timeout value for the POST request is hardcoded to 20 seconds. Consider making this configurable to allow for better performance tuning based on different environments or use cases.
| 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 | |
| timeout_value = get_configured_timeout() # Fetch from config | |
| res = request_session.post( | |
| url, data=payload, headers=JSON_HEADER, timeout=timeout_value | |
| ) |
Enhance Client Initialization and Session Handling
Improve the initialization process and session management in the AgentOps client.
_initialize_autogen_logger()method to configure logging for autogen.Sessionclass to handle session state changes.These changes enhance the reliability and maintainability of the AgentOps client, leading to better debugging and session management.
Original Description
## 📥 Pull Request📘 Description
Make start session non-blocking
(This PR was copied from AgentOps-AI/agentops PR AgentOps-AI#435)