✅ 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.

✅ PR preview is ready!

📉 Python coverage change detected

The Python unit test coverage has decreased by 0.0493%

• Current PR: 92.7958% (20960 statements, 1510 missed)
• Latest develop: 92.8451% (20825 statements, 1490 missed)

💡 Consider adding more unit tests to maintain or improve coverage.

📊 View detailed coverage comparison

📉 Frontend coverage change detected

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

• Current PR: 86.3400% (12614 lines, 1722 missed)
• Latest develop: 86.3400% (12614 lines, 1722 missed)

✅ Coverage change is within normal range.

📊 View detailed coverage comparison

@cursor review

Summary

This PR extends Streamlit's metrics collection capabilities by:

1. Adding new WebSocket session metrics (connection, reconnection, disconnection counters, and active sessions gauge)
2. Implementing metric family filtering to allow requesting specific metrics via the families query parameter
3. Introducing new stat types (CounterStat and GaugeStat) alongside the existing CacheStat
4. Updating the StatsProvider protocol to support filtered stat retrieval

This is a continuation of work from #13265 to make the metrics system more general and performant, as cache stats are expensive to compute and new metrics may need frequent polling.

Code Quality

The code is well-structured and follows existing patterns in the codebase. Key observations:

Positives:

• Clean, well-documented code with proper docstrings (Numpy style)
• Comprehensive type annotations throughout
• Thread-safe implementation using _stats_lock in WebsocketSessionManager for counter access
• Good separation of concerns with the Stat protocol for polymorphic stat handling
• Proper use of Final constants for metric family names

Minor observations:

1. Protocol stub convention (lib/streamlit/runtime/stats.py:44-51): The Stat protocol uses pass for method stubs. While this works, the conventional way is to use ... (ellipsis):

@property
def family_name(self) -> str:
    ...  # More conventional for protocols

2. Lock consistency for active_sessions (lib/streamlit/runtime/websocket_session_manager.py:255): The active_sessions metric reads len(self._active_session_info_by_id) without holding a lock, while counter reads use _stats_lock. While reading dict length is atomic in CPython, this inconsistency could be a concern if the implementation changes:

if family_names is None or ACTIVE_SESSIONS_FAMILY in family_names:
    result[ACTIVE_SESSIONS_FAMILY] = [
        GaugeStat(
            family_name=ACTIVE_SESSIONS_FAMILY,
            value=len(self._active_session_info_by_id),  # No lock held here
            help="Current number of active sessions.",
        ),
    ]

3. TODO acknowledged (lib/streamlit/runtime/stats.py:72-74): There's an existing TODO about potentially consolidating CacheStat with GaugeStat. This is reasonable technical debt for future cleanup.

Test Coverage

Excellent test coverage overall:

• websocket_session_manager_test.py: Comprehensive tests for all session metrics scenarios:

  • Initial stats verification (all zeros)
  • Connection, reconnection, disconnection counter increments
  • Active sessions gauge accuracy
  • Edge cases (invalid session IDs, already-connected sessions)
  • Correct stat format verification

• stats_test.py: Thorough tests for:

  • StatsManager filtering behavior
  • Multi-family provider handling
  • CounterStat and GaugeStat protocol compliance
  • Metric string formatting with/without labels
  • metric_type_string_to_proto mapping

• stats_handler_test.py: HTTP endpoint tests for:

  • Family filtering via query parameters
  • Multiple family filtering
  • Counter and gauge text format output

• Other test files updated: Cache and storage tests properly updated to reflect the new return type.

All tests follow pytest best practices with descriptive docstrings.

Backwards Compatibility

⚠️ Breaking change to StatsProvider interface:

The StatsProvider.get_stats() method signature changed from:

def get_stats(self) -> list[CacheStat]

to:

def get_stats(self, family_names: Sequence[str] | None = None) -> Mapping[str, Sequence[Stat]]

This affects:

• All internal StatsProvider implementations (updated in this PR)
• Any external implementations (if any exist)

Mitigating factors:

• StatsProvider is defined as a Protocol, so external code using duck-typing would need updates
• This appears to be an internal API not documented for external use
• The metrics endpoint behavior is backwards compatible (no filter = all metrics)

Security & Risk

No security concerns identified:

• Metrics endpoint doesn't expose sensitive information
• Thread safety is properly handled for counters
• No new authentication/authorization requirements

Low regression risk:

• Changes are additive (new metrics)
• Existing cache stats functionality preserved
• All internal implementations updated consistently

Recommendations

1. Consider documentation update: If StatsProvider is intended for external use, document the API change in release notes or migration guide.

2. Optional lock consistency (low priority): Consider whether the active sessions read should also be protected or if a comment explaining why it's safe without a lock would be helpful:

# Reading dict length is atomic in CPython; no lock needed
value=len(self._active_session_info_by_id),

3. Future consideration: The TODO about consolidating CacheStat with GaugeStat could simplify the codebase. Consider creating a follow-up issue to track this.

Verdict

APPROVED: This is a well-implemented feature that extends Streamlit's metrics collection with proper test coverage. The breaking change to StatsProvider is acceptable given it's an internal API, and all internal implementations have been properly updated. The code follows existing patterns and best practices.

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

I think it might be possible to add some basic e2e tests to ensure that, e.g. filtering is supported and some counting happens on websocket disconnects / reconnects.

I added some e2e tests to verify that filtering works. Didn't add tests to also consider websocket connect/disconnect increments the metric counts, though, since that code is already pretty well covered by tests of the websocket session manager, which the starlette migration uses without any changes. I'll probably still look to add some e2e test coverage here in a followup (I have a few that I'll get around to late this week) but don't want to increase the size of this PR any further.