Recreate PR #2600 and refresh branch #2801
Conversation
|
|
|
Important Review skippedMore than 25% of the files skipped due to max files limit. The review is being skipped to prevent a low-quality review. 74 files out of 260 files are above the max files limit of 75. Please upgrade to Pro plan to get higher limits. You can disable this status message by setting the 📝 WalkthroughWalkthroughThis pull request introduces comprehensive updates across multiple Rust packages, focusing on dependency version upgrades and observability enhancements. The changes span several components, including the meta service, agent, and observability libraries. Key modifications include updating dependencies like Changes
Sequence DiagramsequenceDiagram
participant Client as Test Client
participant MetaService as Metadata Service
participant Observability as Observability System
Client->>Observability: Inject Trace Context
Client->>MetaService: Set Key-Value
MetaService-->>Client: Confirmation
Client->>MetaService: Get Value
MetaService-->>Client: Return Value
Client->>MetaService: Delete Key
MetaService-->>Client: Deletion Confirmation
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 (
|
Deploying vald with
|
| Latest commit: |
64a8af7
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://bff5f090.vald.pages.dev |
| Branch Preview URL: | https://feature-meta-rebase-main.vald.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (8)
rust/bin/meta/src/main.rs (2)
28-44: ImplementCloneforMetadataMapfor Potential Future UseWhile the current implementation of
MetadataMapworks, consider implementingCloneorCopytraits forMetadataMapto enhance its usability, especially if you plan to pass instances across threads or tasks in the future.
46-51: Handle Possible Errors in the Interceptor FunctionThe
interceptfunction currently does not handle any potential errors that might occur during the extraction of the parent context. Although unlikely, it's good practice to handle or log unexpected situations to aid in debugging.Consider adding error handling or logging within the
interceptfunction:fn intercept(mut req: Request<()>) -> Result<Request<()>, tonic::Status> { let parent_cx = - global::get_text_map_propagator(|prop| prop.extract(&MetadataMap(req.metadata()))); + global::get_text_map_propagator(|prop| prop.extract(&MetadataMap(req.metadata()))).clone(); req.extensions_mut().insert(parent_cx); Ok(req) }rust/bin/meta/src/handler/meta.rs (2)
65-91: Simplify NestedmatchStatements and Improve Error MessagesThe
setmethod has deeply nestedmatchstatements, which can be simplified for better readability. Additionally, the error messages could be more consistent.Refactor the code to flatten the
matchstatements:let key_value = request.into_inner(); -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")); - } -}; +if key_value.key.is_none() || key_value.value.is_none() { + ctx.span().add_event("Invalid argument", vec![KeyValue::new("error", "Key or Value is missing")]); + return Err(tonic::Status::invalid_argument("Key or Value is missing")); +} + +let key = key_value.key.unwrap().key; +let value = if let Some(any_value) = key_value.value.unwrap().value { + any_value.value +} else { + ctx.span().add_event("Invalid argument", vec![KeyValue::new("error", "Value is missing")]); + return Err(tonic::Status::invalid_argument("Value is missing")); +};
119-128: Handle Deletion of Non-Existent Keys GracefullyWhen deleting a key that does not exist, the method may return an error or succeed silently. Consider defining the expected behavior and handling it accordingly.
Perhaps inform the client if the key did not exist:
match self.bucket.remove(&raw_key) { Ok(Some(_)) => { ctx.span().add_event("Key deleted successfully", vec![KeyValue::new("key", key)]); Ok(tonic::Response::new(Empty {})) }, + Ok(None) => { + ctx.span().add_event("Key not found", vec![KeyValue::new("key", key)]); + Err(tonic::Status::not_found("Key not found")) + }, 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))) } }rust/bin/meta/src/handler.rs (1)
29-30: Close the Store Properly on ShutdownEnsure that the
Storeis properly closed when the application shuts down to prevent data corruption.Implement the
Droptrait or provide a shutdown method forMeta:impl Drop for Meta { fn drop(&mut self) { // Close the store if let Err(e) = self.store.close() { eprintln!("Failed to close store: {}", e); } } }rust/bin/meta/src/test_client.rs (2)
48-48: Consider configurable endpoint.The endpoint is hardcoded to "http://[::1]:8081". Consider making it configurable through environment variables or command-line arguments.
+use std::env; + #[tokio::main] async fn main() -> Result<(), Box<dyn std::error::Error>> { - let mut client = MetaClient::connect("http://[::1]:8081").await?; + let endpoint = env::var("META_SERVICE_ENDPOINT") + .unwrap_or_else(|_| "http://[::1]:8081".to_string()); + let mut client = MetaClient::connect(endpoint).await?;
130-134: Improve error handling in get operation.The error handling for the get operation after deletion only prints the error. Consider implementing proper error handling or recovery logic.
- match response { - Ok(res) => println!("Get key: aaa, RESPONSE={:?}", res.into_inner()), - Err(e) => println!("Get key: aaa failed with error: {:?}", e), - } + match response { + Ok(res) => println!("Get key: aaa, RESPONSE={:?}", res.into_inner()), + Err(e) => { + if e.code() == tonic::Code::NotFound { + println!("Key 'aaa' was successfully deleted and is not found"); + } else { + eprintln!("Unexpected error while getting key 'aaa': {:?}", e); + } + } + }rust/bin/meta/Cargo.toml (1)
32-35: Consider adding integration tests for the test client.While the test client is marked as
doc = false, it would be beneficial to have integration tests to ensure it works correctly with the metadata service.Consider adding a
testsdirectory with integration tests for the test client functionality.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
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)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: check-format-diff
- GitHub Check: runner / textlint
- GitHub Check: Cloudflare Pages
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
rust/bin/meta/src/main.rs (1)
96-96: Ensure Proper Shutdown of Observability SystemThe
observability.shutdown()?call might return an error that is currently being ignored. To ensure proper resource cleanup, handle any potential errors during shutdown.Check if the shutdown method can fail and handle the error accordingly:
observability.shutdown()?; +println!("Observability system shut down successfully.");Or handle the error:
-if let Err(e) = observability.shutdown() { - eprintln!("Failed to shut down observability system: {}", e); -}rust/bin/meta/src/handler/meta.rs (1)
79-91: ValidateAnyMessage TypesWhen handling
prost_types::Any, it's important to validate thetype_urlto ensure that the received message is of the expected type.Add validation for the
type_url:if any_value.type_url != "type.googleapis.com/expected.MessageType" { ctx.span().add_event("Invalid message type", vec![KeyValue::new("type_url", any_value.type_url)]); return Err(tonic::Status::invalid_argument("Invalid message type")); }rust/bin/meta/src/handler.rs (2)
28-32: Ensure Thread Safety and Consider Using a Singleton for the StoreThe
storeis wrapped in anArc, which is good for sharing across threads. Ensure that theStoreandBuckettypes are thread-safe if they will be accessed concurrently.Check the documentation for the
kvcrate to confirm thatStoreandBucketare safe to share between threads. If they are not, consider using synchronization primitives or limiting access to a single thread.
30-30:⚠️ Potential issueHandle Possible Errors When Creating the Bucket
When creating the bucket with
store.bucket, consider handling the possibility that the bucket already exists or cannot be created.Add error handling or check for existing buckets:
let bucket = store.bucket::<Raw, Raw>(Some("meta_bucket"))?; +// Alternatively, check if the bucket exists and handle accordinglyLikely invalid or redundant comment.
rust/bin/meta/Cargo.toml (1)
22-24: Verify dependency versions for security and compatibility.Several new dependencies have been added. Please verify:
- Check for any known vulnerabilities in these versions
- Ensure compatibility with the rest of the ecosystem
Also applies to: 26-26
✅ Verification successful
Dependencies are consistently versioned across the codebase
The new dependencies are properly aligned:
prost-types 0.13.2is consistently used across all Rust componentskv 0.24.0andopentelemetry 0.23.0are cleanly introduced only in the meta componentNote: The API documentation mentions an older version of
prost-typesand might need updating, but this is not a blocking issue.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for known vulnerabilities in dependencies cargo audit # List all dependency versions for verification cargo treeLength of output: 218
Script:
#!/bin/bash # Find all Cargo.toml files echo "=== Cargo.toml files ===" fd Cargo.toml # Search for kv dependency versions echo -e "\n=== kv dependency versions ===" rg "kv\s*=\s*\"[^\"]*\"" -A 1 -B 1 # Search for opentelemetry versions echo -e "\n=== opentelemetry versions ===" rg "opentelemetry\s*=\s*\"[^\"]*\"" -A 1 -B 1 # Search for prost-types versions echo -e "\n=== prost-types versions ===" rg "prost-types\s*=\s*\"[^\"]*\"" -A 1 -B 1Length of output: 1594
kmrmt
changed the title
* fix go build argument * update go
This commit fixes the style issues introduced in 8179e7d according to the output from Gofumpt and Prettier. Details: None Co-authored-by: deepsource-autofix[bot] <62050782+deepsource-autofix[bot]@users.noreply.github.com> Co-authored-by: Kiichiro YUKAWA <[email protected]>
* fix validation logic for manager indexer Signed-off-by: aknishid <[email protected]> * Change validation to include zero. Signed-off-by: aknishid <[email protected]> --------- Signed-off-by: aknishid <[email protected]> Co-authored-by: Kiichiro YUKAWA <[email protected]>
Signed-off-by: Vdaas CI <[email protected]> Co-authored-by: Kiichiro YUKAWA <[email protected]>
* add Search_Config validation * change to error function and generate individual errors * add search configuration
Co-authored-by: Kiichiro YUKAWA <[email protected]> Signed-off-by: Kosuke Morimoto <[email protected]>
Co-authored-by: Kiichiro YUKAWA <[email protected]> Signed-off-by: Kosuke Morimoto <[email protected]>
Co-authored-by: Kiichiro YUKAWA <[email protected]> Signed-off-by: Kosuke Morimoto <[email protected]>
Co-authored-by: Kiichiro YUKAWA <[email protected]> Signed-off-by: Kosuke Morimoto <[email protected]>
kmrmt
dismissed stale reviews from kpango, vankichi, and coderabbitai[bot]
via
b4ca563
kmrmt
force-pushed
the
feature/meta/rebase-main
branch
from
9023068 to
b4ca563
Compare
Description
Recreate #2600 and refresh branch.
Related Issue
Versions
Checklist
Special notes for your reviewer
Summary by CodeRabbit
Dependencies
Toolchain
New Features
Refactoring