PR Review: Add Connection Delay Metric for Soniox

Summary

This PR adds connection delay metrics tracking to the Soniox ASR extension, measuring the time between connection initiation and establishment. The implementation is well-structured with comprehensive tests.

Code Quality & Best Practices ✅

Strengths:

• Clean, focused implementation following the repository's extension patterns
• Proper separation of concerns between websocket client and extension logic
• Good use of async/await patterns throughout
• Documentation in websocket.py clearly describes the callback signature change
• Version bump in manifest.json (0.3.6 → 0.3.7) follows semantic versioning

Style & Convention:

• Code adheres to Python conventions with proper formatting
• Consistent with other ASR extensions in the codebase
• Proper use of type hints and docstrings in test file

Potential Issues & Suggestions

1. Default Parameter Value (Minor)

Location: websocket.py:199, conftest.py:199

Using 0 as a default value could lead to misleading metrics if the parameter is accidentally omitted. This would result in an extremely large connection delay (current time minus epoch).

Suggestion: Consider making the parameter required (no default) or handle 0 explicitly with a warning.

2. Clock Skew Considerations (Minor)

Location: extension.py:347-349

The calculation uses time.time() at both points, which is reliable for a single process. However, for defensive programming, consider adding a check for negative delays.

3. Test Boundary Validation (Very Minor)

Location: test_connection_delay_metrics.py:223

The assertion >= 400 is slightly tight given the 500ms simulated delay. Test flakiness could occur under heavy load. Consider using a slightly more forgiving threshold.

Performance Considerations ✅

• Minimal overhead: The timestamp recording and calculation are O(1) operations
• No blocking operations: All async operations are properly awaited
• Efficient: Integer timestamp calculation is very lightweight
• No memory concerns: No additional data structures or caching introduced

Security Concerns ✅

• No security risks identified
• No sensitive data exposure in metrics
• No injection vulnerabilities
• Connection timestamps don't leak sensitive information
• API key handling unchanged and remains secure

Test Coverage ✅

Excellent test coverage:

• ✅ Comprehensive test file (test_connection_delay_metrics.py) with 228 lines
• ✅ Tests the full lifecycle: connection → metrics emission → validation
• ✅ Validates metrics structure (id, module, vendor, metadata)
• ✅ Validates metric values (type checking, range checking)
• ✅ Uses proper mocking with custom connect behavior
• ✅ Includes timeout handling to prevent hanging tests
• ✅ Tests boundary conditions (< 2000ms upper bound)
• ✅ Proper cleanup in on_stop with task cancellation

Minor Enhancement: Consider adding a test case for immediate/near-instant connections (delay ≈ 0ms) to validate edge cases.

Architecture & Design ✅

Well-designed implementation:

1. Timestamp propagation: Clean flow from websocket.py → extension.py
2. Callback signature change: Properly documented and updated in all locations
3. Backwards compatibility: The change modifies internal APIs only
4. Consistent with patterns: Follows the same metrics reporting pattern as other extensions

Files Changed Analysis

extension.py (9 additions, 2 deletions) ✅

• Added parameter to _handle_open()
• Calculates and logs connection delay
• Calls metrics reporting method
• Clean integration into existing flow

websocket.py (8 additions, 2 deletions) ✅

• Records timestamp before connection attempt
• Passes timestamp to OPEN callback
• Updated documentation
• Minimal, focused changes

manifest.json (1 addition, 1 deletion) ✅

• Version bump appropriate for new feature

conftest.py (10 additions, 3 deletions) ✅

• Updated test fixtures to support timestamp parameter
• Maintains backward compatibility with default value

test_connection_delay_metrics.py (228 additions, 0 deletions) ✅

• Comprehensive new test file
• Well-structured with clear test intentions

Final Verdict: ✅ APPROVE with minor suggestions

This is a solid implementation that adds valuable observability to the Soniox ASR extension. The code quality is high, tests are comprehensive, and the changes are minimal and focused.

Recommendations before merge:

1. Consider the default parameter value concern (severity: minor)
2. Optional: Add defensive clock skew check (nice-to-have)
3. Consider adding an edge case test for near-instant connections (enhancement)

None of these are blocking issues. The PR is ready to merge as-is if you choose.

Great work on the implementation and especially on the thorough test coverage! 🎉

Pull Request Review: Add Connection Delay Metric for Soniox

Overview

This PR adds connection delay metrics tracking to the Soniox ASR extension, measuring the time between initiating a WebSocket connection and receiving the open event. The implementation includes comprehensive test coverage.

Code Quality & Best Practices

✅ Strengths

1. Clean metric implementation: The connection delay calculation is straightforward and accurate
2. Comprehensive test coverage: Well-structured test with proper timeout handling and validation
3. Good testing practices: Uses fixtures appropriately, tests edge cases, validates reasonable values
4. Consistent with codebase patterns: Follows existing conventions for logging and testing
5. Version bump: Appropriately incremented version from 0.3.6 to 0.3.7

🔴 CRITICAL ISSUE: Missing Method Definition

The send_connect_delay_metrics method is called but never defined!

Location: extension.py:356
This method is not implemented in the SonioxASRExtension class or the AsyncASRBaseExtension base class.

Required Action: Implement this method following patterns from other extensions. You likely need something like:

async def send_connect_delay_metrics(self, connection_delay_ms: int) -> None:
    from ten_ai_base.message import ModuleMetrics, ModuleType
    await self._send_asr_metrics(
        ModuleMetrics(
            module=ModuleType.ASR,
            vendor=self.vendor(),
            metrics={"connect_delay": connection_delay_ms},
            metadata={}
        )
    )

Potential Bugs & Issues

1. Timestamp Variable Initialization (websocket.py:110)

The _connection_attempt_start_time is initialized to 0 which could cause issues if _handle_open is triggered unexpectedly.

Recommendation: Initialize to None with validation, or add a check before use.

2. Error Handling Missing (extension.py:346-356)

No try-except around metrics sending. If it fails, it could prevent connection initialization from completing.

Recommendation: Add error handling with appropriate logging.

3. Test Flakiness Potential (test_connection_delay_metrics.py:223)

Assertion expects >= 400ms for 0.5s delay could be flaky on slow CI systems.

Recommendation: Use more lenient threshold or add tolerance comment.

Performance & Security

✅ Performance: No concerns - timestamp capture is efficient, measurement overhead negligible
✅ Security: No issues - connection delay is non-sensitive timing information

Test Coverage

✅ Excellent coverage with new 228-line test file covering:

• Connection delay measurement accuracy
• Metrics structure validation
• Timeout handling
• Reasonable value ranges

Suggestions:

1. Add test for multiple reconnections to ensure timestamps reset properly
2. Test edge case where connection_start_timestamp = 0
3. Consider integration test with actual service

Documentation

⚠️ Missing docstrings for:

• _handle_open parameters (should document connection_start_timestamp)
• websocket.py line 342 could be more specific about timestamp unit (milliseconds)

Summary

Action Required:

1. 🔴 CRITICAL: Implement send_connect_delay_metrics method
2. ⚠️ Add error handling around metrics sending
3. ⚠️ Consider initializing _connection_attempt_start_time to None with validation

Nice to Have:

• Add parameter docstrings
• Add test for multiple reconnections
• More lenient test thresholds for CI

Overall Assessment: Solid implementation and well-tested, but cannot be merged until the missing method is implemented. Once addressed, this will be a valuable addition to metrics observability.

Files Changed: 5 files (+256, -8 lines)