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

refactor: reorganize session management into dedicated components #631

Closed
wants to merge 5 commits into from

Conversation

teocns
Copy link
Contributor

@teocns teocns commented Jan 9, 2025

Overview

This PR refactors the session management code to improve maintainability, testability, and separation of concerns, historically proposed in #486. The main changes include:

  1. Breaking down the monolithic Session class into smaller, focused components
  2. Introducing proper dependency management and initialization
  3. Improving error handling and type safety
  4. Maintaining backward compatibility while modernizing the codebase

Key Changes

1. New Module Structure

Created a dedicated session package with clear component separation:

2. Improved Error Event Handling

  • Made ErrorEvent properly inherit from Event base class (booyah! @areibman)
  • Added proper timestamp handling and inheritance
  • Improved error data serialization

3. Client Simplification

  • Streamlined session creation logic
  • Improved tag handling
  • Better type hints and return values
  • Cleaner initialization flow

Implementation Details

Session Components

Each component has a single responsibility:

  • Session: Data container with minimal public API
  • SessionManager: Coordinates between components and manages lifecycle
  • SessionAPI: Handles all HTTP communication
  • SessionTelemetry: Manages OpenTelemetry integration
  • Registry: Tracks active sessions
sequenceDiagram
    participant C as Client
    participant S as Session
    participant M as SessionManager
    participant A as SessionApi
    participant T as SessionTelemetry
    participant E as SessionExporter

    C->>S: start_session()
    S->>M: create()
    M->>A: create_session()
    A-->>M: jwt
    M->>T: setup()
    T->>E: init()

    C->>S: record(event)
    S->>M: record_event()
    M->>T: record_event()
    T->>E: export()
    E->>A: create_events()
Loading

Split session logic into dedicated components for better separation of
concerns
and maintainability:

- Move session code to dedicated session/ module
- Split Session class into:
  - Session: Data container with minimal public API
  - SessionManager: Handles lifecycle and state management
  - SessionApi: Handles API communication
  - SessionTelemetry: Manages event recording and OTEL integration

Key fixes:
- Proper UUID and timestamp serialization in events
- Consistent API key header handling
- Correct token cost formatting in analytics
- Proper session ID inheritance
- Tags conversion and validation
- Event counts type handling

This refactor improves code organization while maintaining backward
compatibility
through the session/__init__.py module.

Signed-off-by: Teo <[email protected]>
@teocns teocns force-pushed the feat/session-refactoring branch from 6a8e7af to eb072db Compare January 9, 2025 14:04
Copy link

codecov bot commented Jan 9, 2025

Codecov Report

Attention: Patch coverage is 72.16495% with 108 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
agentops/session/manager.py 68.59% 20 Missing and 18 partials ⚠️
agentops/session/api.py 72.46% 14 Missing and 5 partials ⚠️
agentops/session/session.py 75.75% 7 Missing and 9 partials ⚠️
agentops/telemetry/exporters/session.py 75.00% 11 Missing and 3 partials ⚠️
agentops/session/telemetry.py 73.17% 6 Missing and 5 partials ⚠️
agentops/client.py 63.63% 3 Missing and 1 partial ⚠️
agentops/session/registry.py 66.66% 1 Missing and 3 partials ⚠️
agentops/event.py 75.00% 1 Missing and 1 partial ⚠️
Flag Coverage Δ
unittests 38.63% <72.16%> (+1.21%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
agentops/session/__init__.py 100.00% <100.00%> (ø)
agentops/event.py 89.28% <75.00%> (-2.88%) ⬇️
agentops/client.py 66.50% <63.63%> (-0.50%) ⬇️
agentops/session/registry.py 66.66% <66.66%> (ø)
agentops/session/telemetry.py 73.17% <73.17%> (ø)
agentops/telemetry/exporters/session.py 75.00% <75.00%> (ø)
agentops/session/session.py 75.75% <75.75%> (ø)
agentops/session/api.py 72.46% <72.46%> (ø)
agentops/session/manager.py 68.59% <68.59%> (ø)

... and 2 files with indirect coverage changes

@teocns
Copy link
Contributor Author

teocns commented Jan 9, 2025

@the-praxs can you please review and run manual (integration) tests? 🙏
make sure this is prod-ready

@teocns teocns requested a review from the-praxs January 9, 2025 14:11
@the-praxs
Copy link
Member

@the-praxs can you please review and run manual (integration) tests? 🙏 make sure this is prod-ready

#632 is failing for both main and this PR so will have to take a look after a while.

#633 is failing for this PR which means we need to have the edge test cases for such issues.

Copy link
Member

@the-praxs the-praxs left a comment

Choose a reason for hiding this comment

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

Left comments on the failing tests for CrewAI and Autogen.

We need to fix the issue with CrewAI failing in this PR. Then we have to fix Autogen in a new PR since it's occurring on main too.

After these I think the PR is ready to be tested and merged, should the tests pass!

@teocns teocns closed this Jan 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants