web/src/plugins/logs/Index.vue (7)

Line range hint 636-643: Correct Watcher Callback Parameters

In the watch function for router.currentRoute.value.query.type, the callback parameters might be mismatched. The prev parameter should correspond to the previous value of router.currentRoute.value.query.type, not the entire previous route object.

Update the callback parameters for clarity and correctness:

watch(
  () => router.currentRoute.value.query.type,
-  (type, prev) => {
+  (type, prevType) => {
    // Use prevType instead of prev
    if (
      searchObj.shouldIgnoreWatcher == false &&
      router.currentRoute.value.name === "logs" &&
-     prev === "stream_explorer" &&
+     prevType === "stream_explorer" &&
      !type
    ) {
      searchObj.meta.pageType = "logs";
      loadLogsData();
    }
  },
);

---

Line range hint 650-656: Simplify Watcher Logic for Query Changes

The watcher on router.currentRoute.value.query can be simplified by directly checking the action property without using hasOwnProperty.

Refactor the watcher for better readability:

watch(
  () => router.currentRoute.value.query,
  () => {
-   if (!router.currentRoute.value.query.hasOwnProperty("action")) {
+   if (!router.currentRoute.value.query.action) {
      showSearchHistory.value = false;
    }
-   if (
-     router.currentRoute.value.query.hasOwnProperty("action") &&
-     router.currentRoute.value.query.action == "history"
-   ) {
+   if (router.currentRoute.value.query.action == "history") {
      showSearchHistory.value = true;
    }
  },
);

---

681-683: Maintain Consistent Code Formatting

There's an extra closing brace and parenthesis that may not be necessary. Ensure the watcher function is properly closed.

Double-check the closing of the watcher function to maintain code consistency.

---

778-778: Enhance Type Safety with Explicit Types

Currently, (item: any) is used in the map function, which can be improved for type safety.

Specify a more precise type if possible:

const streamFieldNames: any =
  searchObj.data.stream.selectedStreamFields.map(
-   (item: any) => item.name,
+   (item: StreamField) => item.name,
  );

Make sure to import or define the StreamField interface accordingly.

---

891-891: Update Function Signature Documentation

With the addition of the isFieldExistInSQL parameter in setInterestingFieldInSQLQuery, update the function's documentation to reflect this change.

Add parameter details to the function comment:

/**
 * Adds or removes an interesting field from the SQL query.
 * @param {any} field - The field to add or remove.
 * @param {boolean} isFieldExistInSQL - Indicates if the field already exists in the SQL query.
 */
const setInterestingFieldInSQLQuery = (field: any, isFieldExistInSQL: boolean) => { /* ... */ }

---

962-965: Avoid Redundant Function Calls

Inside handleQuickModeChange, calling setQuery(searchObj.meta.quickMode); may be unnecessary if searchObj.meta.quickMode hasn't changed.

Verify if this function call is needed to prevent redundant operations.

---

Line range hint 1293-1309: Avoid Potential Race Conditions in Asynchronous Watcher

In the watcher for showHistogram, the use of asynchronous functions like await this.generateHistogramSkeleton(); inside a watcher might lead to unexpected behavior.

Consider refactoring the code to manage asynchronous operations more predictably:

watch(
  () => this.searchObj.meta.showHistogram,
  async (newVal) => {
    if (newVal && !this.searchObj.shouldIgnoreWatcher) {
      // Existing logic
    }
  }
);

Alternatively, use nextTick to ensure the DOM is updated before proceeding.

web/src/composables/useLogs.ts (5)

Line range hint 2486-2504: Use 'forEach' instead of 'map' when not transforming array

The .map() function is used here to iterate over searchObj.data.queryResults.partitionDetail.partitions without returning any transformed array. Since the callback does not return a value and the result of .map() is not utilized, it's more appropriate to use .forEach() for side-effect operations.

Apply this diff to fix the issue:

- searchObj.data.queryResults.partitionDetail.partitions.map(
+ searchObj.data.queryResults.partitionDetail.partitions.forEach(
  (item: any, index: any) => {
    if (item[0] == currentStartTime && item[1] == currentEndTime) {
      totalHits = res.data.hits.reduce(
        (accumulator: number, currentValue: any) =>
          accumulator +
          Math.max(parseInt(currentValue.zo_sql_num, 10), 0),
        0
      );

      searchObj.data.queryResults.partitionDetail.partitionTotal[
        index
      ] = totalHits;

      return;
    }
  }
);

---

Line range hint 2194-2226: Avoid reassigning a 'const' variable

In generateHistogramSkeleton(), the variable date is declared using const but is reassigned within a loop. Reassigning a const variable will cause a runtime error. Declare date with let if it needs to be reassigned.

Apply this diff to fix the issue:

- const date = new Date();
+ let date = new Date();

---

Line range hint 2524-2548: Handle HTTP 429 errors in 'catch' block

In the error handling for the histogram query, HTTP 429 (Too Many Requests) responses are not being handled. The current logic skips error handling when err?.request?.status == 429, which might cause uncaught exceptions or user confusion when rate limits are reached. Consider adding appropriate handling for HTTP 429 errors.

Apply this diff to handle HTTP 429 errors:

  if (err?.request?.status != 429) {
    // Existing error handling
    ...
  }
+ else {
+   searchObj.data.histogram.errorMsg = err.response.data.error || 'Too Many Requests';
+   notificationMsg.value = searchObj.data.histogram.errorMsg;
+ }

---

Line range hint 1822-1836: Possible infinite loop in refreshPartitionPagination

In the refreshPartitionPagination function, there is a for loop that iterates over partitionDetail.partitions. Inside the loop, there is a conditional return true; statement. Using return inside a loop might not behave as intended, as it will exit the entire function on the first iteration that meets the condition.

Consider using continue to skip to the next iteration, or restructure the logic to avoid premature function termination.

---

Line range hint 2096-2102: Unintentional assignment in conditional statement

In the conditional statement if (searchObj.loadingHistogram == false && ...), using == for comparison is acceptable, but it is safer to use === to avoid type coercion issues.

Consider using strict equality checks:

- if (searchObj.loadingHistogram == false && ...)
+ if (searchObj.loadingHistogram === false && ...)

Configuration used: CodeRabbit UI
Review profile: CHILL

web/src/composables/useLogs.ts (4)

Line range hint 2063-2131: LGTM: Improved pagination and error handling

The changes to getPaginatedData are well-implemented:

1. The new appendResult parameter allows for more flexible data handling, which could be useful for pagination or infinite scrolling.
2. The error handling has been improved to provide more detailed information, including trace IDs for debugging.

These enhancements will improve the function's usability and make troubleshooting easier.

Consider using a custom error object or a more structured error response to standardize error handling across the application. This could make it easier to process and display errors consistently throughout the UI.

---

Line range hint 2395-2409: LGTM: Enhanced error handling in histogram query

The updates to getHistogramQueryData improve error handling:

1. More detailed error information is now provided, including trace IDs for better debugging.
2. A specific check for HTTP status 429 (Too Many Requests) has been added, allowing for special handling of rate limiting scenarios.

These changes will make it easier to diagnose and handle errors in the histogram query process.

For consistency, consider extracting the error handling logic into a separate function that can be reused across different API calls in the application. This would ensure a uniform approach to error processing and reduce code duplication.

---

Line range hint 4042-4084: LGTM: Improved ORDER BY clause handling

The refactoring of addOrderByToQuery is a significant improvement:

1. The addition of a try-catch block enhances error handling, making the function more robust.
2. The logic for adding the ORDER BY clause has been updated to be more comprehensive, handling various scenarios more effectively.

These changes will make the function more reliable and easier to maintain.

Consider adding a comment explaining the purpose of the includesTimestamp check, as its usage is not immediately clear in the current implementation. This would improve code readability and make future maintenance easier.

---

Line range hint 1-4245: Overall improvements in error handling and functionality

The changes made to this file represent a significant improvement in several areas:

1. Enhanced error handling across multiple functions, providing more detailed information for debugging.
2. Addition of new functions like resetHistogramWithLimitError to handle specific scenarios.
3. Refactoring of existing functions to improve robustness and handle edge cases better.
4. Consistent code style and structure maintained throughout the changes.

These improvements will lead to a more stable and maintainable codebase, as well as a better user experience due to more informative error messages and improved handling of various scenarios.

As the codebase grows, consider breaking down this large file into smaller, more focused modules. This could improve maintainability and make it easier for multiple developers to work on different parts of the code simultaneously. For example, you could separate histogram-related functions, pagination logic, and error handling into their own modules.

Configuration used: CodeRabbit UI
Review profile: CHILL

web/src/composables/useLogs.ts (11)

1801-1814: LGTM! Consider adding a brief comment explaining the function's purpose.

The resetHistogramWithLimitError function is well-implemented and serves its purpose of resetting the histogram data structure when there's a limit error. It correctly clears existing data and sets appropriate error messages.

Consider adding a brief comment above the function to explain its purpose and when it's called. This would improve code readability and maintainability.

---

Line range hint 1833-1893: Improved time calculations, but histogram data population is missing.

The generateHistogramSkeleton function has been significantly improved with more precise time calculations and better handling of different time intervals. However, there's a potential issue:

• The function no longer populates the histogramResults array, which might affect the histogram generation in other parts of the code.

Please review the commented-out code at the end of the function and ensure that the histogramResults array is still being populated correctly. If the population logic has been moved elsewhere, make sure it's properly implemented to avoid any issues with histogram generation.

---

Line range hint 1955-1975: LGTM! Consider refactoring for code reuse.

The fnHistogramParsedSQL function is correctly implemented and effectively parses the provided SQL query while handling potential errors. It's very similar to the fnParsedSQL function, which is good for consistency.

Consider refactoring these two functions to reduce code duplication. You could create a single helper function that both fnParsedSQL and fnHistogramParsedSQL call with their respective queries. This would improve maintainability and reduce the risk of inconsistencies if changes are needed in the future.

---

Line range hint 1977-2064: Improved error handling and tracing, but consider refactoring for maintainability.

The getPageCount function has been significantly improved:

• It now uses async/await syntax, which enhances readability.
• The addition of trace context generation is great for debugging.
• Error handling has been improved with more detailed error messages.

However, there are some areas for improvement:

1. The function is quite long and complex. Consider breaking it down into smaller, more focused functions to improve maintainability.
2. There's commented-out code that should be removed if it's no longer needed. This will improve code cleanliness.
3. The error handling logic is repeated in several places. Consider extracting it into a separate function to reduce duplication.

Remove the commented-out code if it's no longer needed. If it's kept for future reference, add a comment explaining why it's being kept.

---

Line range hint 2077-2385: Improved error handling and flexibility, but needs refactoring for maintainability.

The getPaginatedData function has been significantly enhanced:

• The use of async/await syntax improves readability.
• Trace context generation is great for debugging.
• Error handling has been improved with more detailed error messages.
• It handles various scenarios like SQL mode, multi-stream queries, and search around data effectively.

However, there are several areas for improvement:

1. The function is extremely long and complex. Consider breaking it down into smaller, more focused functions to improve maintainability and testability.
2. Extract the error handling logic into a separate function to reduce code duplication.
3. Consider using the Strategy pattern or a similar design pattern to handle different scenarios (SQL mode, multi-stream, etc.) more cleanly.

Remove the commented-out code if it's no longer needed. If it's kept for future reference, add a comment explaining why it's being kept.

The function handles many different scenarios, which might make it prone to bugs and difficult to maintain. Consider splitting it into multiple functions, each handling a specific scenario.

---

Line range hint 2387-2411: LGTM! Consider using map for a more idiomatic approach.

The filterHitsColumns function correctly filters the hits columns based on selected fields and always includes the timestamp column. The logic is sound and works as expected.

Consider refactoring the function to use map instead of a forEach loop with push. This would make the code more idiomatic and potentially more efficient. Here's an example of how it could look:

searchObj.data.queryResults.filteredHit = searchObj.data.queryResults.hits.map((hit) => {
  if (searchObj.data.stream.selectedFields.length > 0) {
    const filteredHit = {};
    searchObj.data.stream.selectedFields.forEach((field) => {
      if (hit.hasOwnProperty(field)) {
        filteredHit[field] = hit[field];
      }
    });
    filteredHit[store.state.zoConfig.timestamp_column] = hit[store.state.zoConfig.timestamp_column];
    return filteredHit;
  }
  return hit;
});

This approach creates a new array in a single operation, which is more in line with functional programming principles.

---

Line range hint 2413-2530: Improved cancellation handling and data processing, but needs refactoring.

The getHistogramQueryData function has been significantly enhanced:

• The addition of operation cancellation handling is a great improvement.
• Error handling has been improved with more detailed error messages.
• The histogram data processing seems more efficient.

However, there are several areas for improvement:

1. The function is quite long and complex. Consider breaking it down into smaller, more focused functions to improve maintainability.
2. Extract the error handling logic into a separate function to reduce code duplication.
3. Consider using a state machine or similar pattern to manage the different states of the histogram data retrieval process (cancelled, error, success) more cleanly.

Remove the commented-out code if it's no longer needed. If it's kept for future reference, add a comment explaining why it's being kept.

The error handling logic is repeated in several places. This increases the risk of inconsistencies if changes are needed in the future. Consider centralizing the error handling in a separate function.

---

Line range hint 2532-2563: LGTM! But check the return statement in the inner loop.

The updateFieldValues function correctly updates field values based on the query results, excluding specified fields and using a Set for efficient unique value storage.

There's a potential issue with the return statement inside the inner loop:

if (excludedFields.includes(key)) {
  return;
}

This return will only exit the inner loop, not the outer one. If the intention is to skip processing of excluded fields entirely, consider using continue instead of return, or move this check outside the inner loop.

Consider refactoring the nested loops to use Array.prototype.reduce for a more functional approach. This could make the code more concise and potentially more efficient.

---

Line range hint 2565-3045: Improved schema handling, but requires major refactoring for maintainability.

The extractFields function has been significantly enhanced:

• It now handles user-defined schemas more robustly.
• The processing of multiple streams and common fields has been improved.
• Additional error handling and debugging information have been added.

However, there are critical issues that need to be addressed:

1. The function is extremely long and complex (over 400 lines), making it very difficult to understand, maintain, and test.
2. It handles many different scenarios within a single function, increasing the likelihood of bugs and making it hard to reason about the code's behavior.

A major refactoring is strongly recommended:

1. Break down this function into smaller, more focused functions. Each function should handle a specific task (e.g., processing user-defined schemas, handling interesting fields, processing common fields).
2. Consider using the Strategy pattern or a similar design pattern to handle different scenarios more cleanly.
3. Extract repeated logic (like field processing) into separate helper functions.
4. Use meaningful variable names to improve readability (e.g., UDSFieldCount could be userDefinedSchemaFieldCount).

Remove commented-out code if it's no longer needed. If it's kept for future reference, add a comment explaining why it's being kept.

This refactoring will greatly improve the maintainability, readability, and testability of the code.

---

Line range hint 3047-3279: Improved column handling, but consider refactoring for clarity and maintainability.

The updateGridColumns function has been enhanced:

• It now handles more scenarios, including SQL mode and selected fields.
• The column width calculation has been improved.
• Additional error handling has been added.

However, there are areas that could be improved:

1. The function is quite long and handles multiple responsibilities. Consider breaking it down into smaller, more focused functions (e.g., one for timestamp column, one for selected fields, one for column width calculation).
2. Extract the column width calculation logic into a separate function for better reusability and testability.
3. Consider using a more declarative approach for column definition, possibly using a configuration object or factory function.

Remove commented-out code if it's no longer needed. If it's kept for future reference, add a comment explaining why it's being kept.

The nested loops and conditions make the function complex and harder to maintain. Consider simplifying the logic or breaking it down into smaller, more manageable pieces.

These changes will improve the readability, maintainability, and testability of the code.

---

Line range hint 3281-3315: LGTM! Consider optimizing canvas creation for better performance.

The getColumnWidth function effectively calculates the width of a column based on its content, using a canvas context to measure text width. It correctly handles different font styles for header and content and applies appropriate minimum and maximum constraints.

To optimize performance, consider creating the canvas element once and reusing it across multiple calls to this function. You could do this by:

1. Creating the canvas element at a higher scope (e.g., as a module-level constant).
2. Passing the canvas context as an argument to this function.

This approach would avoid the overhead of creating a new canvas element for each column width calculation, which could be significant if this function is called frequently.

Example:

const canvas = document.createElement("canvas");
const context = canvas.getContext("2d");

function getColumnWidth(context, field) {
  // ... existing function body ...
}

This optimization could lead to improved performance, especially when dealing with tables with many columns.

Configuration used: CodeRabbit UI
Review profile: CHILL

web/src/composables/useLogs.ts (7)

Line range hint 2395-2528: Consider refactoring getHistogramQueryData for improved maintainability.

The changes to this function enhance error handling and data processing. However, there are several areas for improvement:

1. The function is quite long and complex. Consider breaking it down into smaller, more focused functions.
2. There's commented-out code that should be removed if no longer needed (e.g., lines 2436-2453).
3. Error handling logic is repeated in multiple places. Consider extracting this into a separate function.

Here are some suggestions for refactoring:

1. Extract the histogram data generation logic (lines 2408-2454) into a separate function.
2. Create a reusable error handling function to reduce code duplication.
3. Remove or update the commented-out code to improve readability.

Example of extracted error handling function:

function handleHistogramError(err: any) {
  searchObj.loadingHistogram = false;
  let trace_id = "";
  searchObj.data.histogram.errorMsg = typeof err == "string" && err
    ? err
    : "Error while processing histogram request.";
  // ... rest of the error handling logic
}

This refactoring will improve the function's maintainability and readability.

---

Line range hint 2063-2391: Refactor getPaginatedData to improve maintainability and adhere to SOLID principles.

While the changes enhance error handling and data processing, the function has become overly complex and difficult to maintain. Consider the following improvements:

1. Break down the function into smaller, more focused functions adhering to the Single Responsibility Principle.
2. Remove commented-out code if it's no longer needed (e.g., lines 2071-2079, 2280-2287).
3. Extract repeated error handling logic into a separate function.
4. Consider using a state machine or a more structured approach to handle the different stages of data fetching and processing.

Here's a suggested approach for refactoring:

1. Extract the SQL mode and pagination handling into a separate function:

function handleSqlModeAndPagination(queryReq: any, parsedSQL: any) {
  // Logic from lines 2088-2117
}

2. Create a reusable error handling function:

function handlePaginationError(err: any) {
  // Error handling logic
}

3. Extract the data processing logic into a separate function:

function processPaginatedData(res: any) {
  // Data processing logic from lines 2124-2292
}

4. Refactor the main function to use these extracted functions:

const getPaginatedData = async (queryReq: any, appendResult: boolean = false) => {
  if (searchObj.data.isOperationCancelled) {
    handleCancelledOperation();
    return;
  }

  handleSqlModeAndPagination(queryReq, fnParsedSQL());

  try {
    const res = await searchService.search(/* ... */);
    await processPaginatedData(res);
    // ... rest of the success logic
  } catch (err) {
    handlePaginationError(err);
  }
};

This refactoring will significantly improve the function's readability, maintainability, and adherence to SOLID principles.

---

Line range hint 4042-4084: Improvements to addOrderByToQuery function, with suggestions for further enhancements.

The changes to this function have improved the handling of ORDER BY clauses in SQL queries. However, there are some areas for further improvement:

1. Error handling is minimal. Consider adding more detailed error logging or throwing a custom error.
2. The function modifies the query even if an ORDER BY clause already exists. It might be more efficient to check for an existing ORDER BY clause first and only modify if necessary.

Here's a suggested refactor to address these points:

const addOrderByToQuery = (sql: string, column: string, type: "ASC" | "DESC", streamName: string) => {
  try {
    const parsedQuery: any = parser.astify(sql);

    if (!parsedQuery.columns.length || !parsedQuery.from) {
      return sql;
    }

    const hasOrderBy = !!(parsedQuery.orderby && parsedQuery.orderby.length > 0);
    const includesTimestamp = !!parsedQuery.columns.find(
      (col: any) => col?.expr?.column === column || col?.expr?.column === "*"
    );

    if (!hasOrderBy && includesTimestamp) {
      parsedQuery.orderby = [{
        expr: {
          type: "column_ref",
          table: null,
          column: column,
        },
        type: type,
      }];

      return quoteTableNameDirectly(
        parser.sqlify(parsedQuery).replace(/`/g, ""),
        streamName
      );
    }

    return sql; // Return original SQL if no changes are needed
  } catch (err) {
    console.error("Error in addOrderByToQuery:", err);
    return sql;
  }
};

This refactored version only modifies the query if there's no existing ORDER BY clause and the timestamp column is included. It also adds basic error logging.

---

Line range hint 4099-4124: Improvements to getRegionInfo function, with suggestions for error handling and type safety.

The changes to this function have improved the structure of region and cluster data. However, there are some areas for further enhancement:

1. Error handling: The function doesn't handle potential errors from the API call.
2. Type safety: There's no type checking or validation of the API response.

Here's a suggested refactor to address these points:

interface ClusterInfo {
  label: string;
}

interface RegionInfo {
  label: string;
  children: ClusterInfo[];
}

const getRegionInfo = async (): Promise<void> => {
  try {
    const res = await searchService.get_regions();
    const apiData: Record<string, string[]> = res.data;
    
    const clusterData: RegionInfo[] = Object.entries(apiData).map(([region, clusters]) => ({
      label: region,
      children: clusters.map(cluster => ({ label: cluster })),
    }));

    store.dispatch("setRegionInfo", clusterData);
  } catch (error) {
    console.error("Error fetching region info:", error);
    // Consider showing a user-friendly error message
    // showErrorNotification("Failed to fetch region information");
  }
};

This refactored version:

1. Adds error handling for the API call.
2. Uses TypeScript interfaces for better type safety.
3. Simplifies the data transformation using Object.entries and map.
4. Removes the unnecessary regionObj variable.

These changes will make the function more robust and easier to maintain.

---

Line range hint 4135-4169: Enhancements to cancelQuery function with suggestions for improved error handling and return value.

The changes to this function have improved the query cancellation process. However, there are some areas for further enhancement:

1. Error handling could be more specific, potentially differentiating between different types of errors.
2. The function doesn't return any value, making it difficult to programmatically determine if the cancellation was successful.

Here's a suggested refactor to address these points:

const cancelQuery = async (): Promise<boolean> => {
  const tracesIds = [...searchObj.data.searchRequestTraceIds];
  searchObj.data.isOperationCancelled = true;

  try {
    const res = await searchService.delete_running_queries(
      store.state.selectedOrganization.identifier,
      searchObj.data.searchRequestTraceIds
    );

    const isCancelled = res.data.some((item: any) => item.is_success);
    
    if (isCancelled) {
      searchObj.data.isOperationCancelled = false;
      $q.notify({
        message: "Running query cancelled successfully",
        color: "positive",
        position: "bottom",
        timeout: 4000,
      });
    } else {
      throw new Error("No queries were successfully cancelled");
    }

    return isCancelled;
  } catch (error: any) {
    const errorMessage = error.response?.data?.message || "Failed to cancel running query";
    $q.notify({
      message: errorMessage,
      color: "negative",
      position: "bottom",
      timeout: 1500,
    });
    return false;
  } finally {
    searchObj.data.searchRequestTraceIds = searchObj.data.searchRequestTraceIds.filter(
      (id: string) => !tracesIds.includes(id)
    );
  }
};

This refactored version:

1. Returns a boolean indicating whether the cancellation was successful.
2. Uses async/await for better readability and error handling.
3. Provides more specific error handling, including a custom error for when no queries were cancelled.
4. Moves the trace ID cleanup to a finally block to ensure it always happens.

These changes will make the function more robust and easier to use programmatically.

---

Line range hint 4171-4183: Enhance reorderArrayByReference for clarity and predictability.

The new reorderArrayByReference function correctly reorders an array based on a reference array. However, consider the following improvements:

1. The function modifies the input array in-place, which might lead to unexpected side effects. Consider creating a new array instead.
2. The type annotations could be more specific to improve type safety.
3. The function name could be more descriptive to better convey its purpose.

Here's a suggested refactor:

function sortArrayByReferenceOrder<T>(arrayToSort: T[], referenceArray: T[]): T[] {
  return [...arrayToSort].sort((a, b) => {
    const indexA = referenceArray.indexOf(a);
    const indexB = referenceArray.indexOf(b);

    if (indexA === -1 && indexB === -1) return 0;
    if (indexA === -1) return 1;
    if (indexB === -1) return -1;

    return indexA - indexB;
  });
}

This refactored version:

1. Creates a new sorted array instead of modifying the input array.
2. Uses generic type T for better type safety.
3. Has a more descriptive name sortArrayByReferenceOrder.
4. Handles the case where both elements are not in the reference array.

Usage example:

const sortedArray = sortArrayByReferenceOrder(arr1, arr2);

This approach provides a clearer and more predictable behavior while maintaining the original functionality.

---

Line range hint 4185-4202: Optimize reorderSelectedFields for efficiency and robustness.

The new reorderSelectedFields function correctly reorders the selected fields based on a column order. However, consider the following improvements:

1. Avoid unnecessary array copying and comparison for better efficiency.
2. Add more specific type annotations to improve type safety.
3. Handle the case where colOrder is undefined more gracefully.

Here's a suggested refactor:

function reorderSelectedFields(): string[] {
  const selectedFields = searchObj.data.stream.selectedFields;
  const streamKey = searchObj.data.stream.selectedStream;
  let colOrder = searchObj.data.resultGrid.colOrder[streamKey];

  if (!Array.isArray(colOrder)) {
    return selectedFields; // Return original array if colOrder is not valid
  }

  const timestampColumn = store.state.zoConfig.timestamp_column;
  const includesTimestamp = selectedFields.includes(timestampColumn);

  if (!includesTimestamp) {
    colOrder = colOrder.filter(v => v !== timestampColumn);
  }

  // Only reorder if necessary
  if (!arraysEqual(selectedFields, colOrder)) {
    return sortArrayByReferenceOrder(selectedFields, colOrder);
  }

  return selectedFields;
}

// Helper function to check if arrays are equal
function arraysEqual(a: string[], b: string[]): boolean {
  return a.length === b.length && a.every((v, i) => v === b[i]);
}

This refactored version:

1. Avoids unnecessary array copying by only creating a new array if reordering is needed.
2. Uses more specific type annotations.
3. Handles the case where colOrder is undefined or not an array.
4. Uses a helper function arraysEqual to efficiently check if reordering is necessary.
5. Assumes the sortArrayByReferenceOrder function from the previous refactoring suggestion is used.

These changes will make the function more efficient and robust while maintaining its original functionality.

web/src/plugins/logs/Index.vue (2)

Line range hint 652-659: Simplify Conditional Logic in Watcher

The watcher for router.currentRoute.value.query can be simplified for better readability and efficiency.

Consider simplifying the watcher as follows:

 watch(
   () => router.currentRoute.value.query,
   () => {
-    if (!router.currentRoute.value.query.hasOwnProperty("action")) {
-      showSearchHistory.value = false;
-    }
-    if (
-      router.currentRoute.value.query.hasOwnProperty("action") &&
-      router.currentRoute.value.query.action == "history"
-    ) {
-      showSearchHistory.value = true;
-    }
+    showSearchHistory.value =
+      router.currentRoute.value.query.action === "history";
   },
 );

---

Line range hint 1299-1335: Avoid Async Watcher Functions

Using async in a watcher function can lead to unexpected behavior because Vue does not wait for promises in watchers. This might cause race conditions or unhandled promise rejections.

Refactor the watcher to handle asynchronous operations without making the entire watcher function async:

-watch(
+watchEffect(
   async () => {
-    async showHistogram() {
       let parsedSQL = null;
       if (this.searchObj.meta.sqlMode) parsedSQL = this.fnParsedSQL();

       if (
         this.searchObj.meta.showHistogram &&
         !this.searchObj.shouldIgnoreWatcher
       ) {
         this.searchObj.data.queryResults.aggs = [];

         if (
           this.searchObj.meta.sqlMode &&
           !this.isNonAggregatedQuery(parsedSQL)
         ) {
           this.resetHistogramWithLimitError();
         } else if (this.searchObj.meta.histogramDirtyFlag == true) {
           this.searchObj.meta.histogramDirtyFlag = false;
           // this.handleRunQuery();
           this.searchObj.loadingHistogram = true;

-          await this.generateHistogramSkeleton();
+          // Handle asynchronous operations inside the watcher
+          (async () => {
+            await this.generateHistogramSkeleton();

-          this.getHistogramQueryData(this.searchObj.data.histogramQuery)
-            .then((res: any) => {
-              this.refreshTimezone();
-              this.searchResultRef.reDrawChart();
-            })
-            .catch((err: any) => {
-              console.log(err, "err in updating chart");
-            })
-            .finally(() => {
-              this.searchObj.loadingHistogram = false;
-            });
+            try {
+              await this.getHistogramQueryData(this.searchObj.data.histogramQuery);
+              this.refreshTimezone();
+              this.searchResultRef.reDrawChart();
+            } catch (err) {
+              console.log(err, "err in updating chart");
+            } finally {
+              this.searchObj.loadingHistogram = false;
+            }
+          })();

           setTimeout(() => {
             if (this.searchResultRef) this.searchResultRef.reDrawChart();
           }, 100);
         }
       }

       this.updateUrlQueryParams();
-    },
   },
 );

Configuration used: CodeRabbit UI
Review profile: CHILL

web/src/composables/useLogs.ts (3)

217-221: Approve TODO for code organization refactoring.

The TODO comment outlines a good plan for improving code organization by separating concerns into distinct composables. This aligns well with best practices for maintainability and modularity.

Would you like me to create a GitHub issue to track this refactoring task? This will help ensure it's not forgotten and can be prioritized appropriately.

---

4248-4249: Approve isNonAggregatedQuery function with a suggestion for enhancement.

The isNonAggregatedQuery function is implemented correctly and handles the case where parsedSQL might be null. Its name clearly describes its purpose, which is good for readability and maintainability.

Consider if additional checks might be needed for a more comprehensive determination of aggregated queries. For example, you might want to check for the presence of aggregate functions (COUNT, SUM, AVG, etc.) or GROUP BY clauses. This could make the function more robust:

function isNonAggregatedQuery(parsedSQL: any = null): boolean {
  if (!parsedSQL) return true;
  
  const hasLimit = !!parsedSQL.limit;
  const hasGroupBy = !!parsedSQL.groupby;
  const hasAggregateFunctions = parsedSQL.columns?.some((col: any) => 
    col.expr && col.expr.type === 'aggr_func'
  );

  return !hasLimit && !hasGroupBy && !hasAggregateFunctions;
}

This enhanced version checks for LIMIT, GROUP BY, and aggregate functions, providing a more comprehensive determination of non-aggregated queries.

---

Line range hint 4049-4087: Approve addOrderByToQuery function with suggestions for improvement.

The addOrderByToQuery function effectively adds an ORDER BY clause to a SQL query when it doesn't exist. It handles parsing errors and checks for the presence of the timestamp column in the SELECT clause, which are good practices.

Consider the following improvements to make the function more robust:

1. Handle existing ORDER BY clauses more gracefully:

   • If the specified column is already in the ORDER BY clause, update its sort order instead of adding a new clause.
   • If other columns are in the ORDER BY clause, consider whether to preserve them or replace the entire clause.

2. Improve handling of complex queries:

   • Add support for queries with subqueries or Common Table Expressions (CTEs).
   • Consider using a more robust SQL parser that can handle a wider range of SQL syntax.

Here's a sketch of how you might implement these improvements:

function addOrderByToQuery(sql: string, column: string, type: "ASC" | "DESC", streamName: string) {
  try {
    const parsedQuery: any = parser.astify(sql);

    if (!parsedQuery.columns.length || !parsedQuery.from) {
      return sql;
    }

    const existingOrderBy = parsedQuery.orderby || [];
    const columnIndex = existingOrderBy.findIndex((order: any) => order.expr.column === column);

    if (columnIndex !== -1) {
      // Update existing ORDER BY for the column
      existingOrderBy[columnIndex].type = type;
    } else {
      // Add new ORDER BY
      existingOrderBy.push({
        expr: { type: "column_ref", table: null, column: column },
        type: type
      });
    }

    parsedQuery.orderby = existingOrderBy;

    // Handle subqueries and CTEs here if needed

    return quoteTableNameDirectly(parser.sqlify(parsedQuery).replace(/`/g, ""), streamName);
  } catch (err) {
    console.error("Error modifying ORDER BY clause:", err);
    return sql;
  }
}

This improved version updates existing ORDER BY clauses for the specified column and preserves other ordered columns. You may need to add additional logic to handle subqueries and CTEs based on your specific use cases.

web/src/plugins/logs/Index.vue (3)

Line range hint 637-651: Use Strict Equality for Boolean Comparisons

In the watcher, it's better to use strict equality (===) when comparing boolean values to avoid unexpected type coercion.

- if (searchObj.shouldIgnoreWatcher == false && ...)
+ if (searchObj.shouldIgnoreWatcher === false && ...)

---

772-772: Clarify the Purpose of the true Argument

When calling getStream, the purpose of the true argument is not immediately clear. Adding a comment or using a named parameter can improve code readability.

// The third argument 'true' forces a cache refresh
const streamData = getStream(
  searchObj.data.stream.selectedStream[0],
  searchObj.data.stream.streamType || "logs",
  true // Force cache refresh
);

---

Line range hint 1297-1336: Handle Asynchronous Operations Carefully in Watcher

In your watcher for showHistogram, ensure that asynchronous operations are properly handled to prevent race conditions or memory leaks.

• Use try-catch blocks around await calls.
• Cancel any pending operations if the watcher is triggered again before completion.

watch(
  () => this.showHistogram,
  async () => {
    try {
      // Your existing logic
    } catch (error) {
      console.error("Error updating histogram:", error);
    }
  }
);

Configuration used: CodeRabbit UI
Review profile: CHILL