Implement TCP checks #17
Conversation
|
Warning Rate limit exceeded@coolacid has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 22 minutes and 51 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe pull request introduces enhanced health checking capabilities by adding support for TCP protocol checks alongside existing HTTP checks. The changes involve modifying the GitHub Actions workflow to include new health check sources, updating the logging and checking mechanisms, and implementing a new TCP connection verification system. The modifications improve the robustness of endpoint monitoring by expanding the types of connectivity checks that can be performed. Changes
Sequence DiagramsequenceDiagram
participant Workflow as GitHub Actions
participant Index as Health Check Index
participant Checker as HTTP/TCP Checker
participant Logger as Action Logger
Workflow->>Index: Trigger health check
Index->>Index: Validate URL protocol
alt Valid HTTP/HTTPS URL
Index->>Checker: Create HTTPChecker
Checker->>Logger: Log connection attempt
Checker-->>Index: Return connection status
else Valid TCP URL
Index->>Checker: Create TCPChecker
Checker->>Logger: Log connection attempt
Checker-->>Index: Return connection status
else Invalid URL
Index->>Logger: Log warning
Index-->>Workflow: Return false
end
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
log/src/checks/tcp.ts (3)
8-52: Rename nested function to avoid confusion with parent function.The nested function
attemptConnectionhas the same name as the outer function, which can lead to confusion and potential errors. Consider renaming the inner function to improve readability.Apply this diff to rename the inner function:
function attemptConnection(host: string, port: number, logger: ActionLogger, retryCount: number = 5): Promise<boolean> { return new Promise((resolve) => { let attempts = 0; - function attemptConnection() { + function makeConnectionAttempt() { const socket = new net.Socket(); // Set a timeout for the connection attempt socket.setTimeout(1000); // Attempt to connect to the specified host and port socket.on('connect', () => { // If connected, destroy the socket and resolve true socket.removeAllListeners(); // Clean up listeners socket.destroy(); resolve(true); }); // Handle errors that occur during the connection attempt socket.on('error', (err) => { logger.error(`Connection error: ${err.message}`); socket.removeAllListeners(); socket.destroy(); // If it's anything other then a connection refused, retry if (!err.message.includes("ECONNREFUSED")) { handleRetry(); } else { resolve(false); // Resolve the promise on ECONNREFUSED } }); // Handle timeout scenario socket.on('timeout', () => { logger.error('Connection timeout'); socket.destroy(); resolve(false); }); function handleRetry() { if (attempts < retryCount) { attempts += 1; logger.info(`Retrying... (${attempts}/${retryCount})`); - setTimeout(attemptConnection, 500); // Retry after .5 seconds + setTimeout(makeConnectionAttempt, 500); // Retry after .5 seconds } else { resolve(false); // Failed after retrying } } socket.connect(port, host); } - attemptConnection(); + makeConnectionAttempt(); }); }
12-12: Consider making the connection timeout configurable.The connection timeout is hardcoded to 1000 milliseconds. Allowing it to be configurable can enhance flexibility for different network conditions.
62-62: Update log message to reflect correct checker type.The log message says "Created Status Checker," but it should be "Created TCP Checker" to accurately reflect the class.
Apply this diff to correct the log message:
-this.logger.info(`Created Status Checker for ${siteName}`); +this.logger.info(`Created TCP Checker for ${siteName}`);log/src/test/index.test.ts (1)
14-16: Add tests for the newTCPCheckerclass to improve test coverage.Currently, the tests only cover the
HTTPCheckerclass. Including tests forTCPCheckerwill ensure the new TCP checking functionality is properly tested.log/src/checks/http.ts (1)
6-6: Update log message to reflect correct checker type.The log message says "Created Status Checker," but it should be "Created HTTP Checker" to match the class name.
Apply this diff to correct the log message:
-logger.info(`Created Status Checker for ${siteName}`); +logger.info(`Created HTTP Checker for ${siteName}`);log/src/index.ts (1)
48-59: Enhance URL validation robustness.While the current implementation handles basic validation, consider these improvements for better edge case handling:
function isValidUrl(url: string): boolean { try { if (url.startsWith(SUPPORTED_PROTOCOLS.TCP)) { - const [host, port] = url.replace(SUPPORTED_PROTOCOLS.TCP, '').split(':'); - return Boolean(host && port && !Number.isNaN(parseInt(port, 10))); + const parts = url.replace(SUPPORTED_PROTOCOLS.TCP, '').split(':'); + if (parts.length !== 2) return false; + const [host, port] = parts; + const portNum = parseInt(port, 10); + return Boolean( + host && + port && + !Number.isNaN(portNum) && + portNum > 0 && + portNum <= 65535 + ); } new URL(url); return true; } catch { return false; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/health-check.yml(1 hunks)log/src/checks/http.ts(1 hunks)log/src/checks/tcp.ts(1 hunks)log/src/index.ts(3 hunks)log/src/test/index.test.ts(3 hunks)
🔇 Additional comments (2)
log/src/index.ts (2)
1-1: LGTM! Well-structured protocol definitions.The imports and protocol definitions are clean and type-safe. The use of 'as const' ensures compile-time type checking for protocol strings.
Also applies to: 9-10, 42-46
96-109: LGTM! Well-structured protocol-based checker instantiation.The implementation correctly handles different protocols with appropriate error messaging.
| Never SSL->http://oldsplendidfresheclipse.neverssl.com/online/ | ||
| TCPTest->tcp://tcpbin.com:4242 |
There was a problem hiding this comment.
💡 Codebase verification
The current endpoints raise reliability concerns:
- The HTTP endpoint (
neverssl.com) is a static page last modified in 2022, designed for captive portal testing - The TCP endpoint (
tcpbin.com) is a public service without guaranteed availability
Recommendations:
- Use internal endpoints or dedicated health check endpoints with SLAs
- Consider implementing a simple internal HTTP endpoint for basic connectivity tests
- Document the endpoint selection criteria in the health check configuration
🔗 Analysis chain
Review reliability of test endpoints.
The current test endpoints raise some concerns:
neverssl.comis primarily designed for captive portal testing, not for production health checkstcpbin.comis a third-party service that might have:- Reliability issues
- Rate limiting
- No SLA guarantees
Consider using:
- Internal endpoints for TCP checks
- More reliable HTTP endpoints for testing
Run this script to check the reliability of the current endpoints:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the historical uptime of the test endpoints
# Test: Check if the domains are resolvable
echo "Checking DNS resolution..."
host oldsplendidfresheclipse.neverssl.com
host tcpbin.com
# Test: Check TCP connection to tcpbin
echo "Testing TCP connection..."
nc -zv tcpbin.com 4242
Length of output: 494
Script:
#!/bin/bash
# Search for endpoint usage and documentation
echo "Searching for endpoint references..."
rg -i "neverssl|tcpbin" --type yaml --type md
echo "Checking HTTP endpoint..."
curl -I http://oldsplendidfresheclipse.neverssl.com/online/
echo "Looking for health check documentation..."
fd -e md -e txt -e yaml . | xargs rg -i "health.?check|monitoring"
Length of output: 1399
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was so many messy commits on the previous PR, I figured I should clean the whole thing up.
This PR implements the ability to have multiple different types of checks.
It also implements a first alternative check the TCP check. This will make a TCP connection, close and report back if it was successful. Should an unexpected error happen (other then Connection Refused) it will re-try up to 5 times.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Refactor
StatusCheckertoHTTPChecker