remove unnecessary network pings to 8.8.8.8 #1363
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds a workspace dependency resolution for reqwest and changes the network connectivity check to accept a provided Changes
Sequence Diagram(s)sequenceDiagram
actor Caller
participant Analytics as analytics::AnalyticsClient
participant Network as network::is_online(client)
participant Reqwest as reqwest::Client
participant Host as posthog.com
Analytics->>Network: is_online(&self.client).await
Network->>Reqwest: client.get("https://posthog.com/").send().await
Reqwest->>Host: GET /
Host-->>Reqwest: HTTP response (status)
Reqwest-->>Network: Response
Network-->>Analytics: bool (status.is_success())
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
cubic analysis
3 issues found across 3 files • Review in cubic
React with 👍 or 👎 to teach cubic. You can also tag @cubic-dev-ai to give feedback, ask questions, or re-run the review.
crates/network/Cargo.toml
Outdated
| @@ -5,3 +5,4 @@ edition = "2021" | |||
|
|
|||
| [dependencies] | |||
| pinger = { git = "https://github.com/orf/gping", package = "pinger" } | |||
| reqwest.workspace = true | |||
There was a problem hiding this comment.
Cargo.toml syntax is invalid here. The workspace override must be expressed as an inline table (reqwest = { workspace = true }); using the dotted key form without first declaring [dependencies.reqwest] will cause cargo metadata to fail to parse the manifest.
Prompt for AI agents
Address the following comment on crates/network/Cargo.toml at line 8:
<comment>Cargo.toml syntax is invalid here. The workspace override must be expressed as an inline table (`reqwest = { workspace = true }`); using the dotted key form without first declaring `[dependencies.reqwest]` will cause `cargo metadata` to fail to parse the manifest.</comment>
<file context>
@@ -5,3 +5,4 @@ edition = "2021"
[dependencies]
pinger = { git = "https://github.com/orf/gping", package = "pinger" }
+reqwest.workspace = true
</file context>
| reqwest.workspace = true | |
| reqwest = { workspace = true } |
crates/network/src/lib.rs
Outdated
| let target = "8.8.8.8".to_string(); | ||
| let interval = std::time::Duration::from_secs(1); | ||
| let options = pinger::PingOptions::new(target, interval, None); | ||
| let client = Client::builder() |
There was a problem hiding this comment.
A fresh Client is built on every is_online call, defeating reqwest’s connection-pool reuse and introducing overhead. Prefer reusing a single Client instance or passing one in.
Prompt for AI agents
Address the following comment on crates/network/src/lib.rs at line 5:
<comment>A fresh `Client` is built on every `is_online` call, defeating reqwest’s connection-pool reuse and introducing overhead. Prefer reusing a single `Client` instance or passing one in.</comment>
<file context>
@@ -1,16 +1,17 @@
+use reqwest::Client;
+use std::time::Duration;
+
pub async fn is_online() -> bool {
- let target = "8.8.8.8".to_string();
- let interval = std::time::Duration::from_secs(1);
- let options = pinger::PingOptions::new(target, interval, None);
+ let client = Client::builder()
+ .timeout(Duration::from_secs(8))
</file context>
crates/network/src/lib.rs
Outdated
| let client = Client::builder() | ||
| .timeout(Duration::from_secs(8)) | ||
| .build() | ||
| .unwrap(); |
There was a problem hiding this comment.
Calling unwrap() will panic if the Client build fails (e.g., invalid TLS config). A networking utility should not bring down the application on recoverable errors; bubble the error or return false instead.
Prompt for AI agents
Address the following comment on crates/network/src/lib.rs at line 8:
<comment>Calling `unwrap()` will panic if the `Client` build fails (e.g., invalid TLS config). A networking utility should not bring down the application on recoverable errors; bubble the error or return `false` instead.</comment>
<file context>
@@ -1,16 +1,17 @@
+use reqwest::Client;
+use std::time::Duration;
+
pub async fn is_online() -> bool {
- let target = "8.8.8.8".to_string();
- let interval = std::time::Duration::from_secs(1);
- let options = pinger::PingOptions::new(target, interval, None);
+ let client = Client::builder()
+ .timeout(Duration::from_secs(8))
</file context>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
crates/network/src/lib.rs (4)
13-16: Consider HEAD instead of GET to minimize bandwidth and latency.HEAD avoids downloading the response body; good fit for reachability checks.
- match client.get(url).send().await { + match client.head(url).send().await { Ok(resp) if resp.status().is_success() => true, _ => false, }
4-17: Name/semantics mismatch:is_onlinenow tests PostHog reachability, not general connectivity.Confirm all call sites expect “PostHog is reachable” rather than “the device has internet.” If not, either rename (e.g.,
is_posthog_available) or add a short doc comment to clarify intent.If you want, I can scan call sites and prepare a rename PR or add a concise doc comment that explains “Why” we tie the check to PostHog.
11-11: Promote URL to a module-level constant for reuse and clarity.Minor nit to centralize the endpoint.
- let url = "https://posthog.com/"; + let url = POSTHOG_URL;And add this at the top of the file:
const POSTHOG_URL: &str = "https://posthog.com/";
4-17: Optional: reuse a single reqwest Client to avoid rebuilding per call.If
is_onlineis called frequently, reuse a static client to leverage connection pooling and reduce DNS/TLS overhead.Example (adds once_cell):
Cargo.toml:
- once_cell.workspace = true (or a direct dependency)
Code:
use once_cell::sync::Lazy; static CLIENT: Lazy<Client> = Lazy::new(|| { Client::builder() .timeout(Duration::from_secs(8)) .build() .expect("reqwest client") }); pub async fn is_online() -> bool { let url = POSTHOG_URL; match CLIENT.head(url).send().await { Ok(resp) if resp.status().is_success() => true, _ => false, } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
crates/network/Cargo.toml(1 hunks)crates/network/src/lib.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,ts,tsx,rs}
⚙️ CodeRabbit Configuration File
**/*.{js,ts,tsx,rs}: 1. No error handling.
2. No unused imports, variables, or functions.
3. For comments, keep it minimal. It should be about "Why", not "What".
Files:
crates/network/src/lib.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cubic · AI code reviewer
🔇 Additional comments (3)
crates/network/Cargo.toml (1)
7-8: Remove unusedpingerand confirm TLS support for workspacereqwestThe
pingercrate can be safely dropped—no references were found across the workspace.reqwestis correctly overridden by your root[workspace.dependencies]at version 0.12, but no TLS features are explicitly configured. Please verify that the default feature set meets your requirements, or adjust one of the following:
- Enable the appropriate TLS backend under
[workspace.dependencies]in the rootCargo.toml, for example:reqwest = { version = "0.12", features = ["rustls-tls"] }- Or declare
reqwestas a direct dependency incrates/network/Cargo.tomlwith the needed features:[dependencies] reqwest = { workspace = true, features = ["rustls-tls"] }Files to review:
- crates/network/Cargo.toml
- Cargo.toml (root)
Apply this change to drop
pinger:--- a/crates/network/Cargo.toml +++ b/crates/network/Cargo.toml @@ -6,8 +6,7 @@ [dependencies] -pinger = { git = "https://github.com/orf/gping", package = "pinger" } reqwest.workspace = truecrates/network/src/lib.rs (2)
1-2: LGTM on minimal, used imports.Imports are tight and all used. No nits here.
5-8: Avoid panic on client construction (unwrap) — return false on failure.A failed
Client::build()will currently panic; prefer a safe early return to keep this library function non-fatal.- let client = Client::builder() - .timeout(Duration::from_secs(8)) - .build() - .unwrap(); + let client = match Client::builder() + .timeout(Duration::from_secs(8)) + .build() + { + Ok(c) => c, + Err(_) => return false, + };Likely an incorrect or invalid review comment.
steven-mpawulo
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
crates/analytics/src/lib.rs (1)
23-24: Good change: reuse the existing reqwest client in the online checkPassing
&self.clientintohypr_network::is_onlineavoids building a fresh Client and reuses the connection pool. This aligns with prior feedback about client reuse.crates/network/src/lib.rs (1)
1-8: HTTP-based availability check with injected client — LGTMSwitching from ICMP ping to an HTTP check and accepting a
&reqwest::Clientaddresses previous concerns about client reuse and potential panics from client creation.
🧹 Nitpick comments (2)
crates/analytics/src/lib.rs (1)
24-24: Skip the online check in debug builds to avoid unnecessary network I/OYou don’t send analytics in debug builds; avoid the preflight network call in that case.
Apply this diff:
- if !hypr_network::is_online(&self.client).await { + if !cfg!(debug_assertions) && !hypr_network::is_online(&self.client).await { return Ok(()); }crates/network/src/lib.rs (1)
2-6: Target the ingestion host and bound latency with a short per-request timeoutIf the intent is to check PostHog availability (not generic internet), prefer probing the same host used for event ingestion and ensure the call won’t hang indefinitely.
Two small tweaks:
- Use the ingestion host (consistent with the sending endpoint in analytics).
- Add a short per-request timeout to bound the wait.
Apply these diffs:
- let url = "https://posthog.com/"; + // Prefer checking the ingestion host to reflect the dependency we actually use. + let url = "https://us.i.posthog.com/";- match client.get(url).send().await { + match client + .get(url) + .timeout(std::time::Duration::from_secs(3)) + .send() + .await + { Ok(resp) if resp.status().is_success() => true, _ => false, }Note: If the goal is pure connectivity (reachability of the host), counting any HTTP response (even non-2xx) as “online” might reduce false negatives; if the goal is service health, a specific health endpoint (e.g.,
/health) that returns 2xx would be preferable. Confirm the intended semantics and adjust accordingly.Would you like me to probe whether an explicit health endpoint on the ingestion host returns 2xx and is stable across environments?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
crates/analytics/src/lib.rs(1 hunks)crates/network/Cargo.toml(1 hunks)crates/network/src/lib.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/network/Cargo.toml
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,ts,tsx,rs}
⚙️ CodeRabbit Configuration File
**/*.{js,ts,tsx,rs}: 1. No error handling.
2. No unused imports, variables, or functions.
3. For comments, keep it minimal. It should be about "Why", not "What".
Files:
crates/analytics/src/lib.rscrates/network/src/lib.rs
🔇 Additional comments (1)
crates/analytics/src/lib.rs (1)
23-24: No remainingis_online()calls without a client parameter
I ran a global search for bareis_online()invocations (rg -nP --type=rust '\bis_online\s*\(\s*\)') and found zero matches. All call sites now include the required&self.clientargument.
I have removed the ping to 8.8.8.8 and replaced it with a network call to check for posthog availability.
Summary by cubic
Replaced the 8.8.8.8 ICMP ping with an HTTP check to https://posthog.com in is_online. This removes unnecessary network pings and checks the availability of the service we depend on.