✅ PR preview is ready!

✅ Snyk checks have passed. No issues have been found so far.

Status	Scanner	Critical	High	Medium	Low	Total (0)
✅	Open Source Security	0	0	0	0	0 issues
✅	Licenses	0	0	0	0	0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

📉 Frontend coverage change detected

The frontend unit test (vitest) coverage has decreased by 0.0000%

• Current PR: 86.5100% (12761 lines, 1721 missed)
• Latest develop: 86.5100% (12761 lines, 1721 missed)

✅ Coverage change is within normal range.

📊 View detailed coverage comparison

Summary

This PR adds a new metric session_duration_seconds_total to track the cumulative duration of all user sessions in Streamlit. The implementation records session connect/reconnect times and accumulates the total duration when sessions are disconnected or closed.

Changes:

• lib/streamlit/runtime/stats.py: Added new constant SESSION_DURATION_FAMILY
• lib/streamlit/runtime/websocket_session_manager.py: Added tracking infrastructure and metric reporting
• lib/tests/streamlit/runtime/websocket_session_manager_test.py: Added comprehensive test coverage

Code Quality

The code quality is excellent and follows Streamlit's established patterns:

Strengths:

• Uses time.monotonic() correctly for duration tracking, which is not affected by system clock changes
• Thread safety is properly maintained with _stats_lock for all counter operations
• The _accumulate_session_duration method includes clear documentation about the locking requirement
• Proper use of from __future__ import annotations and type hints
• Private members are correctly prefixed with _ (e.g., _session_connect_times, _total_session_duration_seconds)
• Metric naming follows OpenMetrics conventions with _total suffix for counters

Implementation Details:

• _session_connect_times: Dict tracking session IDs to their connection timestamps
• _total_session_duration_seconds: Float accumulator for total session duration
• Duration is calculated on disconnect/close and the connect time entry is removed, preventing double-counting

    def _accumulate_session_duration(self, session_id: str) -> None:
        """Accumulate the session duration for a closed session.

        This method must be called while holding self._stats_lock.
        """
        connect_time = self._session_connect_times.pop(session_id, None)
        if connect_time is not None:
            duration = time.monotonic() - connect_time
            self._total_session_duration_seconds += duration

The implementation correctly handles edge cases:

• Sessions closed without connection return early (no crash)
• Reconnects record new connect times, so duration accumulates correctly across cycles
• Duration while disconnected (in storage) is not counted, which is the intended behavior

Test Coverage

Test coverage is comprehensive and follows the Python unit test guidelines:

New Tests Added (5 tests):

1. test_session_duration_accumulated_on_close - Simple close scenario
2. test_session_duration_accumulated_from_multiple_sessions - Accumulation across multiple sessions
3. test_session_duration_accumulated_on_disconnect - Duration tracked on disconnect
4. test_session_duration_not_double_counted_on_close_from_storage - Prevents double-counting
5. test_session_duration_with_reconnect - Correct accumulation across disconnect/reconnect cycles

Quality of Tests:

• ✅ All tests have descriptive docstrings (per Python test guidelines)
• ✅ Tests are properly type-annotated
• ✅ Use of @patch for mocking time.monotonic provides deterministic behavior
• ✅ Tests cover both happy path and edge cases
• ✅ Tests verify the expected values with clear comments explaining the math

Updated Tests:

• _get_stat_value helper updated to handle CounterStat without labels

            if label_type is None:
                # For GaugeStat or CounterStat without labels
                if isinstance(stat, (GaugeStat, CounterStat)) and not stat.labels:
                    return stat.value

• test_initial_stats_are_zero updated to verify duration starts at 0
• test_get_stats_returns_correct_format updated to verify new metric format

Backwards Compatibility

No breaking changes. This PR is purely additive:

• A new metric family constant is added
• New tracking fields are added internally
• Existing get_stats() return format is extended with a new key
• No changes to public APIs or existing metric families
• Existing users consuming the metrics endpoint will simply see an additional metric

Security & Risk

No security concerns identified:

• Only aggregate duration data is exposed (no PII)
• No new external dependencies introduced
• Thread safety is maintained
• The implementation uses time.monotonic() which cannot be manipulated externally

Risks:

• Minimal risk: Long-lived sessions won't contribute to the metric until disconnected/closed (acknowledged in PR description)
• No regression risk: Changes are isolated to metrics tracking and don't affect core session functionality

Recommendations

The PR is well-implemented with no critical issues. Minor observations:

1. Optional Enhancement: Consider adding a brief comment in stats.py explaining the purpose of SESSION_DURATION_FAMILY, similar to how it might be documented elsewhere, though this is not strictly necessary.

2. Documentation: The PR description clearly explains the caveat about long-lived sessions. This behavior is acceptable and documented.

Verdict

APPROVED: This is a clean, well-tested addition of a new metrics feature. The code follows Streamlit's patterns, has comprehensive test coverage, is backwards compatible, and poses no security risks.

This is an automated AI review. Please verify the feedback and use your judgment.

@cursor review