-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[ENH] [DO NOT MERGE] Demo for #4660 #4665
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
base: cooper/chroma-metering
Are you sure you want to change the base?
Conversation
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
Please tag your PR title with one of: [ENH | BUG | DOC | TST | BLD | PERF | TYP | CLN | CHORE]. See https://docs.trychroma.com/contributing#contributing-code-and-ideas |
rust/metering/src/example.rs
Outdated
fn as_any_mut(&mut self) -> &mut dyn Any { | ||
self | ||
} | ||
fn as_any_owned(self: Box<Self>) -> Box<dyn Any> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not good
Add Example: Extensible Metering Capability System with Trait-Based Macros in Rust This PR introduces a comprehensive demonstration of a trait-based, macro-powered approach for extensible metering event capabilities in Rust. It includes new macro definitions for managing event capabilities, example trait and struct implementations, and an illustrative event stack mechanism. Documentation and commentary outline the design rationale, trade-offs, and usage patterns for adding new capabilities, emphasizing callsite ergonomics and extensibility. The changes are encapsulated in a new Key Changes: Affected Areas: Potential Impact: Functionality: Does not alter production metering logic, but introduces reusable patterns/macros for future capability extension. Intended as a demo-future integrations could leverage these macros to add new functionalities with reduced boilerplate. Performance: No direct runtime impact; dynamic capability checks may add minimal cost when/if this pattern is adopted in production code. Security: No security implications introduced. Scalability: Pattern supports scalable addition of new context capabilities and partial implementations; boilerplate remains manageable due to macro usage. Review Focus: Testing Needed• Run the included unit test ( Code Quality Assessmentrust/metering/src/example.rs: Well-commented and structured as a demonstration. Naming is intentionally generic/example-oriented, as noted by the author. rust/metering/src/macros.rs: Clear, Rust-idiomatic macros with documentation and multiple expansion cases covered. rust/metering/src/lib.rs: Straightforward integration; separation of example code is appropriate. rust/js_bindings/npm/darwin-arm64/package-lock.json: No issues; appears auto-generated/minor. Best PracticesModularity: Testing: Documentation: Potential Issues• Macro-generated error paths return default values or errors that could be easy to misuse (silent no-ops on missing capabilities). This risk is documented by the author. This summary was automatically generated by @propel-code-bot |
// SAFETY(hammadb): We know there is one event on the stack | ||
let peeked = event_stack.peek_event().unwrap(); | ||
|
||
peeked.add_request_start_time(std::time::Instant::now()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
example of what downstream callers who want to populate may do
rust/metering/src/example.rs
Outdated
|
||
peeked.add_request_start_time(std::time::Instant::now()); | ||
|
||
let finalized_event: Result<AEvent, String> = event_stack.finalize_event(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is an example of what you'd do at the "end of the request"
use super::*; | ||
|
||
#[test] | ||
fn test_example() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
example, we can make it a bit nicer facade
Review SummarySkipped posting 6 drafted comments based on your review threshold. Feel free to update them here. Draft Commentsrust/metering/src/example.rs:73-76
Scores:
Reason for filtering: Comment does not violate any strict rules Analysis: Undefined macros will cause compilation failures preventing deployment, but the fix direction is vague without knowing where the macro should be defined or imported from rust/metering/src/example.rs:84-88
Scores:
Reason for filtering: Comment identifies a critical compilation issue that prevents the code from building Analysis: This is a compilation-blocking issue where a required macro is undefined. Production impact is maximum since code won't compile. Fix specificity is moderate as the macro needs to be defined but implementation details aren't provided. Urgency is maximum as this prevents any deployment. rust/metering/src/example.rs:126-128
Scores:
Reason for filtering: Comment does not violate any strict rules and meets quality threshold Analysis: Undefined macro will cause compilation failure preventing deployment. Clear compilation error but fix requires defining the macro implementation. Critical issue that blocks system functionality completely. rust/metering/src/lib.rs:97-97
Scores:
Reason for filtering: This is a minor style improvement with no production impact, low urgency, and falls well below the required threshold of 13 Analysis: This comment addresses Rust field shorthand syntax which is purely stylistic with zero production impact or urgency. While the fix is very specific and correct, the total score of 7 is well below the required threshold of 13 for inclusion. rust/metering/src/macros.rs:60-68
Scores:
Reason for filtering: This is a logging/debugging improvement suggestion rather than a critical bug fix. The current error handling works correctly by providing default values when capability access fails. Adding error logging is a nice-to-have enhancement but doesn't fix any broken functionality. Analysis: Production impact is minimal since the code already handles errors correctly with fallback behavior. Fix is very specific with exact code provided. Urgency is very low as this is purely a debugging enhancement, not a functional fix. Total score of 7 is well below the 13 threshold. rust/metering/src/macros.rs:65-65
Scores:
Reason for filtering: Comment identifies a legitimate compilation error that will occur when macro is used with types not implementing Default trait Analysis: High production impact as this causes compilation failures, but the suggested fix is unclear and doesn't actually solve the constraint issue. The urgency is high since it breaks builds, but total score of 11 falls below threshold of 13. |
WalkthroughThis PR introduces foundational infrastructure for deterministic dependency management and extensible event capability handling in the Rust metering module and its JavaScript bindings. It adds platform-specific and general package-lock.json files for reproducible npm builds, and implements a trait-based, macro-driven framework for metering capabilities, including new macros and example usage. Module visibility and declarations are updated to support these features. Changes
Sequence DiagramThis diagram shows the interactions between components: sequenceDiagram
title Metering Context Capability System
participant Client
participant EventStack
participant MeteringContext as "dyn MeteringContext"
participant AMeteringContext
participant AddRequestStartTime as "AddRequestStartTime Trait"
participant LogCacheAccess as "LogCacheAccess Trait"
participant AsAny as "AsAny Trait"
Note over MeteringContext, AsAny: The system enables type-safe capabilities<br/>on metering contexts with ergonomic access
%% Creating and pushing an event
Client->>AMeteringContext: new("Initial data")
AMeteringContext-->>Client: AMeteringContext instance
Client->>Client: Box as dyn MeteringContext
Client->>EventStack: push_event(boxed_event)
%% Using capabilities through trait methods
Client->>EventStack: peek_event()
EventStack-->>Client: &mut Box<dyn MeteringContext>
%% Using capabilities through trait methods
Client->>MeteringContext: add_request_start_time(Instant::now())
activate MeteringContext
Note over MeteringContext: Capability method forwarding
MeteringContext->>MeteringContext: as_add_request_start_time()
MeteringContext->>AddRequestStartTime: add_request_start_time()
AddRequestStartTime->>AMeteringContext: add_request_start_time()
AMeteringContext->>AMeteringContext: self.request_start_time = Some(start_time)
deactivate MeteringContext
%% Calling an unsupported capability (no-op)
Client->>MeteringContext: cache_hit()
activate MeteringContext
Note over MeteringContext: Method exists but no implementation<br/>for AMeteringContext (no-op)
MeteringContext->>MeteringContext: as_log_cache_access()
MeteringContext-->>Client: (no-op, returns silently)
deactivate MeteringContext
%% Finalizing the event
Client->>EventStack: finalize_event::<AMeteringContext>()
activate EventStack
EventStack->>EventStack: events.pop()
EventStack->>AsAny: as_any_box()
AsAny->>AsAny: downcast::<AMeteringContext>()
alt Successful downcast
AsAny-->>EventStack: Ok(*target_event)
EventStack-->>Client: Ok(AMeteringContext)
else Type mismatch
AsAny-->>EventStack: Err
EventStack-->>Client: Err("Event type mismatch")
else No event
EventStack-->>Client: Err("No event to finalize")
end
deactivate EventStack
Note over Client, AsAny: The capability system allows strongly-typed<br/>access to capabilities without knowing<br/>concrete types at the call site
Note for Windsurf Entelligence.ai can learn from your feedback. Simply add 👍 / 👎 emojis to teach it your preferences. More shortcuts belowEmoji Descriptions:
Interact with the Bot:
Also you can trigger various commands with the bot by doing The current supported commands are
More commands to be added soon. |
@@ -0,0 +1,205 @@ | |||
use crate::{declare_event_capabilities, impl_dyn_event_forwarders, impl_event_capabilities}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correctness: The declare_event_capabilities!
, impl_dyn_event_forwarders!
, and impl_event_capabilities!
macros are used but not defined anywhere in the codebase. This will cause compilation failures when the code is built.
let event = | ||
Box::new(AMeteringContext::new("Initial data".to_string())) as Box<dyn MeteringContext>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correctness: The AMeteringContext
struct does not implement the MeteringContext
trait, but the code expects it to be usable as Box<dyn MeteringContext>
. This will cause a trait bound error during compilation.
// SAFETY(hammadb): We know there is one event on the stack | ||
let peeked = event_stack.peek_event().unwrap(); | ||
peeked.add_request_start_time(std::time::Instant::now()); | ||
peeked.cache_hit(); // This is not supported but we can call it and it should no-op |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correctness: The test calls cache_hit()
method on a MeteringContext
that doesn't implement LogCacheAccess
trait. This will result in a method not found compilation error since AMeteringContext
only implements AddRequestStartTime
.
demo for #4660