feat: SearchEventContext for search usage reports #4934
Conversation
WalkthroughThe pull request introduces several enhancements across multiple files, focusing on the integration of a new Changes
Possibly related PRs
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (8)
src/cli/data/export.rs (1)
40-42: Consider using?operator for error propagationThe query map creation looks good, but consider enhancing error handling in the subsequent operations using the
?operator for more concise error propagation.- let stream_type = match get_stream_type_from_request(&query_map) { - Ok(v) => v.unwrap_or(StreamType::Logs), - Err(_) => return Ok(false), - }; + let stream_type = get_stream_type_from_request(&query_map) + .map_err(|_| false)? + .unwrap_or(StreamType::Logs);src/service/usage/stats.rs (2)
96-96: Consider documenting the intentional None value.The addition of
search_event_contextaligns with the PR objectives. Consider adding a brief comment explaining why it's set toNonein this internal usage statistics context.+ // Search event context is not needed for internal usage statistics search_event_context: None,
155-155: Consider documenting the intentional None value.Similar to the previous instance, consider documenting why
search_event_contextis set toNonehere.+ // Search event context is not needed for internal usage statistics search_event_context: None,src/common/utils/http.rs (2)
85-103: Add documentation for the new function.The function lacks documentation explaining its purpose, parameters, and return value. Consider adding a doc comment.
#[inline(always)] +/// Extracts search event context from HTTP query parameters based on the search event type. +/// +/// # Arguments +/// * `search_event_type` - Type of search event (Dashboards, Alerts, Reports) +/// * `query` - HTTP query parameters +/// +/// # Returns +/// * `Some(SearchEventContext)` if relevant parameters are found +/// * `None` if parameters are missing or search type is not supported pub(crate) fn get_search_event_context_from_request(
91-94: Optimize string allocations.Consider using references where possible to avoid unnecessary string allocations.
- let dashboard_folder = query.get("dashboard_folder").map(String::from); - SearchEventContext::with_dashboard(dashboard_name.to_owned(), dashboard_folder) + let dashboard_folder = query.get("dashboard_folder").map(|s| s.to_string()); + SearchEventContext::with_dashboard(dashboard_name.to_string(), dashboard_folder)src/handler/http/request/traces/mod.rs (1)
Line range hint
258-260: Add timeout validationThe timeout parameter is parsed without validation. Consider adding bounds checking to prevent unreasonable values.
Add validation for the timeout value:
let timeout = query .get("timeout") .map_or(0, |v| v.parse::<i64>().unwrap_or(0)); + if timeout < 0 || timeout > cfg.common.max_query_timeout { + return Ok(MetaHttpResponse::bad_request(&format!( + "timeout must be between 0 and {} seconds", + cfg.common.max_query_timeout + ))); + }src/service/search/mod.rs (1)
465-482: Consider adding a documentation commentWhile the match expression is well-structured, consider adding a brief comment explaining why certain search types (UI, Values, Other) don't require usage reporting. This would help future maintainers understand the business logic.
+ // Skip usage reporting for UI, Values, and Other search types as they are considered + // internal operations. All other search types should be tracked for usage statistics. let (report_usage, search_type, search_event_context) = match in_req.search_type { Some(search_type) => { if matches!(src/config/src/meta/search.rs (1)
777-805: Consider accepting&strinstead ofStringin constructor methods.To improve performance and avoid unnecessary cloning, consider changing the parameters of the
with_*methods to accept&strand convert them toStringwithin the methods.Apply this diff to the methods:
-pub fn with_alert(alert_key: String) -> Self { +pub fn with_alert(alert_key: &str) -> Self { Self { - alert_key: Some(alert_key), + alert_key: Some(alert_key.to_string()), ..Default::default() } } ... -pub fn with_report(report_key: String) -> Self { +pub fn with_report(report_key: &str) -> Self { Self { - report_key: Some(report_key), + report_key: Some(report_key.to_string()), ..Default::default() } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (18)
- src/cli/data/export.rs (3 hunks)
- src/common/utils/http.rs (2 hunks)
- src/config/src/meta/search.rs (6 hunks)
- src/config/src/meta/usage.rs (5 hunks)
- src/handler/http/request/search/mod.rs (6 hunks)
- src/handler/http/request/search/multi_streams.rs (6 hunks)
- src/handler/http/request/traces/mod.rs (1 hunks)
- src/handler/http/router/openapi.rs (1 hunks)
- src/service/alerts/alert.rs (2 hunks)
- src/service/alerts/derived_streams.rs (4 hunks)
- src/service/alerts/mod.rs (4 hunks)
- src/service/alerts/scheduler.rs (1 hunks)
- src/service/db/enrichment_table.rs (1 hunks)
- src/service/metrics/prom.rs (2 hunks)
- src/service/search/cache/mod.rs (1 hunks)
- src/service/search/mod.rs (3 hunks)
- src/service/usage/mod.rs (2 hunks)
- src/service/usage/stats.rs (2 hunks)
🧰 Additional context used
📓 Path-based instructions (18)
src/cli/data/export.rs (1)
Pattern
**/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestionsMake sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
src/common/utils/http.rs (1)
Pattern
**/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestionsMake sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
src/config/src/meta/search.rs (1)
Pattern
**/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestionsMake sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
src/config/src/meta/usage.rs (1)
Pattern
**/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestionsMake sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
src/handler/http/request/search/mod.rs (1)
Pattern
**/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestionsMake sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
src/handler/http/request/search/multi_streams.rs (1)
Pattern
**/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestionsMake sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
src/handler/http/request/traces/mod.rs (1)
Pattern
**/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestionsMake sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
src/handler/http/router/openapi.rs (1)
Pattern
**/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestionsMake sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
src/service/alerts/alert.rs (1)
Pattern
**/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestionsMake sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
src/service/alerts/derived_streams.rs (1)
Pattern
**/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestionsMake sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
src/service/alerts/mod.rs (1)
Pattern
**/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestionsMake sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
src/service/alerts/scheduler.rs (1)
Pattern
**/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestionsMake sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
src/service/db/enrichment_table.rs (1)
Pattern
**/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestionsMake sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
src/service/metrics/prom.rs (1)
Pattern
**/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestionsMake sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
src/service/search/cache/mod.rs (1)
Pattern
**/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestionsMake sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
src/service/search/mod.rs (1)
Pattern
**/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestionsMake sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
src/service/usage/mod.rs (1)
Pattern
**/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestionsMake sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
src/service/usage/stats.rs (1)
Pattern
**/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestionsMake sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
🔇 Additional comments (35)
src/cli/data/export.rs (2)
27-30: LGTM! Imports are well-organizedThe new imports are properly organized and necessary for the search event context functionality.
78-78: LGTM! Verify SearchEventContext usageThe addition of
search_event_contextto the Request struct is correct and aligns with the PR objectives.✅ Verification successful
SearchEventContext usage is properly integrated
The verification shows that
search_event_contextis correctly used throughout the codebase:
- Defined in
src/config/src/meta/search.rswith proper serialization attributes- Used consistently in search-related request structs
- Properly handled in HTTP request processing (
src/handler/http/request/search/)- Integrated with alerts, usage tracking, and other relevant services
- The addition in
src/cli/data/export.rsfollows the same pattern as other implementations🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the SearchEventContext struct usage across the codebase ast-grep --pattern 'struct $struct_name { $$$ search_event_context: $type, $$$ }'Length of output: 89
Script:
#!/bin/bash # Let's try a different approach to find SearchEventContext usage # First, find the struct definition rg -A 5 "struct SearchEventContext" # Then find its usage rg "SearchEventContext" # Also check for snake_case version rg "search_event_context"Length of output: 6020
src/service/db/enrichment_table.rs (1)
54-54: LGTM! Verify consistency with related changes.The addition of
search_event_contextis well-implemented and aligns with the PR objectives to enhance search queries with additional context.Let's verify the consistency of this change with related files:
✅ Verification successful
Let me generate one more verification to check the SearchEventContext struct definition since it wasn't found in the previous results:
The
search_event_contextfield is consistently implemented across the codebaseThe verification shows that:
- The
SearchEventContextstruct is properly defined insrc/config/src/meta/search.rswith appropriate serialization attributes- The field is consistently initialized as
Noneacross all Request structs in various modules- It's properly used with
Somevariants in alert and dashboard contexts where needed- The implementation in
enrichment_table.rsfollows the same pattern as other modules🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all Request structs consistently include search_event_context # and check its usage pattern across the codebase # Check the SearchEventContext struct definition and its fields ast-grep --pattern 'struct SearchEventContext { $$$ }' # Check Request structs that should include search_event_context ast-grep --pattern 'struct Request { $$$ search_event_context: $_, $$$ }' # Verify the initialization pattern rg 'search_event_context: None' -A 2Length of output: 3270
Script:
#!/bin/bash # Search for SearchEventContext type references to understand its definition rg "SearchEventContext" -B 2 -A 2 # Check if there are any non-None initializations of search_event_context rg "search_event_context:\s+Some" -B 2 -A 2Length of output: 6306
src/common/utils/http.rs (1)
26-29: LGTM: Import changes are appropriate.The new imports for
SearchEventContextandSearchEventTypeare correctly organized and necessary for the new functionality.src/config/src/meta/usage.rs (2)
18-18: LGTM: Clean import additionThe SearchEventContext import is appropriately grouped with the related SearchEventType.
299-299: LGTM: Consistent implementation updatesThe
search_event_contextfield is correctly initialized toNonein both theDefaultandFrom<FileMeta>implementations, maintaining consistency with other optional fields.Also applies to: 323-323
src/handler/http/router/openapi.rs (1)
165-165: LGTM! Verify schema completeness.The addition of
SearchEventContextto the OpenAPI schema components is correct and aligns with the PR objectives.Let's verify that all required fields from the PR objectives are included in the schema:
✅ Verification successful
Schema implementation is complete and correctly documented
The SearchEventContext struct in
src/config/src/meta/search.rscontains all the necessary fields for search usage reports:
alert_keyderived_stream_keyreport_keydashboard_nameanddashboard_folder(via the implementation methods)The OpenAPI documentation correctly includes this schema, and its usage is properly integrated across the codebase in alerts, derived streams, and usage tracking.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify SearchEventContext schema includes all required fields # Expected fields from PR objectives: dashboard_name, dashboard_folder, report_key, alert_key # Search for the SearchEventContext struct definition ast-grep --pattern 'struct SearchEventContext { $$$ }'Length of output: 58
Script:
#!/bin/bash # Let's try a different approach to find the SearchEventContext definition # First, let's use ripgrep to find files containing SearchEventContext rg "SearchEventContext" -l # Then let's see the actual content with context rg "SearchEventContext" -A 10 -B 2Length of output: 13978
src/service/usage/mod.rs (2)
105-105: LGTM: Search context addition for function usage trackingThe addition of
search_event_contextwith clone is appropriate here and aligns with the PR objectives.
143-143: Verify aggregation handling with new search contextThe addition looks good, but let's verify the aggregation logic handles the new field correctly.
src/service/search/cache/mod.rs (1)
301-301: LGTM: Search event context tracking added correctly.The addition of
search_event_contexttoRequestStatsaligns with the PR objectives to enhance search queries with additional context (dashboard, report, and alert information) for usage reports.Let's verify the consistent usage of this field across the codebase:
✅ Verification successful
Search event context implementation is consistent across the codebase
The verification confirms:
SearchEventContextstruct is properly defined insrc/config/src/meta/search.rswith expected fields likealert_key- The field is consistently used across multiple components:
- Search handlers (
request/search/mod.rs,request/search/multi_streams.rs)- Usage tracking (
service/usage/stats.rs,service/usage/mod.rs)- Cache implementation (
service/search/cache/mod.rs)- Alert system (
service/alerts/mod.rs)- All implementations properly handle the optional nature of the context with
Option<SearchEventContext>- The cloning pattern is consistently applied when propagating the context
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of search_event_context across the codebase # Test 1: Check for struct definitions and field usage rg -A 5 "struct.*SearchEventContext" # Test 2: Check for places where search_event_context is used rg "search_event_context" # Test 3: Check for request stats usage to ensure consistent implementation ast-grep --pattern 'struct RequestStats { $$$ search_event_context: $_, $$$ }'Length of output: 4396
src/service/alerts/mod.rs (3)
77-78: LGTM: Function signature changes align with PR objectives.The new optional parameters
search_typeandsearch_event_contextare properly typed and named.
322-323: LGTM: Consistent implementation of search context parameters.The search context parameters are correctly forwarded to the
Requeststruct, maintaining consistency with theMultiStreamRequestimplementation.
264-265: Consider validating search context parameters.While the parameters are correctly forwarded to
MultiStreamRequest, consider adding validation to ensure the search context is appropriate for the alert type being evaluated.src/service/metrics/prom.rs (1)
666-666: LGTM! Consistent implementation of search event context.The addition of
search_event_context: Noneto the Request structs in bothget_series()andget_label_values()functions is consistent with the PR objectives of enhancing search queries with additional context parameters.Let's verify the Request struct definition and its usage:
Also applies to: 812-812
✅ Verification successful
Let me generate one more verification to check the struct definition and type:
LGTM! The
search_event_contextfield is properly defined and consistently used.The verification confirms:
SearchEventContextis properly defined as a struct insrc/config/src/meta/search.rs- The field is correctly added as
Option<SearchEventContext>across multiple request structs- The implementation in
prom.rsfollows the same pattern as other files in the codebase- All usages initialize it with
Noneas a safe default value🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the Request struct definition and its usage # Expected: The struct should have search_event_context field defined # Search for the Request struct definition ast-grep --pattern 'struct Request { $$$ search_event_context: $_, $$$ }' # Search for other usages of Request with search_event_context rg 'search_event_context:' -A 2 -B 2Length of output: 9981
Script:
#!/bin/bash # Search for SearchEventContext type definition rg "struct SearchEventContext" -A 5 -B 1 # Search for Request struct definition rg "struct Request" -A 10 -B 1Length of output: 8105
src/handler/http/request/search/multi_streams.rs (4)
40-41: LGTM: Import changes align with new functionality.The addition of
get_search_event_context_from_requestto the imports is appropriate for the new search event context feature.
348-348: LGTM: Proper integration of search event context in RequestStats.The search_event_context is correctly added to RequestStats, enabling proper tracking of search context in usage reports.
566-566: LGTM: Consistent usage of search event context.The search_event_context is properly propagated from multi_req to the request stats for function usage reporting.
931-931: 🛠️ Refactor suggestionVerify if search_event_context should be None in around_multi.
The around_multi function sets search_event_context to None for both forward and backward search requests. Consider deriving the context from the request parameters similar to other search functions for consistency.
Consider updating the requests to include the search event context:
search_type: Some(search::SearchEventType::UI), -search_event_context: None, +search_event_context: get_search_event_context_from_request( + &search::SearchEventType::UI, + &query +), index_type: "".to_string(),Also applies to: 1005-1005
src/service/search/mod.rs (3)
340-340: LGTM: SearchEventContext properly integrated into RequestStatsThe addition of
search_event_contextto RequestStats is well-implemented and aligns with the PR objectives for enhancing search usage reports.
508-508: LGTM: Consistent implementation of SearchEventContextThe addition of
search_event_contextto RequestStats in the search function maintains consistency with the search_multi implementation, ensuring uniform usage reporting across both search paths.
Line range hint
465-508: Verify SearchEventContext propagationLet's verify that all necessary structures have been updated to include the search_event_context field and that it's properly propagated through the search workflow.
✅ Verification successful
Based on the search results, I can now generate the final response:
SearchEventContext field is properly propagated and handled
The verification shows that SearchEventContext is consistently implemented across the codebase:
Defined in relevant structures:
RequestStatsin usage.rsRequestin search.rs- Multiple search-related structures in meta/search.rs
Properly propagated through:
- Search workflow (service/search/mod.rs)
- Cache handling (service/search/cache/mod.rs)
- Multi-stream search (handler/http/request/search/multi_streams.rs)
- Usage tracking (service/usage/mod.rs)
Correctly defaulted to
Nonein appropriate places:
- Alert handling
- Trace requests
- Metrics/Prometheus handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for structures and functions that should handle SearchEventContext echo "Searching for related structures and their SearchEventContext field..." rg -A 5 "struct.*Request" rg -A 5 "struct.*Response" rg -A 5 "struct.*Stats" echo "Searching for places where SearchEventContext is used..." rg "search_event_context"Length of output: 41735
src/handler/http/request/search/mod.rs (6)
42-43: LGTM: Import changes are well-organized.The new import for
get_search_event_context_from_requestis properly placed alongside related HTTP utility functions.
154-157: LGTM: Search event context is properly initialized.The implementation correctly sets the search event context based on the search type, using idiomatic Rust Option chaining.
477-477: LGTM: Around function properly handles search event context.The initialization of
search_event_contexttoNoneis appropriate for around searches.
908-908: LGTM: Values function properly handles search event context.The initialization of
search_event_contexttoNoneis appropriate for values searches.
1229-1229: LGTM: Values_v2 function properly handles search event context.The initialization of
search_event_contexttoNoneis appropriate for values_v2 searches.
42-43: Verify the implementation of get_search_event_context_from_request.Let's verify that the
get_search_event_context_from_requestfunction properly handles all the new parameters mentioned in the PR objectives (dashboard_name, dashboard_folder, report_key, alert_key).✅ Verification successful
Implementation of get_search_event_context_from_request is properly implemented
The function correctly handles all required parameters:
- For dashboards:
dashboard_nameand optionaldashboard_folder- For alerts:
alert_key- For reports:
report_keyThe implementation matches the struct definition and uses appropriate constructor methods for each search event type. The function properly extracts query parameters and maps them to the corresponding SearchEventContext variants.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of get_search_event_context_from_request # Test: Search for the function implementation ast-grep --pattern $'fn get_search_event_context_from_request($_, $_) { $$$ }'Length of output: 81
Script:
#!/bin/bash # Let's try a different approach to find the implementation # First, let's use ripgrep to find the function definition rg -U "fn get_search_event_context_from_request" -A 20 # Also search for the struct or type definition of SearchEventContext rg "struct SearchEventContext" -A 10Length of output: 2316
Script:
#!/bin/bash # Let's get the complete SearchEventContext struct definition and its methods rg "struct SearchEventContext" -A 20 # Also search for the with_dashboard, with_alert, and with_report methods rg "impl SearchEventContext" -A 30Length of output: 3114
src/config/src/meta/search.rs (6)
63-63: LGTM!The addition of the
index_typefield with a default value in theRequeststruct looks good.
442-443: LGTM!Initializing
search_event_contextandindex_typeto default values in theto_query_reqmethod is appropriate.
952-953: LGTM!Including
search_event_contextandindex_typein theto_query_reqmethod enhances the request generation appropriately.
1038-1039: LGTM!Setting
search_event_contexttoNoneand initializingindex_typewith an empty string in the test is appropriate.
60-62:⚠️ Potential issueVerify the use of
#[serde(flatten)]with anOptionfield.Using
#[serde(flatten)]on anOption<SearchEventContext>may not behave as intended when the option isNone. Flattening optional fields can lead to serialization issues. Consider whether flattening is necessary or adjust the implementation accordingly.
875-878:⚠️ Potential issueConsider moving the
#[serde(default)]attribute before the field declaration.The
#[serde(default)]attribute after thesearch_typefield may be intended forsearch_event_context. Attributes should be placed immediately before their associated items. Please reposition it abovepub search_event_context.Apply this diff:
#[serde(default)] #[serde(skip_serializing_if = "Option::is_none")] pub search_type: Option<SearchEventType>, -#[serde(default)] #[serde(flatten)] +#[serde(default)] #[serde(skip_serializing_if = "Option::is_none")] pub search_event_context: Option<SearchEventContext>,Likely invalid or redundant comment.
src/service/alerts/alert.rs (2)
25-28: Imports added correctlyThe new imports for
SearchEventContextandSearchEventTypeare appropriate and necessary for the implemented functionality.
360-370: Enhancedevaluatemethod appropriatelyThe
evaluatemethod now includesalert_keyand passesSearchEventTypeandSearchEventContextcorrectly toevaluate_scheduled, enriching the alert evaluation with additional context.
taimingl
force-pushed
the
feat/search-event-ctx
branch
3 times, most recently
from
4a0770a to
624388f
Compare
taimingl
force-pushed
the
feat/search-event-ctx
branch
from
624388f to
aea054e
Compare
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Outside diff range and nitpick comments (11)
src/report_server/src/router.rs (2)
Line range hint
94-99: Consider adding org_id to cache success logFor consistency with error logging, consider including the
org_idin the success log message as well.- log::info!("Dashboard data cached by report {report_name}"); + log::info!("Dashboard data cached by report {org_id}/{report_name}");
Line range hint
116-122: Add context to email error loggingFor consistency with report generation error logging, consider including the
org_idandreport_namein the email error log as well.- log::error!("Error sending emails to recipients: {e}"); + log::error!("Error sending emails to recipients for report {org_id}/{report_name}: {e}");src/report_server/src/report.rs (1)
Line range hint
139-148: Consider grouping parameters into a struct.The function has 8 parameters now, which makes it harder to maintain and use. Consider grouping related parameters into a struct, such as:
struct ReportConfig<'a> { org_id: &'a str, user_id: &'a str, user_pass: &'a str, web_url: &'a str, timezone: &'a str, report_type: ReportType, report_name: &'a str, }web/src/components/dashboards/PanelContainer.vue (1)
328-328: Consider adding TypeScript type definition for the reportId propWhile the prop addition is correct, consider enhancing type safety by properly defining the prop type:
- "reportId" + { + name: 'reportId', + type: String, + required: false + }This would:
- Provide better type safety
- Enable IDE autocompletion
- Make the code more maintainable
web/src/views/Dashboards/RenderDashboardCharts.vue (1)
214-214: Document the purpose of reportId in search contextThe addition of
reportIdis part of enhancing search queries with context for usage reports, but this isn't clear from the code. Consider adding documentation to help other developers understand its purpose and relationship with search usage reporting.Add JSDoc comments to explain the prop:
+ /** + * Unique identifier for the report, used to enhance search queries with additional + * context for usage reports. This enables tracking search events with their + * originating report context. + * @see PR #4934 for more details about search context enhancement + */ reportId: {src/service/alerts/mod.rs (1)
77-78: Consider documenting the new parametersThe new parameters enhance the function's capabilities, but their purpose and usage should be documented.
Add documentation for the new parameters:
+ /// Evaluates scheduled alerts + /// # Arguments + /// * `stream_param` - Stream parameters + /// * `trigger_condition` - Trigger condition for the alert + /// * `start_time` - Optional start time for evaluation + /// * `search_type` - Optional search event type for context + /// * `search_event_context` - Optional context for search events pub async fn evaluate_scheduled( &self, stream_param: &StreamParams, trigger_condition: &TriggerCondition, start_time: Option<i64>, search_type: Option<SearchEventType>, search_event_context: Option<SearchEventContext>, ) -> Result<(Option<Vec<Map<String, Value>>>, i64), anyhow::Error> {web/src/components/dashboards/PanelSchemaRenderer.vue (1)
Line range hint
4-5: Consider adding JSDoc comments for the new reportId parameter.To improve code documentation and maintainability, consider adding JSDoc comments explaining the purpose and usage of the
reportIdparameter in search usage reports.+/** + * @prop {string} reportId - Unique identifier for tracking search usage in reports + */src/config/src/meta/search.rs (2)
441-441: Consider adding a comment explaining the default None value.Adding a brief comment would help explain why
search_event_contextis initialized asNonein theto_query_reqmethod.
1049-1049: Add test coverage for SearchEventContext.The test case should be expanded to include scenarios with non-None values for
search_event_contextto ensure proper serialization and conversion behavior.Consider adding tests like:
#[test] fn test_search_with_context() { let context = SearchEventContext::with_dashboard( Some("dash-1".to_string()), Some("Dashboard 1".to_string()), None, None, ); let req = Request { // ... other fields ... search_event_context: Some(context), // ... other fields ... }; let rpc_req = cluster_rpc::SearchRequest::from(req.clone()); // Assert context is properly converted }web/src/composables/dashboard/usePanelDataLoader.ts (1)
67-68: Consider adding proper TypeScript types for the new parameter.The
reportIdparameter is typed asany. Consider adding a more specific type to improve type safety and code maintainability.- reportId: any + reportId: string | number | nullsrc/job/files/parquet.rs (1)
294-294: Consider extracting the split segment count as a constant.The change from 5 to 9 segments correctly handles both log and trace file paths. However, to improve maintainability, consider extracting the magic number 9 into a named constant.
+const PATH_SEGMENT_COUNT: usize = 9; + - let columns = prefix.splitn(9, '/').collect::<Vec<&str>>(); + let columns = prefix.splitn(PATH_SEGMENT_COUNT, '/').collect::<Vec<&str>>();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (28)
src/cli/data/export.rs(3 hunks)src/common/utils/http.rs(2 hunks)src/config/src/meta/search.rs(6 hunks)src/config/src/meta/usage.rs(5 hunks)src/handler/http/request/search/mod.rs(6 hunks)src/handler/http/request/search/multi_streams.rs(6 hunks)src/handler/http/request/traces/mod.rs(1 hunks)src/handler/http/router/openapi.rs(1 hunks)src/job/files/parquet.rs(1 hunks)src/report_server/src/report.rs(5 hunks)src/report_server/src/router.rs(1 hunks)src/service/alerts/alert.rs(2 hunks)src/service/alerts/derived_streams.rs(4 hunks)src/service/alerts/mod.rs(4 hunks)src/service/alerts/scheduler.rs(1 hunks)src/service/dashboards/reports.rs(5 hunks)src/service/db/enrichment_table.rs(1 hunks)src/service/metrics/prom.rs(2 hunks)src/service/search/cache/mod.rs(1 hunks)src/service/search/mod.rs(3 hunks)src/service/usage/mod.rs(2 hunks)src/service/usage/stats.rs(2 hunks)web/src/components/dashboards/PanelContainer.vue(2 hunks)web/src/components/dashboards/PanelSchemaRenderer.vue(3 hunks)web/src/composables/dashboard/usePanelDataLoader.ts(5 hunks)web/src/services/search.ts(2 hunks)web/src/views/Dashboards/RenderDashboardCharts.vue(3 hunks)web/src/views/Dashboards/ViewDashboard.vue(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- src/cli/data/export.rs
- src/config/src/meta/usage.rs
- src/handler/http/request/search/mod.rs
- src/handler/http/request/search/multi_streams.rs
- src/handler/http/router/openapi.rs
- src/service/db/enrichment_table.rs
- src/service/metrics/prom.rs
- src/service/search/cache/mod.rs
- src/service/usage/mod.rs
- src/service/usage/stats.rs
🧰 Additional context used
📓 Path-based instructions (12)
src/common/utils/http.rs (1)
Pattern **/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestions
Make sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
src/config/src/meta/search.rs (1)
Pattern **/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestions
Make sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
src/handler/http/request/traces/mod.rs (1)
Pattern **/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestions
Make sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
src/job/files/parquet.rs (1)
Pattern **/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestions
Make sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
src/report_server/src/report.rs (1)
Pattern **/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestions
Make sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
src/report_server/src/router.rs (1)
Pattern **/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestions
Make sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
src/service/alerts/alert.rs (1)
Pattern **/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestions
Make sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
src/service/alerts/derived_streams.rs (1)
Pattern **/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestions
Make sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
src/service/alerts/mod.rs (1)
Pattern **/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestions
Make sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
src/service/alerts/scheduler.rs (1)
Pattern **/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestions
Make sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
src/service/dashboards/reports.rs (1)
Pattern **/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestions
Make sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
src/service/search/mod.rs (1)
Pattern **/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestions
Make sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
🔇 Additional comments (25)
src/report_server/src/router.rs (2)
82-82: LGTM: Report name parameter addition
The addition of &report_name to generate_report call enhances context tracking and aligns with the PR's objective of improving search usage reports.
Line range hint 86-91: Good error logging enhancement
Error logging now includes both org_id and report_name, providing better context for debugging issues during report generation.
web/src/services/search.ts (1)
27-28: LGTM: Parameter declarations follow TypeScript best practices.
The new optional parameters dashboard_id and folder_id are properly typed and align with the PR objectives to enhance search queries with additional context.
Also applies to: 34-35
src/service/alerts/derived_streams.rs (2)
21-21: LGTM: Clean import addition
The new imports for search event context align with the PR objectives.
124-128: LGTM: Clean implementation of trigger module key handling
The changes properly implement the trigger module key handling and follow best practices by passing references.
src/common/utils/http.rs (1)
26-29: LGTM: Clean import organization.
The new imports are properly organized within the existing meta module block.
src/report_server/src/report.rs (2)
224-226: LGTM! Clear and correct implementation.
The search type parameter logic properly handles both cache and report scenarios.
Line range hint 139-148: Verify all callers are updated with the new parameter.
Let's ensure all callers of generate_report have been updated with the new report_name parameter.
✅ Verification successful
Let me gather more context about the function definition and its callers by checking the complete function signatures.
All callers of generate_report are properly updated with the new parameter
The verification shows that all callers of generate_report are correctly passing the report_name parameter:
- In
src/service/dashboards/reports.rs: Passes&self.name - In
src/report_server/src/router.rs: Passes&report_name
Both implementations match the function signature in src/report_server/src/report.rs which requires the report_name: &str parameter.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for generate_report function calls
rg -A 5 "generate_report\(" --type rust
Length of output: 1589
Script:
#!/bin/bash
# Get more context around generate_report function definitions and calls
rg -A 10 "generate_report\(" --type rust
Length of output: 2848
web/src/components/dashboards/PanelContainer.vue (1)
240-240: LGTM: Prop binding follows Vue.js conventions
The addition of :report-id prop binding to PanelSchemaRenderer follows Vue.js best practices and correctly implements the report context enhancement.
src/service/alerts/mod.rs (3)
22-22: LGTM: Import addition is appropriate
The addition of SearchEventContext and SearchEventType imports aligns with the new parameters being used.
264-265: LGTM: Consistent propagation of search context
The search context parameters are consistently propagated to both MultiStreamRequest and Request structs.
Also applies to: 322-323
77-78: Verify the impact on existing alert evaluations
The addition of optional parameters maintains backward compatibility, but we should verify that existing alerts continue to work as expected.
Also applies to: 264-265, 322-323
✅ Verification successful
Let me gather more context about the alert evaluation implementation to verify the impact.
Let me try a different approach to check the implementation.
Let me check how these parameters are used in the call sites.
Optional parameters in evaluate_scheduled are safely implemented
The verification shows that:
- The new parameters
search_typeandsearch_event_contextare added asOption<T>types, maintaining backward compatibility - Existing call sites in
derived_streams.rsandalert.rscontinue to work without these parameters - The function implementation handles the optional parameters gracefully, focusing on core alert evaluation logic with
stream_paramandtrigger_condition
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing alert evaluations to ensure they're not affected
rg -A 2 "evaluate_scheduled\(" --type rust
Length of output: 660
Script:
#!/bin/bash
# Check the implementation of evaluate_scheduled to understand parameter usage
ast-grep --pattern 'async fn evaluate_scheduled($$$)'
Length of output: 55
Script:
#!/bin/bash
# Check the implementation details around evaluate_scheduled
rg -B 5 -A 15 "pub async fn evaluate_scheduled" --type rust
Length of output: 1350
Script:
#!/bin/bash
# Check how evaluate_scheduled is called in the codebase
rg -B 3 -A 2 "\.evaluate_scheduled\(" --type rust
Length of output: 932
web/src/components/dashboards/PanelSchemaRenderer.vue (2)
231-235: LGTM: New prop declaration follows Vue.js best practices.
The reportId prop is correctly defined with appropriate type, default value, and required flag.
267-267: Verify the impact of reportId on panel data loading.
The reportId is correctly destructured from props and passed to usePanelDataLoader. However, we should verify that:
- The panel data loading behavior remains consistent when
reportIdis empty - The search usage reports are correctly associated with the provided
reportId
Also applies to: 287-288
✅ Verification successful
Based on the code analysis from the provided files, I can generate the final response:
The reportId parameter is safely handled in panel data loading.
The code review shows that:
reportIdis properly passed through tousePanelDataLoaderas an optional prop with a default empty string value- The panel data loading implementation in
usePanelDataLoader.tshandlesreportIdsafely as part of the search request parameters - Empty
reportIdvalues do not affect the core panel data loading functionality
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usages of usePanelDataLoader to verify consistent parameter handling
ast-grep --pattern 'usePanelDataLoader($$$)'
# Search for reportId usage in related files
rg -l "reportId" --type ts --type vue
Length of output: 112
Script:
#!/bin/bash
# Search for usages of usePanelDataLoader with proper file types
rg -t js -t jsx -t ts -t tsx -t vue "usePanelDataLoader" -A 3 -B 3
# Search for reportId usage in related files with proper file types
rg -t js -t jsx -t ts -t tsx -t vue "reportId" -A 3 -B 3
# Find all Vue files that might contain panel data loading logic
fd -e vue -e ts -e js "Panel.*"
Length of output: 863
Script:
#!/bin/bash
# Check the implementation of usePanelDataLoader
cat web/src/composables/dashboard/usePanelDataLoader.ts
# Check PanelSchemaRenderer.vue for reportId usage context
cat web/src/components/dashboards/PanelSchemaRenderer.vue
# Check how reportId is used in panel data loading
rg -t js -t ts -t vue "reportId.*panel" -A 3 -B 3
Length of output: 85927
web/src/views/Dashboards/ViewDashboard.vue (1)
213-213: Verify report ID propagation in the codebase
The prop binding syntax is correct, but we need to ensure that the report ID is being properly propagated throughout the application.
Let's verify the usage of report ID in the codebase:
Also applies to: 988-988
✅ Verification successful
Let me try a different approach to verify the report ID propagation.
Report ID propagation is properly implemented
The verification shows that the reportId prop is correctly propagated through the component hierarchy:
- In
ViewDashboard.vue,reportIdis derived from route query parametertaband passed down RenderDashboardCharts.vueaccepts and forwards thereportIdpropPanelContainer.vueandPanelSchemaRenderer.vuecomponents properly handle thereportIdprop- The prop is used in the data loading logic via
usePanelDataLoader.ts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for report ID usage in route queries and component props
echo "Searching for report query parameter usage..."
rg -A 3 "route\.query\.report"
echo "Searching for reportId prop in RenderDashboardCharts component..."
ast-grep --pattern 'const props = defineProps<{
$$$
reportId?: $_
$$$
}>();'
Length of output: 359
Script:
#!/bin/bash
# Search for reportId in the codebase
echo "Searching for reportId usage..."
rg -A 3 "reportId"
# Search for prop definitions containing reportId
echo -e "\nSearching for components with reportId prop..."
ast-grep --pattern 'defineProps<{
$$$
reportId$_: $_
$$$
}>'
# Search for report ID bindings in templates
echo -e "\nSearching for reportId bindings in templates..."
rg -A 3 ':reportId'
Length of output: 5687
src/service/alerts/alert.rs (1)
25-28: LGTM: Clean import organization
The new imports for SearchEventContext and SearchEventType are properly organized within the existing import structure.
src/config/src/meta/search.rs (2)
56-61: LGTM: Well-structured field additions to Request struct.
The new fields are properly implemented with appropriate serde attributes for optional fields.
886-891: LGTM: Consistent implementation with Request struct.
The changes maintain consistency with the Request struct implementation and properly handle ownership in the to_query_req method.
Also applies to: 963-963
src/service/alerts/scheduler.rs (1)
873-875: LGTM! The code follows Rust best practices.
The implementation correctly passes &trigger.module_key as a reference, which is memory efficient and follows Rust's ownership principles.
src/service/search/mod.rs (2)
340-340: LGTM: Correctly adds search event context to request stats.
The addition of search_event_context to RequestStats aligns with the PR objectives to enhance search queries with additional context for usage reports.
465-482: LGTM: Clean refactoring of search type handling.
The refactoring improves readability by:
- Using pattern matching for cleaner control flow
- Correctly determining report_usage based on search type
- Properly maintaining search_event_context when needed
web/src/composables/dashboard/usePanelDataLoader.ts (1)
559-564: LGTM: Search context enhancement implemented correctly.
The addition of dashboard_id and folder_id to the search API calls is implemented consistently and aligns with the PR objectives to enhance search context for usage reports.
Also applies to: 767-772
src/service/dashboards/reports.rs (2)
355-355: Ensure self.name is valid when passing to generate_report
While adding &self.name to the generate_report function, ensure that self.name is validated and non-empty to prevent potential issues during report generation.
427-427: Addition of report_name parameter is appropriate
The inclusion of report_name in the generate_report function signature aligns with the enhanced functionality and usage consistency across the codebase.
src/handler/http/request/traces/mod.rs (1)
275-275:
Implement search_event_context handling
The search_event_context field is set to None but is not utilized in the subsequent code. Based on the PR objectives, this field should capture dashboard, report, and alert context parameters. Please ensure that search_event_context is properly populated and integrated where necessary.
Would you like assistance in implementing the search_event_context handling as per the PR objectives?
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
src/service/usage/mod.rs (1)
465-466: LGTM: Memory optimization using BoxGood optimization to use
Box<T>for enum variants, which reduces the enum's stack size. Consider adding a comment explaining this optimization.Add a doc comment explaining the use of
Box:#[derive(Debug)] +/// Uses Box to reduce the enum's stack size as UsageData and TriggerData might be large structs enum UsageBuffer { Usage(Box<UsageData>), Trigger(Box<TriggerData>), }src/service/metrics/prom.rs (2)
740-740: Add documentation for the new field.The
search_event_contextfield has been added to align with the PR objectives of enhancing search queries with additional context for usage reports. However, the field lacks documentation explaining its purpose and usage.Add a doc comment explaining the field's purpose:
search_type: None, + /// Context information for search usage reporting, including dashboard, report, and alert details search_event_context: None, index_type: "".to_string(),
886-886: Maintain consistency in field documentation.The same field is added here but also lacks documentation. Let's maintain consistency across the codebase.
Add the same doc comment here:
search_type: None, + /// Context information for search usage reporting, including dashboard, report, and alert details search_event_context: None, index_type: "".to_string(),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
src/handler/http/request/search/multi_streams.rs(6 hunks)src/handler/http/router/openapi.rs(1 hunks)src/service/alerts/alert.rs(2 hunks)src/service/alerts/derived_streams.rs(6 hunks)src/service/alerts/mod.rs(5 hunks)src/service/alerts/scheduler.rs(1 hunks)src/service/metrics/prom.rs(2 hunks)src/service/search/mod.rs(3 hunks)src/service/usage/mod.rs(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/handler/http/request/search/multi_streams.rs
- src/handler/http/router/openapi.rs
🧰 Additional context used
📓 Path-based instructions (7)
src/service/alerts/alert.rs (1)
Pattern **/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestions
Make sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
src/service/alerts/derived_streams.rs (1)
Pattern **/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestions
Make sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
src/service/alerts/mod.rs (1)
Pattern **/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestions
Make sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
src/service/alerts/scheduler.rs (1)
Pattern **/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestions
Make sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
src/service/metrics/prom.rs (1)
Pattern **/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestions
Make sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
src/service/search/mod.rs (1)
Pattern **/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestions
Make sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
src/service/usage/mod.rs (1)
Pattern **/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestions
Make sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
🔇 Additional comments (15)
src/service/alerts/derived_streams.rs (5)
25-25: LGTM: Clean import addition
The new imports are properly organized within the existing meta imports group.
81-82: LGTM: Proper module key handling
The changes correctly generate and pass the trigger_module_key to evaluate(), maintaining proper ownership semantics by passing it as a reference.
94-94: LGTM: Consistent module key usage
The trigger creation correctly uses the generated module_key.
131-131: LGTM: Proper trait signature update
The trait signature correctly uses &str for module_key, following Rust best practices for string references.
156-159: Skip: Optimization suggestion already provided
A previous review already suggested optimizing the SearchEventContext creation by removing double Some wrapping and unnecessary string allocation.
src/service/usage/mod.rs (2)
141-141: LGTM: Search event context field added correctly
The search_event_context field is properly added to both UsageData instances, maintaining consistency in usage tracking.
Also applies to: 179-179
198-203: LGTM: Consistent usage of boxed variants
The boxed variants are correctly used across all sites, maintaining consistency in the implementation.
Also applies to: 222-222, 331-336, 352-357, 383-388, 416-421, 556-557
src/service/alerts/mod.rs (2)
24-24: LGTM: Import added correctly
The SearchEventContext import is properly placed with related imports from the search module.
286-287: LGTM: Search context parameters correctly propagated
The search_type and search_event_context parameters are consistently passed to both MultiStreamRequest and Request structs.
Also applies to: 337-338
src/service/metrics/prom.rs (1)
740-740: Verify the type of search_event_context across the codebase.
The new field uses Option<T> which is correct for nullable fields, but we should verify that the same type is used consistently across the codebase.
Also applies to: 886-886
✅ Verification successful
Based on the search results, I can generate the final response as we have comprehensive information about the usage of search_event_context across the codebase.
The type Option<SearchEventContext> is used consistently across the codebase
The field search_event_context is consistently defined as Option<SearchEventContext> in all struct definitions and handled appropriately in the codebase:
- In config definitions (
src/config/src/meta/search.rs,src/config/src/meta/usage.rs) - In service implementations (
src/service/alerts/mod.rs,src/service/search/mod.rs) - In handlers (
src/handler/http/request/search/mod.rs)
The new field's type is consistent with the existing pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all occurrences of SearchEventContext to verify type consistency
rg -A 1 "search_event_context.*:"
Length of output: 4384
src/service/alerts/scheduler.rs (1)
875-877: LGTM! Method signature change looks good.
The changes correctly pass references to avoid unnecessary cloning, which is a good practice in Rust.
Let's verify the method signature change across the codebase:
✅ Verification successful
Based on the gathered information, I can now provide a final response:
Method signature change is correctly implemented and consistent
The verification confirms:
- The
DerivedStreamExttrait insrc/service/alerts/derived_streams.rsdefines the correct method signature withmodule_key: &str - The implementation for
DerivedStreamfollows this signature - The usage in
scheduler.rscorrectly obtains aderived_streamfrompipeline.get_derived_stream()and calls the method with proper reference passing
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the evaluate method signature in DerivedStreamExt implementations
# Expect: All implementations should accept &str for module_key parameter
ast-grep --pattern 'impl $_ for $_ {
$$$
fn evaluate(&$_, $_, $module_key: &str) $$ {
$$$
}
$$$
}'
Length of output: 112
Script:
#!/bin/bash
# Let's try a different approach to find evaluate method implementations
# First, find the trait definition
echo "=== Searching for trait definition ==="
rg "trait.*DerivedStream.*" -A 5
echo -e "\n=== Searching for evaluate method implementations ==="
# Search for evaluate method implementations
rg "fn evaluate.*&.*module_key" -A 3
Length of output: 750
Script:
#!/bin/bash
# Let's search for implementations of DerivedStreamExt
echo "=== Searching for DerivedStreamExt implementations ==="
rg "impl.*DerivedStreamExt.*for" -A 10
# Also search for the struct that's using evaluate
echo -e "\n=== Finding derived_stream type ==="
rg -B 5 "let \(ret, end_time\) = derived_stream" src/service/alerts/scheduler.rs
Length of output: 1272
Script:
#!/bin/bash
# Let's find the complete evaluate method implementation
echo "=== Finding evaluate method implementation ==="
rg "async fn evaluate.*module_key.*" -A 15 src/service/alerts/derived_streams.rs
# Let's also check the type declaration of derived_stream variable
echo -e "\n=== Finding derived_stream variable declaration ==="
rg "let.*derived_stream.*=" -B 5 src/service/alerts/scheduler.rs
Length of output: 659
src/service/search/mod.rs (2)
340-340: LGTM: Correctly adds search event context to RequestStats.
The addition of search_event_context to RequestStats is consistent with the PR objectives and properly handles ownership through cloning.
465-482: LGTM: Clean refactoring of search event context handling.
The refactoring improves code clarity by:
- Using idiomatic Rust pattern matching
- Combining related logic for search_type and search_event_context
- Clearly defining when usage reporting should occur
src/service/alerts/alert.rs (2)
32-32: Approved: Added necessary imports for search context
The import of SearchEventContext and SearchEventType is appropriate and necessary for the new functionality.
389-390: Approved: Passing additional context to evaluate_scheduled
The inclusion of Some(SearchEventType::Alerts) and Some(search_event_ctx) correctly provides the necessary context for the scheduled evaluation.
| #[allow(clippy::too_many_arguments)] | ||
| async fn evaluate_scheduled( | ||
| &self, | ||
| org_id: &str, | ||
| stream_name: Option<&str>, | ||
| stream_type: StreamType, | ||
| trigger_condition: &TriggerCondition, | ||
| start_time: Option<i64>, | ||
| search_type: Option<SearchEventType>, | ||
| search_event_context: Option<SearchEventContext>, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider grouping parameters into a dedicated struct
The method has 8 parameters which could make it harder to maintain. Consider creating a dedicated struct to group related parameters.
Example implementation:
pub struct EvaluateScheduledParams<'a> {
pub org_id: &'a str,
pub stream_name: Option<&'a str>,
pub stream_type: StreamType,
pub trigger_condition: &'a TriggerCondition,
pub start_time: Option<i64>,
pub search_type: Option<SearchEventType>,
pub search_event_context: Option<SearchEventContext>,
}Then update the method signature:
async fn evaluate_scheduled(
&self,
params: EvaluateScheduledParams<'_>,
) -> Result<(Option<Vec<Map<String, Value>>>, i64), anyhow::Error>;| let search_event_ctx = SearchEventContext::with_alert(Some(format!( | ||
| "/alerts/{}/{}/{}/{}", | ||
| self.org_id, self.stream_type, self.stream_name, self.name | ||
| ))); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring alert path construction for consistency
Constructing the alert path inline may lead to inconsistencies across the codebase. Consider creating a helper function or using a constant to build the alert path, ensuring uniformity and easier maintenance.
impl #4914 new parameters acceptable for search queries to add additional context in usage report: **Dashboard** `dashboard_id` `dashboard_name` `dashboard_folder` `dashboard_folder_id` `dashboard_folder_name` **Report** `report_key` **Alert** `alert_key` Co-authored-by: Sai Nikhil <[email protected]> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Hengfei Yang <[email protected]>
impl #4914
new parameters acceptable for search queries to add additional context in usage report:
Dashboard
dashboard_iddashboard_namedashboard_folderdashboard_folder_iddashboard_folder_nameReport
report_keyAlert
alert_keySummary by CodeRabbit
Release Notes
New Features
search_event_contextto enhance search requests with additional context.reportIdprop to multiple components for improved reporting capabilities.Bug Fixes
Documentation
SearchEventContextfor better API documentation.Refactor
These changes collectively enhance the search functionality, reporting capabilities, and overall robustness of the application.