-
Notifications
You must be signed in to change notification settings - Fork 269
More improvements to studio2 publishing #2333
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
Conversation
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Review Summary |
LGTM 👍 |
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.
Caution
Changes requested ❌
Reviewed everything up to 4a84bd2 in 1 minute and 19 seconds. Click for details.
- Reviewed
158
lines of code in4
files - Skipped
0
files when reviewing. - Skipped posting
4
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. engine/baml-rpc/src/ui/mod.rs:5
- Draft comment:
New module 'ui_dashboard_cost' added. Consider keeping module ordering consistent with similar modules for clarity. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
2. engine/baml-rpc/src/ui/ui_dashboard.rs:188
- Draft comment:
GetDashboardDataResponse now includes new metric fields. If these values might sometimes be missing, consider making them optional or ensuring backend always provides them. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
3. engine/baml-rpc/src/ui/ui_dashboard_cost.rs:9
- Draft comment:
The cost stats endpoint defines its own TimeSeriesFloatPoint and MetricSeriesFloat types. Consider reusing shared types to avoid duplication across dashboard endpoints. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% While code deduplication is generally good, we don't have strong evidence that duplicate types exist elsewhere. The comment requires knowledge of other files to verify. Following our rules, we should err on the side of deletion when we need more context. Additionally, these types might have subtle differences from other similar types that justify their separate existence. The comment could be valid if there really are identical types defined elsewhere. Type duplication is a legitimate concern for maintainability. Without seeing the other files to confirm the duplication, we can't be certain this comment is correct or actionable. We need strong evidence to keep comments. Delete the comment since we lack strong evidence of type duplication and would need to look at other files to verify the claim.
4. engine/baml-rpc/src/ui/ui_types.rs:157
- Draft comment:
Cost estimate fields (input_cost, output_cost, total_cost) have been added to UiUsageEstimate. Ensure these are computed and populated consistently across providers. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
Workflow ID: wflow_P9sAxFivRUEl1DE2
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
@@ -151,13 +151,48 @@ pub struct StatusCountOverTime { | |||
pub error_count: u64, | |||
} | |||
|
|||
#[derive(Debug, Serialize, Deserialize, TS)] |
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.
New time series and metric types (TimeSeriesFloatPoint, TimeSeriesIntPoint, MetricSeriesFloat, MetricSeriesInt) are defined here. These are duplicated in ui_dashboard_cost. Consider extracting shared types to a common module to reduce code duplication.
🌿 Preview your docs: https://boundary-preview-6383cdca-7d06-41d5-9682-b4a7e97f216c.docs.buildwithfern.com |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
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.
Important
Looks good to me! 👍
Reviewed 864ef02 in 1 minute and 15 seconds. Click for details.
- Reviewed
121
lines of code in6
files - Skipped
0
files when reviewing. - Skipped posting
9
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. engine/baml-runtime/src/tracingv2/publisher/rpc_converters/trace_data.rs:34
- Draft comment:
Ensure the recursive call to self.to_rpc_event(lookup) here correctly resolves to the impl returning Option (for baml_function_content) without ambiguity. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
2. engine/baml-runtime/src/tracingv2/publisher/rpc_converters/trace_data.rs:60
- Draft comment:
Verify that using lookup.function_lookup(&self.name) with a fallback to 'unknown_hash' is the desired behavior when function_type is BamlLlm. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
3. engine/baml-runtime/src/tracingv2/publisher/rpc_converters/trace_data.rs:124
- Draft comment:
In the LoggedLLMRequest conversion, params are mapped using Cow::Borrowed. Ensure that the lifetimes of these borrowed values remain valid for the RPC event. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
4. engine/baml-runtime/src/tracingv2/publisher/rpc_converters/trace_data.rs:156
- Draft comment:
The conversion for LLMChatMessagePart handles Text, Media, and WithMeta variants; double-check that boxing and cloning are optimal and that no unnecessary allocations occur. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
5. engine/baml-runtime/src/tracingv2/publisher/rpc_converters/trace_data.rs:195
- Draft comment:
Using Cow::Borrowed in the HTTPBody conversion is efficient; ensure that self.raw() remains valid for the RPC lifetime. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
6. engine/baml-runtime/src/tracingv2/publisher/rpc_converters/trace_data.rs:211
- Draft comment:
In the HTTPRequest conversion, headers are passed through redact_headers after cloning. Confirm that this redaction meets security requirements. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
7. engine/baml-runtime/src/tracingv2/publisher/rpc_converters/trace_data.rs:237
- Draft comment:
For HTTPResponse conversion, ensure that optional headers are handled correctly when cloning and applying redact_headers. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
8. engine/baml-runtime/src/tracingv2/publisher/rpc_converters/trace_data.rs:259
- Draft comment:
In the HTTPResponseStream conversion, using Cow::Borrowed(&self.event.data) should be reviewed to ensure the event data remains valid during RPC processing. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
9. engine/baml-runtime/src/tracingv2/publisher/rpc_converters/trace_data.rs:267
- Draft comment:
The LoggedLLMResponse conversion maps optional raw_text_output to a Cow; verify that this conversion correctly represents absence or presence of text without data loss. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
Workflow ID: wflow_9PwQJy5vd7yGrkDP
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
🌿 Preview your docs: https://boundary-preview-5dacf81b-482e-46af-adbd-07c79163c90e.docs.buildwithfern.com |
🌿 Preview your docs: https://boundary-preview-822eaa11-481d-496d-8b18-125a2d69779f.docs.buildwithfern.com |
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.
Important
Looks good to me! 👍
Reviewed ba1a717 in 2 minutes and 4 seconds. Click for details.
- Reviewed
37
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
2
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. engine/baml-rpc/src/auth/api_key.rs:42
- Draft comment:
Fallback slice length changed from 3 to 4. Confirm this is intentional and that it maintains consistency with the predefined codes ('dev', 'stg', 'prod'). Also, consider Unicode safety. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The comment has multiple issues: 1) It asks for confirmation of intention which violates our rules 2) The consistency concern is not valid since the predefined codes are hardcoded separately 3) While Unicode safety could be a real issue, the comment doesn't provide evidence that it's actually problematic in this context 4) The change from 3 to 4 chars seems like a minor change that doesn't warrant discussion. The Unicode safety concern could be legitimate since string slicing in Rust can panic on invalid UTF-8 boundaries. While Unicode safety is a valid concern in general, this code is working with a String type which guarantees valid UTF-8, and len().min(4) ensures we won't slice beyond string bounds. Delete this comment as it primarily asks for confirmation of intention and raises concerns without strong evidence of actual problems.
2. engine/baml-runtime/src/tracingv2/publisher/rpc_converters/mod.rs:618
- Draft comment:
Added 'client_details' in the RawLLMRequest test. Ensure this test data reflects the production API requirements and remains updated if the RPC schema changes. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the PR author to ensure that the test data reflects production API requirements and remains updated if the RPC schema changes. This falls under asking the author to ensure something is tested or verified, which is against the rules.
Workflow ID: wflow_YtlHc4jHPn0iFqoh
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
🌿 Preview your docs: https://boundary-preview-d1c7be64-073e-4ff2-bc99-abcb885eed86.docs.buildwithfern.com |
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.
Important
Looks good to me! 👍
Reviewed 869e45e in 1 minute and 19 seconds. Click for details.
- Reviewed
36
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
2
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. engine/baml-rpc/src/ui/ui_function_call_http_calls.rs:29
- Draft comment:
Add a newline at end of file for POSIX compliance. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is purely informative and does not provide a specific code suggestion or ask for a specific test. It simply states a general best practice without context.
2. engine/baml-rpc/src/ui/ui_function_call_http_calls.rs:7
- Draft comment:
Consider adding Rust doc comments (///) for public structs (e.g., GetFunctionCallHttpCallsRequest and GetFunctionCallHttpCallsResponse) to improve maintainability. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
Workflow ID: wflow_oTtcQbAdA3dqXuC4
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
🌿 Preview your docs: https://boundary-preview-a804f65d-d5f0-416b-8cd8-6b29f26401c7.docs.buildwithfern.com |
🌿 Preview your docs: https://boundary-preview-cb2bb46a-f9b7-44d8-b692-618dab1df67a.docs.buildwithfern.com |
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.
Important
Looks good to me! 👍
Reviewed df0ed86 in 1 minute and 34 seconds. Click for details.
- Reviewed
70
lines of code in4
files - Skipped
0
files when reviewing. - Skipped posting
4
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. engine/baml-rpc/src/lib.rs:30
- Draft comment:
Reordering of public exports: The ui_function_call_http_calls export has been moved before ui_function_calls. This reordering is non-functional but check that it aligns with the project's conventions to avoid any confusion in consumers. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
2. engine/baml-rpc/src/ui/mod.rs:6
- Draft comment:
Module declaration reordering: The ui_function_calls module is now declared after ui_function_call_http_calls. Although this does not change behavior, ensuring a consistent ordering (e.g. alphabetical or logical grouping) helps maintain clarity. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
3. engine/baml-rpc/src/ui/ui_dashboard_cost.rs:12
- Draft comment:
Formatting update for struct fields: Multi-line formatting for fields in GetCostStatsRequest improves readability. Please ensure this style is consistent with other similar definitions in the project. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
4. engine/baml-rpc/src/ui/ui_function_call_http_calls.rs:29
- Draft comment:
Trailing newline added at EOF: This small formatting fix ensures consistency with standard style guidelines. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
Workflow ID: wflow_rOte96drImEHgJ9p
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
🌿 Preview your docs: https://boundary-preview-e410932e-99bb-4d36-a10d-548a36d37633.docs.buildwithfern.com |
Important
Enhance client details handling in HTTP requests, add new metrics and cost structures, and update protobuf message handling.
client_details
toHTTPRequest
inevents.rs
andIntermediateData
intrace_event.rs
.ApiKeyEnvironment::environment()
inapi_key.rs
to handle longer environment strings.ui_dashboard.rs
andui_dashboard_cost.rs
.cffi.pb.go
to includeprotoimpl.MessageState
in various structs for better message handling.GetFunctionCallHttpCalls
endpoint inui_function_call_http_calls.rs
.llm_only
filter toListTracesRequest
inui_traces.rs
.UiUsageEstimate
inui_types.rs
.This description was created by
for df0ed86. You can customize this summary. It will automatically update as commits are pushed.