Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/meta/save metadata #2624

Merged
merged 3 commits into from
Sep 13, 2024
Merged

Feature/meta/save metadata #2624

merged 3 commits into from
Sep 13, 2024

Conversation

kpango
Copy link
Collaborator

@kpango kpango commented Sep 13, 2024

Description

Related Issue

Versions

  • Vald Version: v1.7.13
  • Go Version: v1.23.1
  • Rust Version: v1.81.0
  • Docker Version: v27.2.1
  • Kubernetes Version: v1.31.0
  • Helm Version: v3.16.0
  • NGT Version: v2.2.4
  • Faiss Version: v1.8.0

Checklist

Special notes for your reviewer

Summary by CodeRabbit

  • New Features

    • Introduced a metadata management API with Get, Set, and Delete methods for handling key-value pairs.
    • Enhanced API documentation for better clarity on metadata interactions.
    • Added Swagger API specification for RESTful metadata service.
  • Bug Fixes

    • Improved error handling in API responses.
  • Documentation

    • Expanded documentation to include new metadata structures and methods.
  • Chores

    • Updated project dependencies to support new metadata functionalities.

Copy link
Contributor

coderabbitai bot commented Sep 13, 2024

Walkthrough

Walkthrough

The pull request introduces a new metadata management feature to the API, encompassing the addition of a Meta service with methods for retrieving, setting, and deleting metadata. This includes new message structures in the Protocol Buffers definition and corresponding updates to the API documentation and Rust implementation. The changes enhance the API's functionality by providing a structured approach to metadata handling.

Changes

Files Change Summary
apis/docs/v1/docs.md Added documentation for the new Meta functionality, detailing its components and methods.
apis/proto/v1/meta/meta.proto Introduced a Meta service with Get, Set, and Delete RPC methods for metadata management.
apis/proto/v1/payload/payload.proto Added Meta, Key, Value, and KeyValue message structures for metadata handling.
apis/swagger/v1/meta/meta.swagger.json Defined RESTful endpoints for Meta service operations and included relevant data structures.
rust/Cargo.toml Added a new binary target for the meta functionality in the Rust project.
rust/bin/meta/Cargo.toml Configured a new Rust package named "meta" with necessary dependencies.
rust/bin/meta/src/handler.rs Introduced a Meta struct as a placeholder for future development.
rust/bin/meta/src/handler/meta.rs Implemented Meta trait methods for gRPC service, currently as placeholders.
rust/bin/meta/src/main.rs Established an entry point for the server application, configuring it to listen for incoming requests.
rust/libs/proto/Cargo.toml Added prost-types dependency for enhanced protocol buffer functionality.
rust/libs/proto/src/lib.rs Introduced a meta module to organize metadata-related functionalities.
rust/libs/proto/src/meta.v1.tonic.rs Defined a gRPC client and server for the Meta service, implementing request and response handling.
rust/libs/proto/src/payload.v1.rs Added Key, Value, and KeyValue structures for metadata representation.

Possibly related PRs

Suggested labels

priority/medium, size/XXXL


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?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@kpango kpango force-pushed the feature/meta/save-metadata branch from 13916e8 to f2e604f Compare September 13, 2024 01:33
Copy link

cloudflare-workers-and-pages bot commented Sep 13, 2024

Deploying vald with  Cloudflare Pages  Cloudflare Pages

Latest commit: b7d005a
Status: ✅  Deploy successful!
Preview URL: https://3fdfe8d3.vald.pages.dev
Branch Preview URL: https://feature-meta-save-metadata.vald.pages.dev

View logs

@smorihira smorihira force-pushed the feature/meta/save-metadata branch from f2e604f to f43c33a Compare September 13, 2024 01:40
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Outside diff range and nitpick comments (1)
rust/libs/proto/src/payload.v1.rs (1)

1241-1244: LGTM, but consider adding a doc comment to clarify the purpose.

The empty Meta struct has been introduced correctly. However, its purpose is unclear. Consider adding a documentation comment to explain its intended usage, even if it's a placeholder for now.

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between b63d3de and 269d19a.

Files ignored due to path filters (8)
  • apis/grpc/v1/agent/sidecar/sidecar_vtproto.pb.go is excluded by !**/*.pb.go, !**/*.pb.go
  • apis/grpc/v1/meta/meta.pb.go is excluded by !**/*.pb.go, !**/*.pb.go
  • apis/grpc/v1/meta/meta_vtproto.pb.go is excluded by !**/*.pb.go, !**/*.pb.go
  • apis/grpc/v1/payload/payload.pb.go is excluded by !**/*.pb.go, !**/*.pb.go
  • apis/grpc/v1/payload/payload.pb.json.go is excluded by !**/*.pb.json.go
  • apis/grpc/v1/payload/payload_vtproto.pb.go is excluded by !**/*.pb.go, !**/*.pb.go
  • apis/grpc/v1/rpc/errdetails/error_details.pb.go is excluded by !**/*.pb.go, !**/*.pb.go
  • rust/Cargo.lock is excluded by !**/*.lock
