Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add full-text search on windmill service logs #4576

Open
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

wendrul
Copy link
Contributor

@wendrul wendrul commented Oct 23, 2024

https://github.com/windmill-labs/windmill-ee-private/pull/79


Important

Add full-text search functionality for service logs, including backend indexing and frontend UI components.

  • Backend:
    • Add full-text search for service logs in main.rs using tantivy and parquet features.
    • Introduce service_logs_ee.rs for indexing service logs.
    • Update openapi.yaml with new endpoints /srch/index/search/service_logs and /srch/index/search/count_service_logs.
    • Modify lib.rs to include ServiceLogIndexReader and update run_server() to handle log index readers.
  • Frontend:
    • Add ServiceLogsInner.svelte and LogSnippetViewer.svelte for displaying and interacting with log search results.
    • Update GlobalSearchModal.svelte to support log search with ! prefix.
    • Modify SidebarContent.svelte to include a link to service logs for superadmins.
    • Update package.json to bump lucide-svelte version.
  • Misc:
    • Rename indexer_ee.rs to completed_runs_ee.rs and create service_logs_ee.rs for service log indexing.
    • Add new Svelte components and update existing ones to support log search and display.

This description was created by Ellipsis for 45f5bbd. It will automatically update as commits are pushed.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me! Reviewed everything up to e7d9aee in 40 seconds

More details
  • Looked at 1458 lines of code in 21 files
  • Skipped 1 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. backend/src/main.rs:488
  • Draft comment:
    Ensure that the index_writer is used consistently across feature flags. Currently, index_writer is cloned and used in indexer_f but not in log_indexer_f. This could lead to inconsistencies if feature flags are not set correctly.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The index_writer is being cloned and used in the indexer_f async block, but it is not being used in the log_indexer_f async block. This could lead to confusion or errors if the feature flags are not set correctly.
2. backend/src/main.rs:814
  • Draft comment:
    Ensure that the index_writer is used consistently across feature flags. Currently, index_writer is cloned and used in indexer_f but not in log_indexer_f. This could lead to inconsistencies if feature flags are not set correctly.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The index_writer is being cloned and used in the indexer_f async block, but it is not being used in the log_indexer_f async block. This could lead to confusion or errors if the feature flags are not set correctly.

Workflow ID: wflow_9fGfcewmwyp9eG5K


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link

cloudflare-workers-and-pages bot commented Oct 23, 2024

Deploying windmill with  Cloudflare Pages  Cloudflare Pages

Latest commit: 8ef6dd9
Status: ✅  Deploy successful!
Preview URL: https://35465d7d.windmill.pages.dev
Branch Preview URL: https://win-407-tantivy-service-logs.windmill.pages.dev

View logs

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me! Incremental review on dae3756 in 21 seconds

More details
  • Looked at 12 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. frontend/src/lib/components/search/GlobalSearchModal.svelte:39
  • Draft comment:
    The import for ServiceLogsInner was removed, but the component is still referenced in the description. Ensure this is intentional and that the component is not used elsewhere.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The import statement for ServiceLogsInner was removed, but the component is still referenced in the description. This might be an oversight.

Workflow ID: wflow_XRxTqNTaLez2TbaY


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me! Incremental review on b152db1 in 33 seconds

More details
  • Looked at 97 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 drafted comments based on config settings.
1. frontend/src/routes/(root)/(logged)/service_logs/+page.svelte:42
  • Draft comment:
    Good use of Popover to display parse errors for invalid search terms.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    The Popover component is used to display parse errors when the search term is invalid. The slot="text" is correctly used to provide the content for the popover. This is a good implementation for user feedback.
2. frontend/src/routes/(root)/(logged)/service_logs/+page.svelte:57
  • Draft comment:
    Using bind:queryParseErrors ensures that parse errors are reactive and managed properly between components.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    The ServiceLogsInner component is used with bind:queryParseErrors to handle parse errors. This ensures that any changes in the parse errors are reflected in the parent component, which is a good practice for managing state.
3. frontend/src/lib/components/ServiceLogsInner.svelte:400
  • Draft comment:
    Using SplitPanesWrapper for responsive layout is a good practice for UI adaptability.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    The SplitPanesWrapper component is used to create a responsive layout for the service logs. This is a good approach to ensure that the UI adapts to different screen sizes.

Workflow ID: wflow_P7qJIoUeohXMtpeM


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me! Incremental review on 45f5bbd in 39 seconds

More details
  • Looked at 208 lines of code in 3 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. frontend/src/lib/components/ServiceLogsInner.svelte:537
  • Draft comment:
    Consider optimizing the filter function to reduce the number of iterations over Object.entries(o2). This pattern is repeated in multiple places.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    In ServiceLogsInner.svelte, the filter function is used multiple times to filter logs based on conditions. This could be optimized by combining conditions or reducing the number of iterations.
2. frontend/src/lib/components/ServiceLogsInner.svelte:429
  • Draft comment:
    Ensure that getAllLogs is called with valid date strings to avoid unexpected behavior if the API expects them.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    In ServiceLogsInner.svelte, the getAllLogs function is called with undefined values for queryMinTs and queryMaxTs initially, which might lead to unexpected behavior if the API expects valid date strings.

Workflow ID: wflow_0IUzaPT57uoskqRr


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant