-
Notifications
You must be signed in to change notification settings - Fork 1.7k
[ENH]: add request timing to metering #4877
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-macros-integration
Are you sure you want to change the base?
[ENH]: add request timing to metering #4877
Conversation
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
Integrate New Metering Library and Add Request Execution Timing to Frontend This major PR removes the legacy metering event system from the Rust frontend and fully adopts the new Key Changes: Affected Areas: This summary was automatically generated by @propel-code-bot |
…ithub.com/chroma-core/chroma into cooper/request-latencies
…ithub.com/chroma-core/chroma into cooper/request-latencies
…ithub.com/chroma-core/chroma into cooper/request-latencies
…ithub.com/chroma-core/chroma into cooper/request-latencies
…ithub.com/chroma-core/chroma into cooper/request-latencies
…core/chroma into cooper/request-latencies
chroma_metering::with_current(|context| { | ||
context.latest_collection_logical_size_bytes(latest_collection_logical_size_bytes) | ||
context.latest_collection_logical_size_bytes(latest_collection_logical_size_bytes); | ||
context.request_completed_at(Utc::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.
given that the event is created in server.rs
, should we log the request_completed_at
there?
@@ -938,6 +950,12 @@ impl ServiceBasedFrontend { | |||
) -> Result<DeleteCollectionRecordsResponse, DeleteCollectionRecordsError> { | |||
let mut records = Vec::new(); | |||
|
|||
// NOTE(c-gamble): We are using a non-standard metering pattern in which we create a metering context within a |
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.
I think we only need to do latency tracking for
add()/upsert()/update()
get()/query()
Prefer we just do the minimal needed set for the product at hand
@@ -76,16 +84,25 @@ initialize_metering! { | |||
pub tenant: String, | |||
pub database: String, | |||
pub collection_id: String, | |||
pub request_received_at: DateTime<Utc>, |
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.
suggest we only do this for events we intend to display this for
@@ -254,6 +324,21 @@ initialize_metering! { | |||
.log_size_bytes | |||
.store(log_size_bytes, Ordering::SeqCst); | |||
} | |||
|
|||
/// Handler for [`crate::core::RequestCompletedAt`] capability for collection write contexts | |||
fn __handler_collection_write_request_completed_at( |
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.
Should we be using duration ?
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.
that can be derived from created_at
and completed_at
right
Instant
works as well
@@ -230,17 +290,27 @@ initialize_metering! { | |||
pub collection_id: String, | |||
#[serde(flatten)] | |||
pub action: WriteAction, | |||
pub request_received_at: DateTime<Utc>, |
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.
Would it make more sense to either
- have a start() mutator, that internally sets this, and a finish() mutator that ends it
- directly hydrate a
time_elapsed
capability ?
This PR uses the
chroma-metering
library introduced in #4868 to add execution timing to requests received on the frontend.pytest
for python,yarn test
for js,cargo test
for rustDocumentation Changes
N/A