Files selected for processing (13)
  • apis/docs/v1/docs.md (4 hunks)
  • apis/proto/v1/meta/meta.proto (1 hunks)
  • apis/proto/v1/payload/payload.proto (2 hunks)
  • apis/swagger/v1/meta/meta.swagger.json (1 hunks)
  • rust/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/libs/proto/Cargo.toml (1 hunks)
  • rust/libs/proto/src/lib.rs (1 hunks)
  • rust/libs/proto/src/meta.v1.tonic.rs (1 hunks)
  • rust/libs/proto/src/payload.v1.rs (1 hunks)
Files skipped from review due to trivial changes (1)
  • rust/bin/meta/src/handler.rs
Additional context used
buf
apis/proto/v1/meta/meta.proto

21-21: import "google/api/annotations.proto": file does not exist

(COMPILE)

checkov
apis/swagger/v1/meta/meta.swagger.json

[HIGH] 1-167: Ensure that security requirement defined in securityDefinitions - version 2.0 files

(CKV_OPENAPI_6)


[HIGH] 1-167: Ensure that the global security field has rules defined

(CKV_OPENAPI_4)


[HIGH] 1-167: Ensure that security operations is not empty.

(CKV_OPENAPI_5)


[HIGH] 1-167: Ensure that securityDefinitions is defined and not empty - version 2.0 files

(CKV_OPENAPI_1)

Markdownlint
apis/docs/v1/docs.md

688-688: null
Link fragments should be valid

(MD051, link-fragments)

Additional comments not posted (26)
rust/Cargo.toml (1)

20-20: LGTM!

The addition of the "bin/meta" member to the [workspace] section is correct and consistent with the PR objective of introducing metadata functionality.

rust/libs/proto/Cargo.toml (1)

26-26: LGTM!

The addition of the prost-types dependency with version 0.13.2 is consistent with the existing prost and tonic dependencies. It aligns with the purpose of enhancing protocol buffer functionality in the project.

rust/bin/meta/Cargo.toml (3)

1-15: License header looks good.

The file includes the standard Apache License 2.0 header with the appropriate copyright notice. The formatting and content of the license header are correct.


16-19: Package configuration is appropriate.

The package configuration specifies the correct name, version, and edition for the meta binary target. The package name aligns with the purpose of the binary, the version indicates an initial release, and the edition is set to the latest stable version of Rust.


21-24: Dependencies are properly specified.

The dependencies required by the meta binary target are appropriately listed:

  • The proto dependency is sourced from a local path, suggesting a custom library within the project.
  • The tokio dependency is included with the "full" feature set, enabling asynchronous runtime capabilities.
  • The tonic dependency is specified with a specific version, ensuring compatibility with the gRPC framework.

The dependencies are relevant and necessary for the functionality of the meta binary target.

rust/libs/proto/src/lib.rs (1)

40-44: LGTM!

The addition of the meta module and its submodule v1 follows the existing pattern in the library and is consistent with the library's structure and naming conventions. The inclusion of the meta.v1.tonic.rs file suggests that the meta module will contain metadata-related functionality, likely generated using the Tonic framework for gRPC.

The changes enhance the library's structure by organizing related functionalities under the new meta namespace, which may facilitate better code management and modularization.

rust/bin/meta/src/main.rs (1)

1-30: LGTM!

The code in main.rs looks good overall:

  • It follows Rust conventions and best practices.
  • It uses tokio for async execution and returns a Result from main for error handling.
  • It sets up a gRPC server using tonic, which is a good choice.
  • It creates an instance of the Meta handler using its default method, registers it as a service, and starts the server correctly.

No issues found. Good job!

rust/bin/meta/src/handler/meta.rs (3)

21-26: LGTM! Remember to implement the method.

The method signature is correct and matches the gRPC service definition. Please remember to implement the method before merging this PR.


27-32: LGTM! Remember to implement the method.

The method signature is correct and matches the gRPC service definition. Please remember to implement the method before merging this PR.


34-39: LGTM! Remember to implement the method.

The method signature is correct and matches the gRPC service definition. Please remember to implement the method before merging this PR.

apis/proto/v1/meta/meta.proto (2)

30-45: LGTM!

The Meta service and its methods (Get, Set, and Delete) are correctly defined using the Protocol Buffers syntax. The service definition follows the standard conventions and includes the appropriate HTTP annotations for each method.


22-22: Verify the relative import path for v1/payload/payload.proto.

The import statement assumes a relative path for v1/payload/payload.proto. Please ensure that this file exists at the specified location relative to the current file.

Run the following script to verify the import path:

Verification successful

Import path is correct. The file apis/proto/v1/payload/payload.proto exists at the specified location, confirming that the import path in apis/proto/v1/meta/meta.proto is correct.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the existence of the imported file.

# Test: Check if the file exists relative to the current file.
if [ -f "apis/proto/v1/payload/payload.proto" ]; then
  echo "Import path is correct. File exists at apis/proto/v1/payload/payload.proto"
else
  echo "Import path is incorrect. File does not exist at apis/proto/v1/payload/payload.proto"
fi

Length of output: 207

apis/swagger/v1/meta/meta.swagger.json (3)

1-13: LGTM!

The API info and tags are properly defined.

Tools
checkov

[HIGH] 1-167: Ensure that security requirement defined in securityDefinitions - version 2.0 files

(CKV_OPENAPI_6)


[HIGH] 1-167: Ensure that the global security field has rules defined

(CKV_OPENAPI_4)


[HIGH] 1-167: Ensure that security operations is not empty.

(CKV_OPENAPI_5)


[HIGH] 1-167: Ensure that securityDefinitions is defined and not empty - version 2.0 files

(CKV_OPENAPI_1)


14-99: LGTM!

The API paths and operations are well-structured and consistent. The request/response schemas are properly referenced.


100-165: LGTM!

The API models are accurately defined with proper types, references and detailed descriptions.

rust/libs/proto/src/meta.v1.tonic.rs (3)

16-171: LGTM!

The meta_client module provides a well-structured and idiomatic Rust implementation of a gRPC client for the Meta service. The code follows best practices and effectively utilizes the tonic library for gRPC functionality.


173-456: LGTM!

The meta_server module provides a well-structured and idiomatic Rust implementation of a gRPC server for the Meta service. The code follows best practices, such as defining a trait for the server methods, implementing necessary traits from the tonic library, and handling incoming requests effectively using async/await.


1-456: Excellent work!

The rust/libs/proto/src/meta.v1.tonic.rs file provides a well-structured, idiomatic, and efficient implementation of a gRPC client and server for the Meta service using the tonic library. The code follows Rust best practices and effectively utilizes async/await for asynchronous programming. Great job!

apis/proto/v1/payload/payload.proto (1)

780-793: LGTM! The new Meta message structure enhances flexibility and extensibility.

The introduction of the Meta message structure, along with its nested Key, Value, and KeyValue messages, provides a powerful mechanism for storing and associating diverse data types with specific keys. This addition expands the capabilities of the payload without affecting existing functionality.

Key highlights:

  • The Key message allows for clear identification of each metadata entry.
  • The Value message, utilizing the google.protobuf.Any type, enables the storage of any arbitrary message type, promoting extensibility.
  • The KeyValue message establishes a strong relationship between a key and its corresponding value, ensuring clarity and ease of use.

These enhancements offer great flexibility and adaptability for future metadata requirements.

rust/libs/proto/src/payload.v1.rs (3)

1247-1252: Looks good!

The Key struct with the key field of type String has been defined correctly.


1253-1258: Looks good!

The Value struct with the optional value field of type Any has been defined correctly. Using Any allows flexibility in the types of values that can be stored.


1259-1266: Looks good!

The KeyValue struct correctly composes the Key and Value structs to represent a key-value pair. Making both fields optional allows for flexibility in usage.

apis/docs/v1/docs.md (4)

53-56: LGTM!

The addition of the Meta entry to the table of contents is consistent with the introduction of the new Meta section in the documentation.


661-689: Documentation for the new Meta message type looks good!

The Meta message type and its fields (Meta.Key, Meta.KeyValue, and Meta.Value) are clearly documented, following the existing documentation structure.

Note that the Meta.Value field uses the google.protobuf.Any type, which allows for storing arbitrary message types. While this provides flexibility, it may require additional handling in the code to pack and unpack the stored value.

Tools
Markdownlint

688-688: null
Link fragments should be valid

(MD051, link-fragments)


1346-1361: LGTM!

The documentation for the new Meta service and its methods (Get, Set, and Delete) is clear, concise, and consistent with the existing documentation structure.


688-688: The static analysis hint about the link fragment is a false positive and can be safely ignored, as the link fragment is valid and points to the correct location in the documentation.

Tools
Markdownlint

688-688: null
Link fragments should be valid

(MD051, link-fragments)

apis/proto/v1/meta/meta.proto Show resolved Hide resolved
apis/swagger/v1/meta/meta.swagger.json Show resolved Hide resolved
Copy link
Collaborator

@hlts2 hlts2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@kpango kpango merged commit 1c45978 into main Sep 13, 2024
173 of 175 checks passed
@kpango kpango deleted the feature/meta/save-metadata branch September 13, 2024 04:04
@coderabbitai coderabbitai bot mentioned this pull request Oct 10, 2024
2 tasks
@coderabbitai coderabbitai bot mentioned this pull request Nov 14, 2024
2 tasks
@coderabbitai coderabbitai bot mentioned this pull request Nov 19, 2024
2 tasks
takuyaymd pushed a commit to takuyaymd/vald that referenced this pull request Dec 2, 2024
takuyaymd pushed a commit to takuyaymd/vald that referenced this pull request Dec 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants