implement meta and apply observability to meta #2823
Conversation
|
|
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the 📝 WalkthroughWalkthroughThis pull request encompasses a comprehensive update across multiple Rust packages, focusing on dependency version upgrades, observability enhancements, and minor structural modifications. The changes primarily involve updating dependencies for libraries like Changes
Possibly Related PRs
Suggested Labels
Suggested Reviewers
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 (
|
|
[CHATOPS:HELP] ChatOps commands.
|
kmrmt
requested review from
a team,
kpango and
vankichi
and removed request for
a team
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
rust/libs/observability/src/observability.rs (1)
Line range hint
102-112: Fix incorrect condition in shutdown.The tracer provider shutdown is using the meter's enabled flag instead of the tracer's enabled flag.
Apply this fix:
- if self.config.meter.enabled { + if self.config.tracer.enabled { if let Some(ref provider) = self.tracer_provider { for result in provider.force_flush() { result?;
🧹 Nitpick comments (13)
rust/libs/observability/src/macros.rs (1)
Line range hint
16-150: Consider reducing macro duplication and improving documentation.While the changes themselves are good, here are some suggestions for future improvements:
- The type comments (e.g.,
// typ = f64 or u64) could be moved to the macro documentation.- Consider extracting common patterns into helper macros to reduce duplication.
- Add examples in the documentation showing how to use each instrument type.
rust/bin/meta/src/main.rs (5)
45-50: Consider adding debug logs for interception.
Adding lightweight logs can help trace when requests lack context metadata, aiding debugging.fn intercept(mut req: Request<()>) -> Result<Request<()>, tonic::Status> { let parent_cx = global::get_text_map_propagator(|prop| prop.extract(&MetadataMap(req.metadata()))); + tracing::debug!("Extracted parent context for request: {:?}", parent_cx); req.extensions_mut().insert(parent_cx); Ok(req) }
54-71: Remove or clarify the large commented YAML block.
Uncommenting or relocating it to documentation will keep the codebase clean and maintainable.- // let config_yaml = r#"enabled: false - // endpoint: "" - // ... - // "#; - // - // let observability_cfg = serde_yaml::from_str(config_yaml).unwrap(); +// TODO: Provide references or docs on how to supply a YAML config externally if necessary
72-80: Prefer making the endpoint configurable.
Hardcoding"http://127.0.0.1:4318"might limit deployment flexibility. Consider environment variables or config.let observability_cfg = Config::new() .enabled(true) .attribute(SERVICE_NAME, "vald-lb-gateway") .attribute("target_pod", "target_pod") .attribute("target_node", "target_node") .attribute("exported_kubernetes_namaspace", "default") .attribute("kubernetes_name", "vald-lb-gateway") - .endpoint("http://127.0.0.1:4318") + .endpoint(std::env::var("OTEL_EXPORTER_ENDPOINT").unwrap_or_else(|_| "http://127.0.0.1:4318".to_string())) .tracer(Tracer::new().enabled(true));
83-86: Expose path configuration to avoid hardcoding.
"/tmp/meta/database"is likely environment-specific. Using config or environment variables improves portability.
87-91: Consider optional concurrency or resource limits for the Tonic server.
For high-load scenarios, Tonic provides knobs like concurrency limits or compression. Might be worth exploring.rust/bin/meta/src/handler/meta.rs (1)
60-105:Meta::setmethod exhibits robust validation checks.
Nice usage of separate error messages for missing key vs. missing value. Consider logging or measuring value size for better diagnostic insight.rust/bin/meta/src/handler.rs (2)
22-24: Add documentation for the public Meta struct.The
Metastruct is a public API and should be documented with rustdoc comments explaining its purpose, fields, and usage.Add documentation above the struct:
+/// Meta provides key-value storage functionality using the kv crate. +/// It maintains a thread-safe store and a bucket for metadata operations. pub struct Meta { store: Arc<Store>, bucket: Bucket<'static, Raw, Raw>, }
27-33: Enhance error handling and configuration.The implementation has several areas for improvement:
- The bucket name is hardcoded
- No cleanup implementation for resources
- Basic error handling
Consider these improvements:
impl Meta { + const DEFAULT_BUCKET: &'static str = "meta_bucket"; + pub fn new(cfg_path: &str) -> Result<Self, kv::Error> { let cfg = Config::new(cfg_path); let store = Arc::new(Store::new(cfg)?); - let bucket = store.bucket::<Raw, Raw>(Some("meta_bucket"))?; + let bucket = store.bucket::<Raw, Raw>(Some(Self::DEFAULT_BUCKET))?; Ok(Meta { store, bucket }) } + + /// Ensures proper cleanup of resources + pub fn cleanup(&self) -> Result<(), kv::Error> { + self.store.flush()?; + Ok(()) + } }rust/libs/observability/src/observability.rs (2)
54-65: Consider validating endpoint URLs earlier.The endpoint URL parsing and joining is performed during provider setup, which could lead to runtime errors. Consider validating URLs during config initialization.
71-80: Enhance tracer provider configuration with batch options.The tracer provider could benefit from configurable batch options for better performance tuning.
Consider adding batch configuration:
let provider = TracerProvider::builder() - .with_batch_exporter(exporter, runtime::Tokio) + .with_batch_exporter( + exporter, + runtime::Tokio, + opentelemetry_sdk::trace::BatchConfig::default() + .with_max_queue_size(self.config.tracer.batch_size) + .with_scheduled_delay(self.config.tracer.batch_delay), + )rust/bin/meta/src/test_client.rs (1)
51-135: Reduce code duplication in request creation.The code for creating requests is duplicated across different operations.
Consider extracting common functionality:
fn create_key_request(key: &str) -> Request<meta::Key> { let mut request = Request::new(meta::Key { key: key.to_string(), }); inject_trace_context(&mut request); request } fn create_key_value_request(key: &str, value: &[u8]) -> Request<meta::KeyValue> { let mut request = Request::new(meta::KeyValue { key: Some(meta::Key { key: key.to_string(), }), value: Some(meta::Value { value: Some(Any { type_url: "".to_string(), value: value.to_vec(), }), }), }); inject_trace_context(&mut request); request }rust/bin/meta/Cargo.toml (1)
22-30: Consider adding version constraints for dependencies.Some dependencies might benefit from version constraints to ensure compatibility.
Consider using these constraints:
-kv = "0.24.0" +kv = ">=0.24.0, <0.25.0" -opentelemetry = "0.27.1" +opentelemetry = { version = "0.27.1", features = ["trace", "metrics"] }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
rust/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
rust/bin/agent/Cargo.toml(1 hunks)rust/bin/meta/Cargo.toml(1 hunks)rust/bin/meta/src/handler.rs(1 hunks)rust/bin/meta/src/handler/meta.rs(1 hunks)rust/bin/meta/src/main.rs(1 hunks)rust/bin/meta/src/test_client.rs(1 hunks)rust/libs/algorithm/Cargo.toml(1 hunks)rust/libs/algorithms/ngt/Cargo.toml(1 hunks)rust/libs/algorithms/qbg/Cargo.toml(1 hunks)rust/libs/observability/Cargo.toml(1 hunks)rust/libs/observability/src/lib.rs(1 hunks)rust/libs/observability/src/macros.rs(2 hunks)rust/libs/observability/src/observability.rs(2 hunks)rust/libs/proto/Cargo.toml(1 hunks)rust/rust-toolchain(1 hunks)versions/RUST_VERSION(1 hunks)
✅ Files skipped from review due to trivial changes (7)
- rust/rust-toolchain
- versions/RUST_VERSION
- rust/libs/algorithm/Cargo.toml
- rust/libs/algorithms/ngt/Cargo.toml
- rust/libs/algorithms/qbg/Cargo.toml
- rust/bin/agent/Cargo.toml
- rust/libs/proto/Cargo.toml
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: check-format-diff
- GitHub Check: build / build
- GitHub Check: Analyze (go)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (20)
rust/libs/observability/src/macros.rs (7)
79-87: LGTM! Consistent API update for observable counter.The changes maintain consistency with the updated OpenTelemetry API pattern.
93-98: LGTM! Consistent API update for up-down counter.The changes maintain consistency with the updated OpenTelemetry API pattern.
104-112: LGTM! Consistent API update for observable up-down counter.The changes maintain consistency with the updated OpenTelemetry API pattern.
118-123: LGTM! Consistent API update for histogram.The changes maintain consistency with the updated OpenTelemetry API pattern.
129-134: LGTM! Consistent API update for gauge.The changes maintain consistency with the updated OpenTelemetry API pattern.
140-148: LGTM! Consistent API update for observable gauge.The changes maintain consistency with the updated OpenTelemetry API pattern.
68-73: LGTM! Macro hygiene improvement and API update.The changes correctly:
- Use
$crate::pastefor proper macro hygiene- Update to the newer
.build()method from OpenTelemetry APILet's verify the OpenTelemetry API compatibility:
✅ Verification successful
✅ Verified: OpenTelemetry API usage is correct
The changes align with OpenTelemetry 0.27.x API patterns and Rust macro best practices. The
.build()method is the correct API for counter instrument creation.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check OpenTelemetry version and API changes cargo tree -p opentelemetry --depth=0Length of output: 135
Script:
#!/bin/bash # First, check Cargo.toml for OpenTelemetry version rg "opentelemetry.*=" rust/libs/observability/Cargo.toml # Then, look for other counter usage patterns to verify API consistency ast-grep --pattern 'counter().with_description($_).with_unit($_).build()'Length of output: 397
Script:
#!/bin/bash # Search for meter counter usage patterns rg "\.counter\(" -A 3 rust/libs/observability/src/Length of output: 52
rust/bin/meta/src/main.rs (5)
19-21: Good job importing the new observability artifacts.
No issues identified; these imports align well with the observability setup.
22-26: All opentelemetry-related imports look correct.
You’re bringing in the necessary crates for propagator functionality.
27-28: StructMetadataMapis a sound approach for implementingExtractor.
It neatly wraps Tonic’sMetadataMapfor custom tracing needs.
29-43: Extractor trait implementation looks solid.
Binary and ASCII keys are handled identically, which might be intentional. Please confirm that including binary-based keys in the same collection is acceptable.
95-95: Verify error handling onobservability.shutdown().
Any error returned here is potentially swallowed. Consider logging or capturing it for troubleshooting.rust/bin/meta/src/handler/meta.rs (3)
17-20: New imports for kv, defer, opentelemetry, and observability appear consistent.
They match the use of contexts and spans.
27-58:Meta::getmethod handles key lookups gracefully.
The structured error handling, especially for missing keys and DB failures, is well-implemented. Ensure that the stored data aligns with theprost_types::Anyapproach to avoid type mismatches.
107-128:Meta::deletemethod is straightforward and consistent withgetandset.
No issues found.rust/libs/observability/src/lib.rs (1)
21-22: Exportingpasteunder doc(hidden) is acceptable.
If the crate usage is strictly internal, hiding it in docs is fine.rust/bin/meta/Cargo.toml (1)
32-35: LGTM! Binary target configuration looks good.The test client binary configuration is properly set up with the correct path and documentation settings.
rust/libs/observability/Cargo.toml (3)
27-28: LGTM! Runtime dependencies are up-to-date.The updates to tokio and serde_json are minor version bumps within stable releases, maintaining backward compatibility.
32-33: LGTM! Utility dependencies are up-to-date.The updates to anyhow and url are minor version bumps within stable releases.
24-26: Verify OpenTelemetry migration requirements.The significant version jump in OpenTelemetry packages (0.23/0.16 → 0.27.x) may introduce breaking changes. Please ensure that all OpenTelemetry API usage has been updated according to the migration guides.
Run this script to check for potential breaking changes in OpenTelemetry usage:
Also applies to: 29-29
✅ Verification successful
OpenTelemetry update appears safe ✅
The codebase uses stable OpenTelemetry APIs (span kinds, tracer provider, and standard exporters) that are unlikely to have breaking changes between versions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for OpenTelemetry API usage patterns that might need updates # Common patterns that often change between versions patterns=( "opentelemetry::trace::" "opentelemetry::metrics::" "opentelemetry_sdk::trace::" "opentelemetry_otlp::" ) echo "Searching for OpenTelemetry API usage patterns..." for pattern in "${patterns[@]}"; do echo -e "\nChecking pattern: $pattern" rg "$pattern" -t rust doneLength of output: 1234
kmrmt
force-pushed
the
feature/meta/implement-apis2
branch
from
726c7e2 to
2e979e0
Compare
Deploying vald with
|
| Latest commit: |
7e85aa2
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://9eaccbfd.vald.pages.dev |
| Branch Preview URL: | https://feature-meta-implement-apis2.vald.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
rust/libs/observability/src/observability.rs (2)
54-65: Consider adding retry logic for exporter initialization.The metric exporter initialization could fail if the endpoint is temporarily unavailable. Consider adding retry logic with backoff.
+ const MAX_RETRIES: u32 = 3; + const RETRY_DELAY_MS: u64 = 1000; + let mut retries = 0; + let exporter = loop { + match MetricExporter::builder() + .with_tonic() + .with_endpoint(Url::parse(obj.config.endpoint.as_str())?.join("/v1/metrics")?.as_str()) + .with_timeout(obj.config.meter.export_timeout_duration) + .build() { + Ok(e) => break e, + Err(e) if retries < MAX_RETRIES => { + retries += 1; + tokio::time::sleep(std::time::Duration::from_millis(RETRY_DELAY_MS)).await; + continue; + } + Err(e) => return Err(e.into()), + } + };
71-80: Consider validating trace configuration before initialization.The tracer configuration should be validated before initializing the provider to ensure all required fields are present and valid.
+ fn validate_trace_config(&self) -> Result<()> { + if self.config.endpoint.is_empty() { + return Err(anyhow::anyhow!("Trace endpoint is required")); + } + Ok(()) + } + if obj.config.tracer.enabled { + obj.validate_trace_config()?; let exporter = SpanExporter::builder()rust/bin/meta/src/handler/meta.rs (1)
119-128: Consider adding deletion confirmation.The delete operation should optionally return whether the key existed before deletion.
match self.bucket.remove(&raw_key) { - Ok(_) => { - ctx.span().add_event("Key deleted successfully", vec![KeyValue::new("key", key)]); + Ok(old_value) => { + let existed = old_value.is_some(); + ctx.span().add_event( + "Key deleted successfully", + vec![ + KeyValue::new("key", key), + KeyValue::new("existed", existed.to_string()), + ], + ); Ok(tonic::Response::new(Empty {})) },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
rust/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
rust/bin/agent/Cargo.toml(1 hunks)rust/bin/meta/Cargo.toml(1 hunks)rust/bin/meta/src/handler.rs(1 hunks)rust/bin/meta/src/handler/meta.rs(1 hunks)rust/bin/meta/src/main.rs(1 hunks)rust/bin/meta/src/test_client.rs(1 hunks)rust/libs/algorithm/Cargo.toml(1 hunks)rust/libs/algorithms/ngt/Cargo.toml(1 hunks)rust/libs/algorithms/qbg/Cargo.toml(1 hunks)rust/libs/observability/Cargo.toml(1 hunks)rust/libs/observability/src/lib.rs(1 hunks)rust/libs/observability/src/macros.rs(2 hunks)rust/libs/observability/src/observability.rs(2 hunks)rust/libs/proto/Cargo.toml(1 hunks)rust/rust-toolchain(1 hunks)versions/RUST_VERSION(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- versions/RUST_VERSION
- rust/rust-toolchain
- rust/libs/algorithm/Cargo.toml
- rust/libs/observability/src/lib.rs
- rust/bin/meta/src/handler.rs
- rust/libs/algorithms/qbg/Cargo.toml
- rust/libs/algorithms/ngt/Cargo.toml
- rust/bin/meta/src/test_client.rs
- rust/libs/proto/Cargo.toml
- rust/bin/meta/Cargo.toml
- rust/bin/agent/Cargo.toml
- rust/libs/observability/Cargo.toml
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (7)
rust/libs/observability/src/macros.rs (5)
68-73: LGTM! API updates for Counter macros.The changes correctly update the Counter macros to:
- Use
$crate::paste::paste!for proper macro resolution- Use
.build()instead of.init()to align with OpenTelemetry SDK updatesAlso applies to: 79-87
93-98: LGTM! API updates for UpdownCounter macros.The changes correctly update the UpdownCounter macros to match the Counter macro changes.
Also applies to: 104-112
118-123: LGTM! API updates for Histogram macro.The changes correctly update the Histogram macro to match the API updates.
129-134: LGTM! API updates for Gauge macros.The changes correctly update the Gauge macros to match the API updates.
Also applies to: 140-148
73-73: Verify OpenTelemetry SDK version compatibility.The change from
.init()to.build()across all instrument types suggests an API update in the OpenTelemetry SDK. Let's verify the SDK version in use.Also applies to: 87-87, 98-98, 112-112, 123-123, 134-134, 148-148
✅ Verification successful
OpenTelemetry SDK version is compatible with
.build()APIThe codebase uses OpenTelemetry SDK 0.27.1, which fully supports the
.build()method for instrument initialization.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check OpenTelemetry SDK version in Cargo.toml files # Find all Cargo.toml files and check for opentelemetry dependency versions fd Cargo.toml | xargs rg -A 5 "opentelemetry"Length of output: 1209
rust/bin/meta/src/main.rs (2)
27-43: LGTM! Well-implemented context propagation.The
MetadataMapimplementation correctly handles both ASCII and Binary metadata keys, ensuring proper context propagation.
45-50: LGTM! Clean interceptor implementation.The interceptor correctly extracts and propagates the parent context through request extensions.
kmrmt
force-pushed
the
feature/meta/implement-apis2
branch
from
4c7e526 to
997eadc
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
rust/bin/meta/src/main.rs (2)
72-80: 🛠️ Refactor suggestionAvoid hardcoding observability configuration.
The observability configuration should be moved to environment variables or configuration files for better maintainability and flexibility.
83-84: 🛠️ Refactor suggestionAvoid hardcoding server configuration.
The server address and database path should be configurable through environment variables or configuration files.
rust/bin/meta/src/handler/meta.rs (2)
40-43: 🛠️ Refactor suggestionAvoid hardcoding type URL in Any message.
The type URL for the Any message should be configurable or derived from the actual message type.
93-94: 🛠️ Refactor suggestionConsider adding input size validation.
The key and value sizes should be validated to prevent potential DoS attacks through large inputs.
🧹 Nitpick comments (1)
rust/bin/meta/src/main.rs (1)
45-50: Add error handling for context extraction.While the context propagation is implemented correctly, consider adding error handling for cases where context extraction fails.
fn intercept(mut req: Request<()>) -> Result<Request<()>, tonic::Status> { - let parent_cx = - global::get_text_map_propagator(|prop| prop.extract(&MetadataMap(req.metadata()))); + let parent_cx = global::get_text_map_propagator(|prop| { + let ctx = prop.extract(&MetadataMap(req.metadata())); + tracing::debug!("Extracted parent context: {:?}", ctx); + ctx + }); req.extensions_mut().insert(parent_cx); Ok(req) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
rust/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
rust/bin/agent/Cargo.toml(1 hunks)rust/bin/meta/Cargo.toml(1 hunks)rust/bin/meta/src/handler.rs(1 hunks)rust/bin/meta/src/handler/meta.rs(1 hunks)rust/bin/meta/src/main.rs(1 hunks)rust/bin/meta/src/test_client.rs(1 hunks)rust/libs/algorithm/Cargo.toml(1 hunks)rust/libs/algorithms/ngt/Cargo.toml(1 hunks)rust/libs/algorithms/qbg/Cargo.toml(1 hunks)rust/libs/observability/Cargo.toml(1 hunks)rust/libs/observability/src/lib.rs(1 hunks)rust/libs/observability/src/macros.rs(2 hunks)rust/libs/observability/src/observability.rs(2 hunks)rust/libs/proto/Cargo.toml(1 hunks)rust/rust-toolchain(1 hunks)rust/rust-toolchain.toml(1 hunks)versions/RUST_VERSION(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (15)
- versions/RUST_VERSION
- rust/rust-toolchain
- rust/rust-toolchain.toml
- rust/libs/observability/src/lib.rs
- rust/bin/meta/src/test_client.rs
- rust/libs/algorithm/Cargo.toml
- rust/libs/observability/src/macros.rs
- rust/libs/proto/Cargo.toml
- rust/bin/meta/src/handler.rs
- rust/bin/agent/Cargo.toml
- rust/libs/algorithms/ngt/Cargo.toml
- rust/bin/meta/Cargo.toml
- rust/libs/observability/Cargo.toml
- rust/libs/algorithms/qbg/Cargo.toml
- rust/libs/observability/src/observability.rs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (3)
rust/bin/meta/src/main.rs (2)
19-27: LGTM!The imports are well-organized and the struct declaration follows Rust naming conventions.
29-43: LGTM!The
Extractortrait implementation is robust, handling both ASCII and Binary metadata keys while properly managing potential errors.rust/bin/meta/src/handler/meta.rs (1)
108-129: LGTM!The delete method implementation is clean, with proper error handling and logging.
kmrmt
force-pushed
the
feature/meta/implement-apis2
branch
2 times, most recently
from
e6201e1 to
1e635cb
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
rust/libs/observability/src/observability.rs (1)
106-113:⚠️ Potential issueFix incorrect condition in tracer shutdown.
There's a critical bug in the shutdown logic. The condition checks
meter.enabledinstead oftracer.enabled, which could prevent proper shutdown of the tracer provider.- if self.config.meter.enabled { + if self.config.tracer.enabled { if let Some(ref provider) = self.tracer_provider { for result in provider.force_flush() { result?;
♻️ Duplicate comments (1)
rust/bin/meta/src/handler/meta.rs (1)
33-34: 🛠️ Refactor suggestionAdd input validation for key size.
The key size should be validated to prevent potential DoS attacks.
let key = request.into_inner().key; + const MAX_KEY_SIZE: usize = 1024; // 1KB + if key.len() > MAX_KEY_SIZE { + ctx.span().add_event("Invalid argument", vec![KeyValue::new("error", "Key size exceeds limit")]); + return Err(tonic::Status::invalid_argument("Key size exceeds limit")); + } let raw_key = Raw::from(key.as_bytes());
🧹 Nitpick comments (10)
rust/bin/meta/src/main.rs (3)
29-43: Consider improving error handling in the Extractor implementation.The
getmethod silently ignores string conversion errors withto_str().ok(). Consider logging these errors or propagating them to help with debugging invalid metadata.impl<'a> Extractor for MetadataMap<'a> { fn get(&self, key: &str) -> Option<&str> { - self.0.get(key).and_then(|metadata| metadata.to_str().ok()) + self.0.get(key).and_then(|metadata| { + metadata.to_str().map_err(|e| { + tracing::warn!("Invalid metadata value for key {}: {}", key, e); + e + }).ok() + }) }
45-50: Add observability to the interceptor.The interceptor should include tracing and metrics to monitor its performance and track any context extraction issues.
fn intercept(mut req: Request<()>) -> Result<Request<()>, tonic::Status> { + let start = std::time::Instant::now(); + let span = tracing::info_span!("intercept_request"); + let _guard = span.enter(); + let parent_cx = global::get_text_map_propagator(|prop| prop.extract(&MetadataMap(req.metadata()))); req.extensions_mut().insert(parent_cx); + + tracing::debug!( + duration_ms = start.elapsed().as_millis() as u64, + "Request intercepted successfully" + ); Ok(req) }
87-95: Add graceful shutdown handling.The server should handle graceful shutdown to ensure all in-flight requests are completed and resources are properly cleaned up.
+ let (tx, rx) = tokio::sync::oneshot::channel(); + let server = Server::builder() .add_service(proto::meta::v1::meta_server::MetaServer::with_interceptor( meta, intercept, )) - .serve(addr) + .serve_with_shutdown(addr, async { + rx.await.ok(); + tracing::info!("Shutting down server..."); + }); + tokio::select! { + _ = server => { + tracing::info!("Server stopped"); + } + _ = tokio::signal::ctrl_c() => { + tracing::info!("Received ctrl+c signal"); + tx.send(()).ok(); + } + } observability.shutdown()?;rust/libs/observability/src/observability.rs (2)
54-65: Add error handling for URL parsing.The URL parsing could fail silently. Consider adding explicit error messages.
- .with_endpoint(Url::parse(obj.config.endpoint.as_str())?.join("/v1/metrics")?.as_str()) + .with_endpoint( + Url::parse(obj.config.endpoint.as_str()) + .map_err(|e| anyhow::anyhow!("Failed to parse endpoint URL: {}", e))? + .join("/v1/metrics") + .map_err(|e| anyhow::anyhow!("Failed to join metrics path: {}", e))? + .as_str() + )
71-80: Apply consistent error handling for URL parsing.Apply the same error handling improvement as suggested for the meter provider.
- .with_endpoint(Url::parse(obj.config.endpoint.as_str())?.join("/v1/traces")?.as_str()) + .with_endpoint( + Url::parse(obj.config.endpoint.as_str()) + .map_err(|e| anyhow::anyhow!("Failed to parse endpoint URL: {}", e))? + .join("/v1/traces") + .map_err(|e| anyhow::anyhow!("Failed to join traces path: {}", e))? + .as_str() + )rust/bin/meta/src/handler/meta.rs (5)
17-17: Consider using specific imports instead of wildcard.Replace
use kv::*;with specific imports to improve code clarity and prevent potential naming conflicts.-use kv::*; +use kv::{Raw, Error}; // Import only the types you need
54-57: Enhance error messages with more context.The database error message could include more context about the operation being performed.
Err(e) => { - ctx.span().add_event("Database error", vec![KeyValue::new("error", e.to_string())]); - Err(tonic::Status::internal(format!("Database error: {}", e))) + let error_msg = format!("Failed to get value for key '{}': {}", key, e); + ctx.span().add_event("Database error", vec![ + KeyValue::new("error", error_msg.clone()), + KeyValue::new("operation", "get"), + ]); + Err(tonic::Status::internal(error_msg)) }
124-127: Enhance error messages in delete method.The error message could include more context about the operation being performed.
Err(e) => { - ctx.span().add_event("Failed to delete key", vec![KeyValue::new("error", e.to_string())]); - Err(tonic::Status::internal(format!("Failed to delete key: {}", e))) + let error_msg = format!("Failed to delete key '{}': {}", key, e); + ctx.span().add_event("Failed to delete key", vec![ + KeyValue::new("error", error_msg.clone()), + KeyValue::new("operation", "delete"), + ]); + Err(tonic::Status::internal(error_msg)) }
24-24: Add documentation for the public API.Consider adding documentation comments that describe the purpose, parameters, and return values of each method in the Meta trait implementation.
+/// Implementation of the Meta service for key-value operations. +/// +/// This implementation provides methods for getting, setting, and deleting key-value pairs +/// with OpenTelemetry instrumentation for observability. impl meta_server::Meta for super::Meta {
17-20: Extract common constants and validation logic.Consider moving the validation logic and constants to a separate module to promote code reuse and maintainability.
+mod validation { + pub const MAX_KEY_SIZE: usize = 1024; // 1KB + pub const MAX_VALUE_SIZE: usize = 1024 * 1024; // 1MB + + pub fn validate_key_size(key: &[u8], ctx: &Context) -> Result<(), tonic::Status> { + if key.len() > MAX_KEY_SIZE { + ctx.span().add_event("Invalid argument", vec![KeyValue::new("error", "Key size exceeds limit")]); + return Err(tonic::Status::invalid_argument("Key size exceeds limit")); + } + Ok(()) + } + + pub fn validate_value_size(value: &[u8], ctx: &Context) -> Result<(), tonic::Status> { + if value.len() > MAX_VALUE_SIZE { + ctx.span().add_event("Invalid argument", vec![KeyValue::new("error", "Value size exceeds limit")]); + return Err(tonic::Status::invalid_argument("Value size exceeds limit")); + } + Ok(()) + } +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
rust/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
rust/bin/agent/Cargo.toml(1 hunks)rust/bin/meta/Cargo.toml(1 hunks)rust/bin/meta/src/handler.rs(1 hunks)rust/bin/meta/src/handler/meta.rs(1 hunks)rust/bin/meta/src/main.rs(1 hunks)rust/bin/meta/src/test_client.rs(1 hunks)rust/libs/algorithm/Cargo.toml(1 hunks)rust/libs/algorithms/ngt/Cargo.toml(1 hunks)rust/libs/algorithms/qbg/Cargo.toml(1 hunks)rust/libs/observability/Cargo.toml(1 hunks)rust/libs/observability/src/lib.rs(1 hunks)rust/libs/observability/src/macros.rs(2 hunks)rust/libs/observability/src/observability.rs(2 hunks)rust/libs/proto/Cargo.toml(1 hunks)rust/rust-toolchain(1 hunks)rust/rust-toolchain.toml(1 hunks)versions/RUST_VERSION(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (14)
- rust/rust-toolchain
- versions/RUST_VERSION
- rust/rust-toolchain.toml
- rust/libs/algorithm/Cargo.toml
- rust/libs/observability/src/lib.rs
- rust/bin/meta/src/handler.rs
- rust/bin/meta/src/test_client.rs
- rust/libs/algorithms/qbg/Cargo.toml
- rust/libs/observability/src/macros.rs
- rust/bin/agent/Cargo.toml
- rust/bin/meta/Cargo.toml
- rust/libs/algorithms/ngt/Cargo.toml
- rust/libs/proto/Cargo.toml
- rust/libs/observability/Cargo.toml
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (6)
rust/bin/meta/src/main.rs (3)
19-27: LGTM! Well-structured imports and struct definition.The imports are well-organized and the MetadataMap struct is properly designed with lifetime annotations.
72-80: Avoid hardcoding observability configuration.The observability configuration is currently hardcoded. Consider moving this to a configuration file or environment variables for better maintainability and flexibility.
83-84: Avoid hardcoding server configuration.The server address and database path should be configurable through environment variables or configuration files.
rust/libs/observability/src/observability.rs (1)
16-19: Verify removal of Context import.The removal of
Contextfrom anyhow imports suggests it's no longer used. Let's verify there are no remaining uses.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for any remaining .context() or Context usage rg --type rust 'Context|\.context\(' rust/libs/observability/Length of output: 477
Import Verification: Context Removal is Correct
The search confirms that the
Contextremoved from theanyhowimports inrust/libs/observability/src/observability.rsis no longer used in that file. The only remainingContextusages appear inrust/libs/observability/src/macros.rsasopentelemetry::Context, which is intentional and distinct. Therefore, the removal is valid.rust/bin/meta/src/handler/meta.rs (2)
40-43: Avoid hardcoding type URL in Any message.The type URL for the Any message should be configurable or derived from the actual message type.
- type_url: "type.googleapis.com/your.package.MessageType".to_string(), + type_url: std::env::var("META_VALUE_TYPE_URL") + .unwrap_or_else(|_| "type.googleapis.com/vald.v1.MetaValue".to_string()),
93-94: Consider adding input size validation.The key and value sizes should be validated to prevent potential DoS attacks through large inputs.
+ const MAX_KEY_SIZE: usize = 1024; // 1KB + const MAX_VALUE_SIZE: usize = 1024 * 1024; // 1MB + + if key.len() > MAX_KEY_SIZE { + ctx.span().add_event("Invalid argument", vec![KeyValue::new("error", "Key size exceeds limit")]); + return Err(tonic::Status::invalid_argument("Key size exceeds limit")); + } + if value.len() > MAX_VALUE_SIZE { + ctx.span().add_event("Invalid argument", vec![KeyValue::new("error", "Value size exceeds limit")]); + return Err(tonic::Status::invalid_argument("Value size exceeds limit")); + } + let raw_key = Raw::from(key.as_bytes()); let raw_value = sled::IVec::from(value);
kmrmt
force-pushed
the
feature/meta/implement-apis2
branch
from
1e635cb to
31da4e8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
rust/bin/meta/src/main.rs (1)
45-50: Add error handling and documentation for the interceptor.While the implementation is correct, consider these improvements:
- Add documentation explaining the function's purpose
- Add error handling for context extraction failures
+/// Intercepts incoming requests to extract and propagate tracing context. +/// This ensures proper trace context propagation across service boundaries. fn intercept(mut req: Request<()>) -> Result<Request<()>, tonic::Status> { - let parent_cx = - global::get_text_map_propagator(|prop| prop.extract(&MetadataMap(req.metadata()))); + let parent_cx = global::get_text_map_propagator(|prop| { + prop.extract(&MetadataMap(req.metadata())) + }); req.extensions_mut().insert(parent_cx); Ok(req) }rust/bin/meta/src/handler/meta.rs (1)
71-91: Simplify nested matching for better readability.The nested matching structure for key-value validation can be simplified using combinators.
- let key = match key_value.key { - Some(k) => k.key, - None => { - ctx.span().add_event("Invalid argument", vec![KeyValue::new("error", "Key is missing")]); - return Err(tonic::Status::invalid_argument("Key is missing")); - } - }; - - let value = match key_value.value { - Some(v) => match v.value { - Some(any_value) => any_value.value, - None => { - ctx.span().add_event("Invalid argument", vec![KeyValue::new("error", "Value is missing")]); - return Err(tonic::Status::invalid_argument("Value is missing")); - } - }, - None => { - ctx.span().add_event("Invalid argument", vec![KeyValue::new("error", "Value is missing")]); - return Err(tonic::Status::invalid_argument("Value is missing")); - } - }; + let key = key_value.key.ok_or_else(|| { + ctx.span().add_event("Invalid argument", vec![KeyValue::new("error", "Key is missing")]); + tonic::Status::invalid_argument("Key is missing") + })?.key; + + let value = key_value.value + .and_then(|v| v.value) + .ok_or_else(|| { + ctx.span().add_event("Invalid argument", vec![KeyValue::new("error", "Value is missing")]); + tonic::Status::invalid_argument("Value is missing") + })?.value;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
rust/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
rust/bin/agent/Cargo.toml(1 hunks)rust/bin/meta/Cargo.toml(1 hunks)rust/bin/meta/src/handler.rs(1 hunks)rust/bin/meta/src/handler/meta.rs(1 hunks)rust/bin/meta/src/main.rs(1 hunks)rust/bin/meta/src/test_client.rs(1 hunks)rust/libs/algorithm/Cargo.toml(1 hunks)rust/libs/algorithms/ngt/Cargo.toml(1 hunks)rust/libs/algorithms/qbg/Cargo.toml(1 hunks)rust/libs/observability/Cargo.toml(1 hunks)rust/libs/observability/src/lib.rs(1 hunks)rust/libs/observability/src/macros.rs(2 hunks)rust/libs/observability/src/observability.rs(2 hunks)rust/libs/proto/Cargo.toml(1 hunks)rust/rust-toolchain(1 hunks)rust/rust-toolchain.toml(1 hunks)versions/RUST_VERSION(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (15)
- rust/libs/observability/src/lib.rs
- rust/rust-toolchain.toml
- rust/rust-toolchain
- versions/RUST_VERSION
- rust/libs/observability/src/macros.rs
- rust/bin/meta/src/handler.rs
- rust/libs/algorithm/Cargo.toml
- rust/bin/meta/src/test_client.rs
- rust/libs/algorithms/qbg/Cargo.toml
- rust/libs/observability/src/observability.rs
- rust/libs/proto/Cargo.toml
- rust/libs/algorithms/ngt/Cargo.toml
- rust/bin/agent/Cargo.toml
- rust/bin/meta/Cargo.toml
- rust/libs/observability/Cargo.toml
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: check-format-diff
- GitHub Check: build / build
- GitHub Check: Analyze (go)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (9)
rust/bin/meta/src/main.rs (5)
19-43: LGTM! Clean implementation of metadata extraction.The
MetadataMapstruct and itsExtractorimplementation are well-structured and handle both ASCII and Binary metadata keys correctly.
54-71: Remove or implement TODO comments.The commented-out YAML configuration suggests an incomplete implementation. Either implement the YAML configuration loading or remove the commented code to maintain cleaner source files.
72-80: Avoid hardcoding observability configuration.The observability configuration is currently hardcoded. Consider moving this to a configuration file or environment variables for better maintainability and flexibility.
83-84: Avoid hardcoding server configuration.The server address and database path should be configurable through environment variables or configuration files.
85-96: LGTM! Well-structured server setup with graceful shutdown.The server setup correctly includes the interceptor, and the observability shutdown is properly handled.
rust/bin/meta/src/handler/meta.rs (4)
17-24: LGTM! Well-structured imports and trait implementation.The code demonstrates good organization with proper imports for key-value operations, deferred cleanup, and observability integration.
40-43: Avoid hardcoding type URL in Any message.The type URL for the Any message should be configurable or derived from the actual message type.
- type_url: "type.googleapis.com/your.package.MessageType".to_string(), + type_url: std::env::var("META_VALUE_TYPE_URL") + .unwrap_or_else(|_| "type.googleapis.com/vald.v1.MetaValue".to_string()),
93-94: Consider adding input size validation.The key and value sizes should be validated to prevent potential DoS attacks through large inputs.
+ const MAX_KEY_SIZE: usize = 1024; // 1KB + const MAX_VALUE_SIZE: usize = 1024 * 1024; // 1MB + + if key.len() > MAX_KEY_SIZE { + ctx.span().add_event("Invalid argument", vec![KeyValue::new("error", "Key size exceeds limit")]); + return Err(tonic::Status::invalid_argument("Key size exceeds limit")); + } + if value.len() > MAX_VALUE_SIZE { + ctx.span().add_event("Invalid argument", vec![KeyValue::new("error", "Value size exceeds limit")]); + return Err(tonic::Status::invalid_argument("Value size exceeds limit")); + } + let raw_key = Raw::from(key.as_bytes()); let raw_value = sled::IVec::from(value);
116-118: Add input validation for key size in delete method.The key size should be validated to prevent potential DoS attacks.
let key = request.into_inner().key; + const MAX_KEY_SIZE: usize = 1024; // 1KB + if key.len() > MAX_KEY_SIZE { + ctx.span().add_event("Invalid argument", vec![KeyValue::new("error", "Key size exceeds limit")]); + return Err(tonic::Status::invalid_argument("Key size exceeds limit")); + } let raw_key = Raw::from(key.as_bytes());
Description
cherry pick meta API implementation from #2600 and opentelemetry migration from #2801
and some fixes
Related Issue
Versions
Checklist
Special notes for your reviewer
Summary by CodeRabbit
Dependency Updates
New Features
Metastruct for improved metadata handling.Improvements