implement dedicated service for /metrics#7838
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/actix/metrics_service.rs (1)
52-52: Consider logging the full bind address for clarity.The log message only includes the port, but the
bind_addrvariable (line 30) contains the completehost:portaddress. Logging the full address would provide more complete information for debugging and monitoring.🔎 Suggested improvement
- log::info!("Qdrant metrics listening on {port}"); + log::info!("Qdrant metrics listening on {}", bind_addr);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/actix/metrics_service.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.github/review-rules.md)
**/*.rs: Prefer explicitSomeType::from(x)over implicitx.into()in Rust
In new code, don't usetransmute_from_u8,transmute_to_u8,transmute_from_u8_to_slice,transmute_from_u8_to_mut_slice,transmute_to_u8_slice- usebytemuckorzerocopycrates instead
Prefer explicit exhaustive matches over catch-all_arm in Rust match expressions, so we don't miss handling new enum variants when they are added later (exceptions: tests and benchmarks, or when sure new variants won't affect logic)
Prefer explicit field ignoring using: _over using..in Rust struct destructuring (exceptions: tests and benchmarks, or when sure new fields won't affect logic)
Files:
src/actix/metrics_service.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). (11)
- GitHub Check: rust-tests (macos-latest)
- GitHub Check: rust-tests (windows-latest)
- GitHub Check: test-consensus-compose
- GitHub Check: rust-tests (ubuntu-latest)
- GitHub Check: lint
- GitHub Check: integration-tests
- GitHub Check: integration-tests-consensus
- GitHub Check: test-shard-snapshot-api-s3-minio
- GitHub Check: e2e-tests
- GitHub Check: test-consistency
- GitHub Check: Test Python bindings
🔇 Additional comments (1)
src/actix/metrics_service.rs (1)
12-28: Well-structured metrics service initialization.The initialization pattern is sound:
- Single worker thread is appropriate for a low-traffic metrics endpoint
- Proper use of
web::Data::from(line 18) follows the coding guideline for explicit conversion- Compress middleware and shared state setup follow Actix best practices
| server = if settings.service.enable_tls { | ||
| log::info!( | ||
| "TLS enabled for metrics API (TTL: {})", | ||
| settings | ||
| .tls | ||
| .as_ref() | ||
| .and_then(|tls| tls.cert_ttl) | ||
| .map(|ttl| ttl.to_string()) | ||
| .unwrap_or_else(|| "none".into()), | ||
| ); | ||
|
|
||
| let config = certificate_helpers::actix_tls_server_config(&settings) | ||
| .map_err(io::Error::other)?; | ||
| server.bind_rustls_0_23(bind_addr, config)? | ||
| } else { | ||
| log::info!("TLS disabled for metrics API"); | ||
|
|
||
| server.bind(bind_addr)? | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Fix implicit conversion on line 40.
Line 40 uses .into() which violates the coding guideline. Use explicit String::from instead.
🔎 Proposed fix
settings
.tls
.as_ref()
.and_then(|tls| tls.cert_ttl)
.map(|ttl| ttl.to_string())
- .unwrap_or_else(|| "none".into()),
+ .unwrap_or_else(|| String::from("none")),Based on coding guidelines: "Prefer explicit SomeType::from(x) over implicit x.into() in Rust"
TLS configuration and error handling look correct.
The conditional TLS setup properly propagates errors and the log messages now correctly reference "metrics API" (addressing the previous review feedback).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| server = if settings.service.enable_tls { | |
| log::info!( | |
| "TLS enabled for metrics API (TTL: {})", | |
| settings | |
| .tls | |
| .as_ref() | |
| .and_then(|tls| tls.cert_ttl) | |
| .map(|ttl| ttl.to_string()) | |
| .unwrap_or_else(|| "none".into()), | |
| ); | |
| let config = certificate_helpers::actix_tls_server_config(&settings) | |
| .map_err(io::Error::other)?; | |
| server.bind_rustls_0_23(bind_addr, config)? | |
| } else { | |
| log::info!("TLS disabled for metrics API"); | |
| server.bind(bind_addr)? | |
| }; | |
| server = if settings.service.enable_tls { | |
| log::info!( | |
| "TLS enabled for metrics API (TTL: {})", | |
| settings | |
| .tls | |
| .as_ref() | |
| .and_then(|tls| tls.cert_ttl) | |
| .map(|ttl| ttl.to_string()) | |
| .unwrap_or_else(|| String::from("none")), | |
| ); | |
| let config = certificate_helpers::actix_tls_server_config(&settings) | |
| .map_err(io::Error::other)?; | |
| server.bind_rustls_0_23(bind_addr, config)? | |
| } else { | |
| log::info!("TLS disabled for metrics API"); | |
| server.bind(bind_addr)? | |
| }; |
🤖 Prompt for AI Agents
In src/actix/metrics_service.rs around lines 32 to 50, replace the implicit
conversion using `.into()` on line 40 with an explicit `String::from(...)`;
specifically, change the unwrap_or_else closure so it returns
String::from("none") instead of "none".into(), ensuring the mapped value type is
explicitly constructed as a String.
* implement dedicated service for /metrics * Update src/actix/metrics_service.rs
* implement dedicated service for /metrics * Update src/actix/metrics_service.rs
In production monitoring with services like
alloyit might be tricky to provide proper API key for all internal services.Therefore, it is a common pattern to expose system metrics via independent internal service on a dedicated port.
This PR introduces support for such service in Qdrant. Service is disabled by default and can be enabled with ENV or config.
Service is not protected by API key by-design. If you are intended to use this approach, make sure the service only exposed to internal private network.