fix: include exxclude search based on data type #4841
Conversation
WalkthroughThe pull request introduces significant modifications across multiple components related to log filtering functionality. A new method Changes
Possibly related PRs
Suggested labels
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: 3
🧹 Outside diff range and nitpick comments (9)
web/src/plugins/logs/data-table/CellActions.vue (2)
66-71: LGTM: EnhancedaddSearchTermfunction with improved type safetyThe updated
addSearchTermfunction signature and implementation are well-designed. The changes provide better type safety and more flexibility in handling different field types. This modification aligns perfectly with the template changes and improves the overall functionality of the component.Consider adding JSDoc comments to the
addSearchTermfunction to document the purpose of each parameter and the function itself. This would enhance code readability and maintainability. For example:/** * Emits an event to add a search term based on the given field, value, and action. * @param {string} field - The field to search in. * @param {string | number | boolean} field_value - The value to search for. * @param {string} action - The action to perform ('include' or 'exclude'). */ const addSearchTerm = ( field: string, field_value: string | number | boolean, action: string, ) => { emit("addSearchTerm", field, field_value, action); };
Inconsistent Usage of
addSearchTermDetectedThe
addSearchTermmethod is being called with differing numbers of arguments across the codebase:
- 0 Arguments:
web/components/rum/SearchBar.vueweb/plugins/traces/SearchBar.vueweb/plugins/logs/SearchBar.vueweb/plugins/logs/IndexList.vue- 1 Argument:
web/plugins/traces/fields-sidebar/BasicValuesFilter.vueweb/components/common/sidebar/FieldList.vue- 3 Arguments (Correct Usage):
web/plugins/logs/JsonPreview.vueweb/plugins/logs/data-table/CellActions.vueThis inconsistency may lead to unexpected behavior and should be addressed to ensure that all
addSearchTermcalls adhere to the expected three-parameter structure.🔗 Analysis chain
Line range hint
1-78: Summary: Improved search term handling in CellActions componentThe changes made to the
CellActions.vuecomponent successfully enhance its functionality and flexibility in handling search terms. The modifications in both the template and script sections are consistent and well-implemented. Key improvements include:
- More granular control over search term addition in the template.
- Enhanced type safety and flexibility in the
addSearchTermfunction.- Better alignment between the template and script sections.
These changes contribute to a more robust and maintainable component, which should improve the overall user experience when working with log data tables.
To ensure that these changes are consistent with the rest of the codebase, please run the following verification script:
This script will help identify any potential inconsistencies in how the
addSearchTermevent or method is used across the project.
To address the issues encountered with the previous verification scripts, please run the following updated scripts:
[/run_scripts]
To further investigate the inconsistent usage of
addSearchTerm, please run the following scripts to identify the number of arguments in eachaddSearchTermcall:[/run_scripts]
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of addSearchTerm event across the codebase # Test: Search for components that listen to the addSearchTerm event # Expect: Components using this event should handle three parameters rg --type vue -A 5 '@addSearchTerm' # Test: Search for other occurrences of addSearchTerm method # Expect: No inconsistent usage of addSearchTerm method rg --type vue -A 5 'addSearchTerm'Length of output: 126
Script:
#!/bin/bash # Description: Verify the usage of addSearchTerm event across the codebase # Test: Search for components that listen to the addSearchTerm event # Expect: Components using this event should handle three parameters rg '@addSearchTerm' --glob '*.vue' -A 5 # Test: Search for other occurrences of addSearchTerm method # Expect: No inconsistent usage of addSearchTerm method rg 'addSearchTerm' --glob '*.vue' -A 5Length of output: 18918
Script:
#!/bin/bash # Description: Find all addSearchTerm calls and count the number of arguments # Extract all lines with addSearchTerm calls in .vue files rg 'addSearchTerm\s*\(' --glob '*.vue' > addSearchTerm_calls.txt # Count the number of arguments in each call while IFS= read -r line; do # Extract the part inside the parentheses args=$(echo "$line" | grep -oP '(?<=addSearchTerm\().*(?=\))') # Count the number of commas to determine the number of arguments if [ -z "$args" ]; then count=0 else count=$(echo "$args" | awk -F',' '{print NF}') fi echo "Arguments count in line: $count" done < addSearchTerm_calls.txtLength of output: 6190
web/src/plugins/logs/DetailTable.vue (2)
163-163: LGTM! Consider using constants for action strings.The changes to
toggleIncludeSearchTermandtoggleExcludeSearchTermmethod calls are correct and align with the updated method signatures. They now pass the field name, field value, and action type.Consider defining constants for the action strings "include" and "exclude" to improve maintainability and reduce the risk of typos. For example:
const INCLUDE_ACTION = 'include'; const EXCLUDE_ACTION = 'exclude'; // Then use them in the template @click="toggleIncludeSearchTerm(value, key, INCLUDE_ACTION)" @click="toggleExcludeSearchTerm(value, key, EXCLUDE_ACTION)"Also applies to: 193-193
389-395: LGTM! Consider unifying the method implementations.The updates to
toggleIncludeSearchTermandtoggleExcludeSearchTermmethods are well-implemented. They now usegetFilterExpressionByFieldTypeto construct search expressions based on field type, which should provide more accurate and flexible filtering.Since both methods have identical implementations except for the
actionparameter, consider unifying them into a single method to reduce code duplication:toggleSearchTerm(field: string|number, field_value: string|number|boolean, action: 'include' | 'exclude') { const searchExpression = getFilterExpressionByFieldType( field, field_value, action, ); this.$emit("add:searchterm", searchExpression); }Then update the template to use this unified method:
@click="toggleSearchTerm(value, key, 'include')" @click="toggleSearchTerm(value, key, 'exclude')"This approach would make the code more DRY and easier to maintain.
Also applies to: 397-403
web/src/plugins/logs/TenstackTable.vue (1)
Line range hint
1-621: Consider adding type information foraddSearchTermemitThe
addSearchTermemit is declared in theemitsarray, but it doesn't specify the structure of the emitted event. To improve code clarity and maintainability, consider adding a comment or using TypeScript to define the structure of the emitted event.You could add a comment like this:
const emits = defineEmits<{ // Other emits... (e: 'addSearchTerm', field: string, field_value: string | number | boolean, action: string): void // Other emits... }>()This will provide better type checking and auto-completion for components using this emit.
web/src/composables/useLogs.ts (1)
245-246: Consider using a more widely supported method for object property checks.The
selectedStreamFieldTypevariable is a good addition for caching field types. However, be aware thatObject.hasOwn()used in thegetFilterExpressionByFieldTypefunction is a relatively new method and might not be supported in all JavaScript environments.Consider using the more widely supported
Object.prototype.hasOwnProperty.call(obj, prop)orprop in objfor better compatibility:if (Object.prototype.hasOwnProperty.call(stream, "schema") && !Object.prototype.hasOwnProperty.call(selectedStreamFieldType, stream.name)) { // ... }web/src/plugins/logs/IndexList.vue (3)
440-442: Maintain Consistent Formatting in Function CallsIn the
addSearchTermfunction call, the arguments are formatted across multiple lines. For consistency and readability, ensure that similar function calls follow the same formatting style.
457-457: Maintain Consistent Formatting in Function CallsThe
addSearchTermfunction call here is formatted differently compared to the previous call. For improved readability, consider formatting the arguments across multiple lines, similar to the previous instance.Apply this diff to align the formatting:
- addSearchTerm( - props.row.name,value.key, 'exclude', - ) + addSearchTerm( + props.row.name, + value.key, + 'exclude', + )
1019-1019: Remove Unnecessary Commented CodeThe line
// searchObj.meta.showDetailTab = false;is commented out. If this code is no longer needed, consider removing it to keep the codebase clean and maintainable.Apply this diff to remove the commented line:
- // searchObj.meta.showDetailTab = false;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- web/src/composables/useLogs.ts (4 hunks)
- web/src/plugins/logs/DetailTable.vue (4 hunks)
- web/src/plugins/logs/IndexList.vue (4 hunks)
- web/src/plugins/logs/JsonPreview.vue (3 hunks)
- web/src/plugins/logs/TenstackTable.vue (1 hunks)
- web/src/plugins/logs/data-table/CellActions.vue (3 hunks)
🧰 Additional context used
🔇 Additional comments (11)
web/src/plugins/logs/data-table/CellActions.vue (1)
19-19: LGTM: Improved flexibility in search term handlingThe changes to the
@clickevent handlers for the include and exclude buttons are well-implemented. By passing separate arguments for the field, value, and action, the component now provides more granular control over search term addition. This change aligns well with the updatedaddSearchTermfunction signature and enhances the overall flexibility of the component.Also applies to: 30-30
web/src/plugins/logs/JsonPreview.vue (3)
144-144: LGTM: Template changes foraddSearchTermlook goodThe template has been updated correctly to use the new
addSearchTermmethod signature. The changes properly pass the field (key), field value (value[key]), and action ('include' or 'exclude') as parameters. This modification aligns well with the enhanced search term functionality.Also applies to: 172-172
311-316: LGTM:addSearchTermmethod updated correctlyThe
addSearchTermmethod has been properly updated to accept three parameters:field,field_value, andaction. This change aligns with the modifications in the template and enhances the search term functionality by allowing more specific control over include/exclude actions.
Line range hint
1-577: Summary: Enhanced search term functionality with minor queryThe changes in this file successfully implement a more granular search term functionality by updating the
addSearchTermmethod to include 'include' and 'exclude' actions. The modifications are consistent across both the template and script sections, improving the component's capability to handle more specific search queries.The implementation is clean and aligns well with the intended functionality. However, there's one minor query regarding an unused import (
getFilterExpressionByFieldType) that needs clarification or removal if not needed.Overall, these changes enhance the component's functionality and maintain good code quality.
web/src/plugins/logs/DetailTable.vue (2)
421-421: LGTM! Correct import of the new utility function.The addition of
getFilterExpressionByFieldTypeto the destructured import fromuseLogsis correct and necessary for the updated implementations of the search term toggle methods.
Line range hint
1-580: Overall, the changes improve search term handling and align with PR objectives.The modifications to
DetailTable.vueenhance the flexibility of search term handling by incorporating field types into the filter expression generation. The changes are well-implemented and consistent throughout the component.Key improvements:
- Updated method signatures for
toggleIncludeSearchTermandtoggleExcludeSearchTerm.- Integration of
getFilterExpressionByFieldTypefor type-specific filter expressions.- Consistent updates in both the template and script sections.
Consider the suggested minor improvements to further enhance code maintainability:
- Using constants for action strings in the template.
- Unifying the toggle methods to reduce code duplication.
These changes successfully address the PR objective of improving include/exclude search based on data type.
web/src/plugins/logs/TenstackTable.vue (3)
Line range hint
1-1000: Summary of changes and next stepsThe main change in this file is the update to the
addSearchTermfunction, which now accepts more specific parameters for improved search functionality. This change enhances the flexibility and precision of the search feature.To ensure the changes are fully integrated:
- Update parent components that listen to the "addSearchTerm" event.
- Review and update the
JsonPreviewandCellActionscomponents to use the newaddSearchTermsignature correctly.- Consider adding type information or comments for the
addSearchTermemit to improve code clarity.These changes represent a positive improvement to the search functionality, but require careful integration to maintain consistency across the application.
Line range hint
1-621: VerifyJsonPreviewandCellActionscomponentsThe
addSearchTermfunction is passed as a prop to both thejson-previewandcell-actionscomponents. Given the changes to theaddSearchTermfunction signature, these components may need to be updated to provide the correct parameters when invoking the function.Please review and update the
JsonPreviewandCellActionscomponents to ensure they calladdSearchTermwith the correct parameters (field,field_value, andaction). Run the following script to locate these components:#!/bin/bash # Description: Locate JsonPreview and CellActions components and check their usage of addSearchTerm # Find JsonPreview component echo "Checking JsonPreview component:" rg --type vue "export default.*JsonPreview" -l | xargs rg --type vue "addSearchTerm" -C 3 echo "\nChecking CellActions component:" # Find CellActions component rg --type vue "export default.*CellActions" -l | xargs rg --type vue "addSearchTerm" -C 3
622-627: ImprovedaddSearchTermfunction signatureThe
addSearchTermfunction has been updated to accept more specific parameters (field,field_value, andaction), which allows for more precise search term handling. This change enhances the flexibility and functionality of the search feature.Please ensure that any parent components listening to the "addSearchTerm" event are updated to handle these new parameters. Run the following script to check for potential usage:
✅ Verification successful
Please run the following corrected script to verify potential usages of the
addSearchTermevent in parent components:
Verification Successful
No existing parent components are using the
@addSearchTermevent. The updatedaddSearchTermfunction does not impact any current implementations.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find potential usages of addSearchTerm event in parent components # Search for components that import TenstackTable rg --type vue "import.*TenstackTable.*from" -l | while read -r file; do # Check if the file contains @addSearchTerm event listener if rg --type vue "@addSearchTerm" "$file" > /dev/null; then echo "Potential usage found in: $file" rg --type vue "@addSearchTerm" -C 3 "$file" fi doneLength of output: 584
Script:
#!/bin/bash # Description: Find potential usages of addSearchTerm event in parent components # Search for .vue files that import TenstackTable fd -e vue . | while read -r file; do if rg "@addSearchTerm" "$file" > /dev/null; then echo "Potential usage found in: $file" rg "@addSearchTerm" -C 3 "$file" fi doneLength of output: 22251
web/src/composables/useLogs.ts (1)
4288-4288: LGTM: ExportedgetFilterExpressionByFieldTypefromuseLogs.The addition of
getFilterExpressionByFieldTypeto the return object ofuseLogsis appropriate. This makes the new function available for use in other parts of the application that importuseLogs, allowing for wider use of the new filtering functionality.web/src/plugins/logs/IndexList.vue (1)
705-705: ImportinggetFilterExpressionByFieldTypeGood job on importing
getFilterExpressionByFieldTypefromuseLogs(). This enhances modularity and reusability by utilizing a shared function for generating filter expressions based on field types.
web/src/plugins/logs/JsonPreview.vue
Outdated
| emit("addFieldToTable", value); | ||
| }; | ||
| const { searchObj,searchAggData } = useLogs(); | ||
| const { searchObj,searchAggData, getFilterExpressionByFieldType } = useLogs(); |
There was a problem hiding this comment.
💡 Codebase verification
Unused import getFilterExpressionByFieldType found
The getFilterExpressionByFieldType function is imported from useLogs but is not used in JsonPreview.vue. Please remove the unused import to maintain clean and efficient code.
🔗 Analysis chain
Query: Unused import getFilterExpressionByFieldType
The getFilterExpressionByFieldType function is imported from the useLogs composable but doesn't appear to be used within the visible part of the script. Could you clarify the intended use of this imported function or remove it if it's not needed?
To check for any usage of getFilterExpressionByFieldType in the entire file, you can run:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
rg "getFilterExpressionByFieldType" web/src/plugins/logs/JsonPreview.vue
Length of output: 155
| const getFilterExpressionByFieldType = ( | ||
| field: string, | ||
| field_value: string|number|boolean, | ||
| action: string, | ||
| ) => { | ||
| let operator = action == "include" ? "=" : "!="; | ||
| try { | ||
| let fieldTypeList: any = {}; | ||
| let fieldType: string = "utf8"; | ||
| searchObj.data.streamResults.list.forEach((stream: any) => { | ||
| if (searchObj.data.stream.selectedStream.includes(stream.name)) { | ||
| if ( | ||
| Object.hasOwn(stream, "schema") && | ||
| !Object.hasOwn(selectedStreamFieldType, stream.name) | ||
| ) { | ||
| selectedStreamFieldType[stream.name] = {}; | ||
| stream.schema.forEach((schema: any) => { | ||
| selectedStreamFieldType[stream.name][schema.name] = schema.type; | ||
| }); | ||
| fieldTypeList = { | ||
| ...fieldTypeList, | ||
| ...selectedStreamFieldType[stream.name], | ||
| }; | ||
| } else { | ||
| if (Object.hasOwn(selectedStreamFieldType, stream.name)) { | ||
| fieldTypeList = { | ||
| ...fieldTypeList, | ||
| ...selectedStreamFieldType[stream.name], | ||
| }; | ||
| } | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| if (Object.hasOwn(fieldTypeList, field)) { | ||
| fieldType = fieldTypeList[field]; | ||
| } | ||
|
|
||
| if (field_value == "null" || field_value == "NULL" || field_value == "") { | ||
| operator = action == "include" ? "is" : "is not"; | ||
| field_value = "null"; | ||
| } | ||
| let expression = field_value == "null" ? `${field} ${operator} ${field_value}` : `${field} ${operator} '${field_value}'`; | ||
|
|
||
| if ( | ||
| fieldType.toLowerCase() == "int64" || | ||
| fieldType.toLowerCase() == "float64" | ||
| ) { | ||
| expression = `${field} ${operator} ${field_value}`; | ||
| } else if (fieldType.toLowerCase() == "boolean") { | ||
| operator = action == "include" ? "is" : "is not"; | ||
| expression = `${field} ${operator} ${field_value}`; | ||
| } | ||
|
|
||
| return expression; | ||
| } catch (e: any) { | ||
| console.log("Error while getting filter expression by field type", e); | ||
| return `${field} ${operator} '${field_value}'`; | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and consider potential side effects in getFilterExpressionByFieldType.
The getFilterExpressionByFieldType function is a valuable addition for constructing type-specific filter expressions. However, there are a few areas that could be improved:
-
Error handling: The current catch-all error handling might mask specific issues. Consider adding more granular error handling.
-
Global state mutation: The function mutates the global
selectedStreamFieldTypeobject, which could lead to unexpected behavior in concurrent scenarios. -
Type handling: The function uses string comparison for type checking, which might be case-sensitive and error-prone.
Consider the following improvements:
- Enhance error handling:
try {
// ... existing code ...
} catch (e: any) {
console.error("Error in getFilterExpressionByFieldType:", e.message);
// Consider throwing a custom error or returning a specific error value
throw new Error(`Failed to generate filter expression: ${e.message}`);
}- Avoid mutating global state:
const getStreamFieldTypes = (stream: any) => {
if (!stream.schema) return {};
return Object.fromEntries(stream.schema.map((schema: any) => [schema.name, schema.type]));
};
const fieldTypeList = searchObj.data.streamResults.list
.filter((stream: any) => searchObj.data.stream.selectedStream.includes(stream.name))
.reduce((acc: any, stream: any) => ({...acc, ...getStreamFieldTypes(stream)}), {});- Use a more robust type checking mechanism:
const isNumericType = (type: string) => ['int64', 'float64'].includes(type.toLowerCase());
const isBooleanType = (type: string) => type.toLowerCase() === 'boolean';
// Then in your switch statement:
if (isNumericType(fieldType)) {
expression = `${field} ${operator} ${field_value}`;
} else if (isBooleanType(fieldType)) {
// ... existing boolean handling ...
}These changes will make the function more robust, easier to maintain, and less prone to unexpected behavior.
There was a problem hiding this comment.
suggestion considered and implemented
There was a problem hiding this comment.
@bjp232004, glad to hear that you've considered and implemented the suggestion!
(\ (\
( ^.^)
o_(")(")
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
web/src/plugins/logs/IndexList.vue
Outdated
| const addSearchTerm = ( | ||
| field: string, | ||
| field_value: string | number | boolean, | ||
| action: string, | ||
| ) => { | ||
| // searchObj.meta.showDetailTab = false; | ||
| searchObj.data.stream.addToFilter = term; | ||
| const expression = getFilterExpressionByFieldType( | ||
| field, | ||
| field_value, | ||
| action, | ||
| ); | ||
| searchObj.data.stream.addToFilter = expression; |
There was a problem hiding this comment.
Add Error Handling in addSearchTerm Function
Consider adding error handling to the addSearchTerm function to manage cases where getFilterExpressionByFieldType might return an undefined value or throw an error. This will enhance the robustness of the function.
You can apply this diff to include basic error handling:
const addSearchTerm = (
field: string,
field_value: string | number | boolean,
action: string,
) => {
- // searchObj.meta.showDetailTab = false;
const expression = getFilterExpressionByFieldType(
field,
field_value,
action,
);
+ if (expression) {
searchObj.data.stream.addToFilter = expression;
+ } else {
+ console.error('Failed to generate filter expression');
+ }
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const addSearchTerm = ( | |
| field: string, | |
| field_value: string | number | boolean, | |
| action: string, | |
| ) => { | |
| // searchObj.meta.showDetailTab = false; | |
| searchObj.data.stream.addToFilter = term; | |
| const expression = getFilterExpressionByFieldType( | |
| field, | |
| field_value, | |
| action, | |
| ); | |
| searchObj.data.stream.addToFilter = expression; | |
| const addSearchTerm = ( | |
| field: string, | |
| field_value: string | number | boolean, | |
| action: string, | |
| ) => { | |
| const expression = getFilterExpressionByFieldType( | |
| field, | |
| field_value, | |
| action, | |
| ); | |
| if (expression) { | |
| searchObj.data.stream.addToFilter = expression; | |
| } else { | |
| console.error('Failed to generate filter expression'); | |
| } | |
| }; |
There was a problem hiding this comment.
considered and fixed
There was a problem hiding this comment.
@bjp232004 Thank you for addressing the issue! 🐰🎉
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
web/src/plugins/logs/IndexList.vue (1)
1015-1030: Improved search term handling with type-specific filteringThe
addSearchTermfunction has been refactored to use a newgetFilterExpressionByFieldTypefunction, which should provide more accurate filtering based on the field type. This is a good improvement for type-specific search capabilities.However, there are a couple of points to consider:
- The error handling could be more informative.
- The function doesn't handle the case where
expressionis an empty string.Consider enhancing the error handling and empty string case:
const addSearchTerm = ( field: string, field_value: string | number | boolean, action: string, ) => { const expression = getFilterExpressionByFieldType( field, field_value, action, ); - if (expression) { + if (expression && expression.trim() !== '') { searchObj.data.stream.addToFilter = expression; } else { - console.error("Failed to generate filter expression"); + console.error(`Failed to generate filter expression for field: ${field}, value: ${field_value}, action: ${action}`); + // Optionally, you could notify the user here } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- web/src/composables/useLogs.ts (4 hunks)
- web/src/plugins/logs/IndexList.vue (4 hunks)
- web/src/plugins/logs/JsonPreview.vue (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- web/src/composables/useLogs.ts
- web/src/plugins/logs/JsonPreview.vue
🧰 Additional context used
🔇 Additional comments (3)
web/src/plugins/logs/IndexList.vue (3)
Line range hint
1-1452: Summary of changes and suggestionsThe changes in this file primarily focus on improving the search functionality by introducing type-specific filtering through the
getFilterExpressionByFieldTypefunction. This is a positive enhancement that should provide more accurate search results.Key points and suggestions:
- The
addSearchTermfunction has been refactored to use the new filtering method.- Error handling in
addSearchTermcould be improved to be more informative and handle empty expressions.- Usage of
addSearchTermin the template seems correct, but parameter types should be verified.- The implementation of
getFilterExpressionByFieldTypein theuseLogscomposable should be reviewed to ensure it handles all necessary field types correctly.Overall, these changes appear to be a step in the right direction for improving search capabilities. Implementing the suggested refinements will further enhance the robustness of the search functionality.
705-705: Verify implementation of getFilterExpressionByFieldTypeThe
getFilterExpressionByFieldTypefunction is imported from theuseLogscomposable. Ensure that this function is properly implemented to handle different field types and generate appropriate filter expressions.To verify the implementation of
getFilterExpressionByFieldType, run the following script:#!/bin/bash # Description: Check the implementation of getFilterExpressionByFieldType in useLogs composable # Test: Look for the getFilterExpressionByFieldType function definition echo "Searching for getFilterExpressionByFieldType implementation:" rg -A 20 'const getFilterExpressionByFieldType = ' src/composables/useLogs.ts # Note: Adjust the file path if necessaryThis will help ensure that the function is correctly implemented and handles various field types appropriately.
440-442: Verify parameters passed to addSearchTermThe
addSearchTermfunction is correctly used in the template with the updated parameter list. However, ensure thatprops.row.name,value.key, and the action ('include' or 'exclude') are the correct types expected by the function.To confirm the types of the passed parameters, you can run the following script:
Also applies to: 457-457
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
src/common/infra/wal.rs (2)
410-410: LGTM. Consider adding more context to the log message.The addition of this log statement improves traceability for WAL file lock requests.
Consider including the number of files being locked in the log message for additional context:
log::info!("[trace_id {}] lock_request for {} wal files", trace_id, files.len());
Line range hint
419-419: LGTM. Consider adding more context and ensuring consistency with lock_request.The addition of this log statement improves traceability for WAL file lock release requests.
For consistency with the
lock_requestfunction and to provide more context, consider modifying the log message as follows:let files = locker.remove(trace_id); log::info!("[trace_id {}] release_request for {} wal files", trace_id, files.as_ref().map_or(0, |f| f.len()));This change adds the number of files being released to the log message, providing more context and maintaining consistency with the
lock_requestlogging style.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- src/common/infra/wal.rs (1 hunks)
- src/job/files/parquet.rs (3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/common/infra/wal.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/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 suggestionsMake sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
🔇 Additional comments (2)
src/common/infra/wal.rs (1)
410-410: Overall improvements in WAL operation loggingThe changes in this file enhance the traceability of WAL (Write-Ahead Logging) file locking operations by adding logging statements to both
lock_requestandrelease_requestfunctions. These additions will significantly improve debugging and monitoring capabilities without altering the core functionality of the WAL system.Also applies to: 419-419
src/job/files/parquet.rs (1)
90-91: Appropriate addition ofSKIPPED_LOCK_FILESThe static variable
SKIPPED_LOCK_FILESis correctly declared to track files that are locked and should be skipped during processing.
| let mut need_skip = true; | ||
| // wait for 5s | ||
| for _ in 0..50 { | ||
| if wal::lock_files_exists(&file.key) { | ||
| log::warn!( | ||
| "[INGESTER:JOB:{thread_id}] the file is still in use, waiting for a few ms: {}", | ||
| file.key | ||
| ); | ||
| time::sleep(time::Duration::from_millis(100)).await; | ||
| } else { | ||
| need_skip = false; | ||
| break; | ||
| } | ||
| } | ||
| if need_skip { | ||
| log::warn!( | ||
| "[INGESTER:JOB:{thread_id}] the file is still in use, add it to the skip_list: {}", | ||
| file.key | ||
| ); | ||
| SKIPPED_LOCK_FILES.write().await.insert(file.key.clone()); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using tokio::time::timeout for waiting
Instead of manually looping and sleeping to wait for the file to be released, you could use tokio::time::timeout to simplify the code and improve readability.
Apply this diff to refactor the waiting mechanism:
-let mut need_skip = true;
-// wait for 5s
-for _ in 0..50 {
- if wal::lock_files_exists(&file.key) {
- log::warn!(
- "[INGESTER:JOB:{thread_id}] the file is still in use, waiting for a few ms: {}",
- file.key
- );
- time::sleep(time::Duration::from_millis(100)).await;
- } else {
- need_skip = false;
- break;
- }
-}
-if need_skip {
+if let Err(_) = tokio::time::timeout(time::Duration::from_secs(5), async {
+ while wal::lock_files_exists(&file.key) {
+ log::warn!(
+ "[INGESTER:JOB:{thread_id}] the file is still in use, waiting: {}",
+ file.key
+ );
+ time::sleep(time::Duration::from_millis(100)).await;
+ }
+})
+.await
{
log::warn!(
"[INGESTER:JOB:{thread_id}] the file is still in use, add it to the skip_list: {}",
file.key
);
SKIPPED_LOCK_FILES.write().await.insert(file.key.clone());
continue;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let mut need_skip = true; | |
| // wait for 5s | |
| for _ in 0..50 { | |
| if wal::lock_files_exists(&file.key) { | |
| log::warn!( | |
| "[INGESTER:JOB:{thread_id}] the file is still in use, waiting for a few ms: {}", | |
| file.key | |
| ); | |
| time::sleep(time::Duration::from_millis(100)).await; | |
| } else { | |
| need_skip = false; | |
| break; | |
| } | |
| } | |
| if need_skip { | |
| log::warn!( | |
| "[INGESTER:JOB:{thread_id}] the file is still in use, add it to the skip_list: {}", | |
| file.key | |
| ); | |
| SKIPPED_LOCK_FILES.write().await.insert(file.key.clone()); | |
| continue; | |
| } | |
| if let Err(_) = tokio::time::timeout(time::Duration::from_secs(5), async { | |
| while wal::lock_files_exists(&file.key) { | |
| log::warn!( | |
| "[INGESTER:JOB:{thread_id}] the file is still in use, waiting: {}", | |
| file.key | |
| ); | |
| time::sleep(time::Duration::from_millis(100)).await; | |
| } | |
| }) | |
| .await | |
| { | |
| log::warn!( | |
| "[INGESTER:JOB:{thread_id}] the file is still in use, add it to the skip_list: {}", | |
| file.key | |
| ); | |
| SKIPPED_LOCK_FILES.write().await.insert(file.key.clone()); | |
| continue; | |
| } |
| // check if the file is still locking | ||
| if SKIPPED_LOCK_FILES.read().await.contains(&file_key) | ||
| && !wal::lock_files_exists(&file_key) | ||
| { | ||
| log::warn!( | ||
| "[INGESTER:JOB] the file was released, delete it: {}", | ||
| file_key | ||
| ); | ||
| if tokio::fs::remove_file(&wal_dir.join(&file_key)) | ||
| .await | ||
| .is_ok() | ||
| { | ||
| // delete metadata from cache | ||
| WAL_PARQUET_METADATA.write().await.remove(&file_key); | ||
| // need release all the files | ||
| PROCESSING_FILES.write().await.remove(&file_key); | ||
| // delete from skip list | ||
| SKIPPED_LOCK_FILES.write().await.remove(&file_key); | ||
| } | ||
| } |
There was a problem hiding this comment.
Handle potential errors when removing files
When attempting to remove the file with tokio::fs::remove_file, consider handling the Err case to log any errors that might occur during file deletion. This ensures that any issues are not silently ignored.
Apply this diff to handle the error:
if tokio::fs::remove_file(&wal_dir.join(&file_key))
.await
.is_ok()
{
// existing code...
} else {
log::error!(
"[INGESTER:JOB] Failed to remove file: {}",
file_key
);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // check if the file is still locking | |
| if SKIPPED_LOCK_FILES.read().await.contains(&file_key) | |
| && !wal::lock_files_exists(&file_key) | |
| { | |
| log::warn!( | |
| "[INGESTER:JOB] the file was released, delete it: {}", | |
| file_key | |
| ); | |
| if tokio::fs::remove_file(&wal_dir.join(&file_key)) | |
| .await | |
| .is_ok() | |
| { | |
| // delete metadata from cache | |
| WAL_PARQUET_METADATA.write().await.remove(&file_key); | |
| // need release all the files | |
| PROCESSING_FILES.write().await.remove(&file_key); | |
| // delete from skip list | |
| SKIPPED_LOCK_FILES.write().await.remove(&file_key); | |
| } | |
| } | |
| // check if the file is still locking | |
| if SKIPPED_LOCK_FILES.read().await.contains(&file_key) | |
| && !wal::lock_files_exists(&file_key) | |
| { | |
| log::warn!( | |
| "[INGESTER:JOB] the file was released, delete it: {}", | |
| file_key | |
| ); | |
| if tokio::fs::remove_file(&wal_dir.join(&file_key)) | |
| .await | |
| .is_ok() | |
| { | |
| // delete metadata from cache | |
| WAL_PARQUET_METADATA.write().await.remove(&file_key); | |
| // need release all the files | |
| PROCESSING_FILES.write().await.remove(&file_key); | |
| // delete from skip list | |
| SKIPPED_LOCK_FILES.write().await.remove(&file_key); | |
| } else { | |
| log::error!( | |
| "[INGESTER:JOB] Failed to remove file: {}", | |
| file_key | |
| ); | |
| } | |
| } |
bjp232004
force-pushed
the
4033-interacting-with-boolean-data-causes-queries-to-become-unresponsive
branch
from
aacc2e3 to
9c7a6b0
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
web/src/composables/useLogs.ts (2)
2709-2709: Remove debugging console.log statement.This console.log statement appears to be a debugging artifact. It's generally a good practice to remove such statements before committing code to production.
- console.log("stream", stream)
4236-4297: Approve implementation with suggestions for improvement.The
getFilterExpressionByFieldTypefunction is a valuable addition for constructing type-specific filter expressions. Here are some suggestions for improvement:
- Consider using a type union for the
field_valueparameter instead of separate types.- The
fieldTypevariable could be initialized with a more specific type.- Consider using a switch statement for better readability when handling different field types.
Here's a refactored version incorporating these suggestions:
const getFilterExpressionByFieldType = ( field: string | number, field_value: string | number | boolean | null, action: 'include' | 'exclude' ): string => { let operator = action === 'include' ? '=' : '!='; try { let fieldType: 'utf8' | 'int64' | 'float64' | 'boolean' = 'utf8'; // ... (rest of the code remains the same until the type checking) switch (true) { case field_value === null || field_value === '': operator = action === 'include' ? 'is' : 'is not'; return `${field} ${operator} null`; case isNumericType(fieldType): return `${field} ${operator} ${field_value}`; case isBooleanType(fieldType): operator = action === 'include' ? 'is' : 'is not'; return `${field} ${operator} ${field_value}`; default: return `${field} ${operator} '${field_value}'`; } } catch (e: any) { console.error("Error in getFilterExpressionByFieldType:", e.message); return `${field} ${operator} '${field_value}'`; } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- web/src/composables/useLogs.ts (4 hunks)
- web/src/plugins/logs/DetailTable.vue (3 hunks)
- web/src/plugins/logs/IndexList.vue (4 hunks)
- web/src/plugins/logs/JsonPreview.vue (3 hunks)
- web/src/plugins/logs/SearchResult.vue (2 hunks)
- web/src/plugins/logs/TenstackTable.vue (1 hunks)
- web/src/plugins/logs/data-table/CellActions.vue (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- web/src/plugins/logs/DetailTable.vue
- web/src/plugins/logs/JsonPreview.vue
- web/src/plugins/logs/data-table/CellActions.vue
🧰 Additional context used
🔇 Additional comments (8)
web/src/plugins/logs/IndexList.vue (3)
440-442: Improved search term addition flexibilityThe
addSearchTermmethod has been updated to accept three parameters (field, value, and action) instead of a single term. This change enhances the flexibility and precision of the search functionality, allowing for more specific include/exclude operations.Also applies to: 457-457
1015-1040: Enhanced addSearchTerm function with improved type handling and error managementThe
addSearchTermfunction has been significantly improved:
- It now uses
getFilterExpressionByFieldTypeto generate filter expressions based on the field type, improving type safety and flexibility.- Error handling has been added to notify users when filter expression generation fails.
These changes enhance the robustness and user-friendliness of the search functionality.
705-705: Improved code organization with getFilterExpressionByFieldTypeThe
getFilterExpressionByFieldTypefunction has been added to the imported functions from theuseLogscomposable. This change improves code organization by centralizing the filter expression generation logic, making it reusable across different components.web/src/composables/useLogs.ts (1)
4340-4340: Approve addition ofgetFilterExpressionByFieldTypeto the return object.The inclusion of
getFilterExpressionByFieldTypein the return object of theuseLogscomposable is correct. This makes the new function accessible to components that use this composable, allowing them to generate filter expressions based on field types.web/src/plugins/logs/TenstackTable.vue (1)
623-628: LGTM!The
addSearchTermfunction has been updated correctly to accept the new parameters and emit the event accordingly.web/src/plugins/logs/SearchResult.vue (3)
384-384: ImportgetFilterExpressionByFieldTypeis properly addedThe function
getFilterExpressionByFieldTypeis now imported fromuseLogs(). This ensures that it can be used within this component.
Line range hint
579-586: Ensure consistency in method calls withinTenstackTablecomponentIn the
<tenstack-table>component, there is an event listener for@add-search-term="addSearchTerm". Verify that the emitted event provides the necessary parameters toaddSearchTermbased on its new signature.Run the following script to locate and review the event emissions for
add-search-term:#!/bin/bash # Description: Find all emissions of the `add-search-term` event and display their arguments. rg "@add-search-term" -A 5 -B 5
Line range hint
523-535: Update event emission in<DetailTable>componentIn the
<DetailTable>component within the template, the event names have been updated to match the newaddSearchTermfunction signature:@add:searchterm="addSearchTerm" @remove:searchterm="removeSearchTerm"Ensure that these events emit the correct parameters (
field,field_value,action) to align with the updatedaddSearchTermfunction.Run the following script to inspect the event emissions related to
add:searchterm:#!/bin/bash # Description: Find all emissions of the `add:searchterm` event and display their arguments. rg "@add:searchterm" -A 5 -B 5
web/src/composables/useLogs.ts
Outdated
| const selectedStreamFieldType:any = {}; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a more specific type for selectedStreamFieldType.
Instead of using any, consider defining a more specific type or interface for selectedStreamFieldType. This would improve type safety and make the code more self-documenting.
interface StreamFieldTypes {
[fieldName: string]: string;
}
const selectedStreamFieldType: StreamFieldTypes = {};There was a problem hiding this comment.
@bjp232004, understood. Since selectedStreamFieldType has been removed, the previous suggestion is no longer applicable.
(ಥ﹏ಥ)
✏️ Learnings added
Learnt from: bjp232004
PR: openobserve/openobserve#4841
File: web/src/composables/useLogs.ts:0-0
Timestamp: 2024-10-21T11:15:45.924Z
Learning: In `web/src/composables/useLogs.ts`, the variable `selectedStreamFieldType` has been removed.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| const addSearchTerm = ( | ||
| field: string | number, | ||
| field_value: string | number | boolean, | ||
| action: string, | ||
| ) => { | ||
| const searchExpression = getFilterExpressionByFieldType( | ||
| field, | ||
| field_value, | ||
| action, | ||
| ); | ||
| searchObj.data.stream.addToFilter = searchExpression; |
There was a problem hiding this comment.
💡 Codebase verification
Verification of addSearchTerm Updates Needed
Several calls to addSearchTerm do not match the updated function signature and need to be reviewed:
-
BasicValuesFilter.vue-
addSearchTerm(${row.name}='${value.key}') -
addSearchTerm(${row.name}!='${value.key}')
-
-
FieldList.vue-
addSearchTerm(${props.row.name}='') -
addSearchTerm(${props.row.name}='${value.key}') -
addSearchTerm(${props.row.name}!='${value.key}')
-
-
SearchBar.vueandrum/SearchBar.vue-
addSearchTermis defined as a method and may require alignment with the new signature.
-
Please update these instances to match the new addSearchTerm function signature.
🔗 Analysis chain
Update addSearchTerm function signature and implementation
The addSearchTerm function has been updated to accept field, field_value, and action parameters. It now constructs the search expression using getFilterExpressionByFieldType and assigns it to searchObj.data.stream.addToFilter. This implementation appears correct.
However, please verify that all calls to addSearchTerm within this component and other components have been updated to match the new signature.
Run the following script to find usages of addSearchTerm and check their parameters:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all calls to `addSearchTerm` and display their parameter lists.
# Search for all occurrences of `addSearchTerm(` and display the lines with context.
rg --glob '!SearchResult.vue' 'addSearchTerm\(' -A 1 -B 1
Length of output: 1124
Script:
#!/bin/bash
# Description: Correctly find all calls to `addSearchTerm` and display their parameter lists, excluding SearchResult.vue.
rg --glob '!web/src/plugins/logs/SearchResult.vue' 'addSearchTerm\(' -A 1 -B 1
Length of output: 4830
Script:
#!/bin/bash
# Description: Find all calls to `addSearchTerm` and display their parameter lists, excluding SearchResult.vue.
rg --glob '!web/src/plugins/logs/SearchResult.vue' 'addSearchTerm\(' -A 1 -B 1
Length of output: 4830
There was a problem hiding this comment.
This is not required.
There was a problem hiding this comment.
@bjp232004, understood. Thank you for the clarification.
(；へ：)
✏️ Learnings added
Learnt from: bjp232004
PR: openobserve/openobserve#4841
File: web/src/plugins/logs/SearchResult.vue:445-455
Timestamp: 2024-10-21T11:16:45.535Z
Learning: In the OpenObserve project, changes to the `addSearchTerm` function's signature in `web/src/plugins/logs/SearchResult.vue` do not require updating its usage in other components such as `BasicValuesFilter.vue`, `FieldList.vue`, or `SearchBar.vue`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
web/src/composables/useLogs.ts (1)
4234-4295: New functiongetFilterExpressionByFieldTypeaddedA new function
getFilterExpressionByFieldTypehas been implemented to generate filter expressions based on field types. This function enhances the flexibility of query building by considering the data type of fields when constructing filter expressions.Key points:
- The function handles different data types (numeric, boolean, and string).
- It includes special handling for null values.
- It dynamically determines the field type based on the stream schema.
- The function uses a more robust type checking mechanism with
isNumericTypeandisBooleanTypehelper functions.Suggestions for improvement:
- Consider adding input validation for the
actionparameter to ensure it's either "include" or "exclude".- The error handling could be more specific, logging the exact nature of the error rather than a generic message.
Consider the following improvements:
- Add input validation:
if (action !== "include" && action !== "exclude") { throw new Error("Invalid action. Must be either 'include' or 'exclude'."); }
- Enhance error logging:
} catch (e: any) { console.error("Error in getFilterExpressionByFieldType:", e.message); console.error("Inputs:", { field, field_value, action }); return `${field} ${operator} '${field_value}'`; }These changes will make the function more robust and easier to debug.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- web/src/composables/useLogs.ts (3 hunks)
- web/src/plugins/logs/DetailTable.vue (3 hunks)
- web/src/plugins/logs/IndexList.vue (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- web/src/plugins/logs/DetailTable.vue
🧰 Additional context used
📓 Learnings (1)
web/src/plugins/logs/IndexList.vue (1)
Learnt from: bjp232004 PR: openobserve/openobserve#4841 File: web/src/plugins/logs/SearchResult.vue:445-455 Timestamp: 2024-10-21T11:16:45.923Z Learning: In the OpenObserve project, changes to the `addSearchTerm` function's signature in `web/src/plugins/logs/SearchResult.vue` do not require updating its usage in other components such as `BasicValuesFilter.vue`, `FieldList.vue`, or `SearchBar.vue`.
🔇 Additional comments (4)
web/src/plugins/logs/IndexList.vue (2)
1015-1034: ImprovedaddSearchTermfunction with better flexibility and error handling.The changes to the
addSearchTermfunction are well-implemented:
- The new function signature with
field,field_value, andactionparameters allows for more precise and flexible search term addition.- The use of
getFilterExpressionByFieldTypesuggests a more type-aware approach to generating filter expressions.- The added error handling with user notification improves the robustness of the function.
These changes should lead to a more reliable and user-friendly search experience.
440-442: Correct usage of the updatedaddSearchTermfunction.The calls to
addSearchTermhave been properly updated to match its new signature:
- Line 440-442:
addSearchTerm(props.row.name, value.key, 'include')- Line 457:
addSearchTerm(props.row.name,value.key, 'exclude')These changes correctly implement the new function signature, passing the field name, value, and action (include/exclude) as separate parameters.
Also applies to: 457-457
web/src/composables/useLogs.ts (2)
4338-4338: Updated return statement to include new functionThe
getFilterExpressionByFieldTypefunction has been added to the return statement of theuseLogscomposable. This makes the function available for use by components that import this composable.This change is correct and necessary to expose the new functionality.
Line range hint
1-4339: Summary of changesThe main modification to this file is the addition of the
getFilterExpressionByFieldTypefunction, which enhances the log querying capabilities by generating type-specific filter expressions. This function has been properly integrated into theuseLogscomposable by including it in the return statement.The implementation is solid, with room for minor improvements in error handling and input validation. Overall, these changes should positively impact the flexibility and accuracy of log filtering operations in the application.
bjp232004
force-pushed
the
4033-interacting-with-boolean-data-causes-queries-to-become-unresponsive
branch
from
3e911f4 to
7d99dfa
Compare
bjp232004
deleted the
4033-interacting-with-boolean-data-causes-queries-to-become-unresponsive
branch
2 participants
fixed #4788 #4033
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes