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

perf: fetch only new inputs on refresh of inputs history #5315

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

Conversation

pyranota
Copy link
Collaborator

@pyranota pyranota commented Feb 17, 2025

Important

Optimized input history fetching by adding a since parameter to only retrieve new inputs, enhancing performance across backend and frontend components.

  • Backend:
    • Added since query parameter to get_input_history in inputs.rs to fetch inputs created after a specific date.
    • Updated SQL query in get_input_history to filter results based on v2_job_completed.started_at.
  • Frontend:
    • Modified loadInputsPageFn in HistoricList.svelte to use since parameter for non-discovery fetches.
    • Updated InfiniteList.svelte to pass discovery flag to loadInputs function.
    • Adjusted SavedInputsPicker.svelte and CaptureTable.svelte to accommodate new loadInputsPageFn signature.

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

Copy link

cloudflare-workers-and-pages bot commented Feb 17, 2025

Deploying windmill with  Cloudflare Pages  Cloudflare Pages

Latest commit: d5a3ead
Status: ✅  Deploy successful!
Preview URL: https://ae59f155.windmill.pages.dev
Branch Preview URL: https://perf-since-arg-historic-inpu.windmill.pages.dev

View logs

@pyranota pyranota marked this pull request as ready for review February 17, 2025 21:16
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.

❌ Changes requested. Reviewed everything up to 50b9b27 in 2 minutes and 27 seconds

More details
  • Looked at 207 lines of code in 6 files
  • Skipped 0 files when reviewing.
  • Skipped posting 8 drafted comments based on config settings.
1. backend/windmill-api/openapi.yaml:10159
  • Draft comment:
    Consider adding a format (e.g. date-time) to the 'since' parameter schema instead of only type string for clearer API documentation.
  • Reason this comment was not posted:
    Confidence changes required: 50% <= threshold 50%
    None
2. backend/windmill-api/src/inputs.rs:162
  • Draft comment:
    Review the fallback behavior using DateTime::default() when 'since' is not provided. Ensure it matches the intended query semantics (e.g. fetching all historical inputs).
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 30% <= threshold 50%
    The comment is asking the PR author to ensure that the fallback behavior matches the intended query semantics. This violates the rule against asking the author to ensure behavior is intended. However, it does suggest reviewing the fallback behavior, which could be seen as a specific code suggestion. Overall, it leans more towards asking for confirmation rather than providing a specific suggestion or improvement.
3. frontend/src/lib/components/HistoricList.svelte:34
  • Draft comment:
    Ensure the discovery flag handling aligns with the new 'since' parameter – verify that updating 'lastChecked' only after a successful non-discovery fetch does not introduce race conditions.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    The comment is asking the PR author to verify that a specific behavior does not introduce race conditions. This falls under the rule of not asking the author to ensure behavior is intended or to double-check things. Therefore, this comment should be removed.
4. frontend/src/lib/components/InfiniteList.svelte:36
  • Draft comment:
    The signature update to include the 'discovery' parameter is consistent across components. Verify that its usage in loadData and corresponding logic in scenarios such as loadMore and refresh is well-tested.
  • Reason this comment was not posted:
    Confidence changes required: 50% <= threshold 50%
    None
5. backend/windmill-api/src/inputs.rs:162
  • Draft comment:
    Parsing 'since' as DateTime may cause timezone inconsistencies since other timestamps use Utc. Consider using DateTime or ensuring proper conversion. Additionally, using DateTime::default() as a fallback might be error‐prone if not intended.
  • Reason this comment was not posted:
    Marked as duplicate.
6. frontend/src/lib/components/HistoricList.svelte:45
  • Draft comment:
    Updating 'lastChecked' immediately (before awaiting the fetch) might lead to race conditions. Consider updating it after a successful fetch to ensure only new inputs are filtered.
  • Reason this comment was not posted:
    Marked as duplicate.
7. frontend/src/lib/components/HistoricList.svelte:80
  • Draft comment:
    Consider splitting the comma-separated operations into separate statements for improved readability.
  • Reason this comment was not posted:
    Marked as duplicate.
8. frontend/src/lib/components/InfiniteList.svelte:36
  • Draft comment:
    Loader function calls are updated to include the 'discovery' flag. Ensure that this change is documented and consistently used across all components.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    The comment is asking the PR author to ensure that the change is documented and consistently used across all components. This falls under the rule of not asking the author to ensure behavior or documentation, which is not allowed.

Workflow ID: wflow_oqqBmUEzQQRVUFd4


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@@ -10159,6 +10159,10 @@ paths:
in: query
schema:
type: boolean
- name: since
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider adding a description and format (e.g. ISO8601) for the new 'since' query parameter to improve API documentation clarity.

Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>
@pyranota pyranota marked this pull request as draft February 17, 2025 21:20
@pyranota pyranota marked this pull request as ready for review February 17, 2025 21:24
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.

❌ Changes requested. Reviewed everything up to 27ca01b in 3 minutes and 48 seconds

More details
  • Looked at 207 lines of code in 6 files
  • Skipped 0 files when reviewing.
  • Skipped posting 11 drafted comments based on config settings.
1. backend/windmill-api/openapi.yaml:10159
  • Draft comment:
    New 'since' query parameter added. Confirm that its type (string) and description are sufficient for clients.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
2. backend/windmill-api/src/inputs.rs:162
  • Draft comment:
    Using DateTime::default() as a fallback may introduce unexpected start date values. Consider a more meaningful default (e.g., epoch start) or explicitly reject missing 'since' parameter if required.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
    Using DateTime::default() actually returns the Unix epoch, which is a reasonable and common default when filtering by date - it effectively means "show all history". The comment suggests this might be unexpected, but in this context it's actually a sensible choice that will return all historical data. The comment seems to misunderstand that DateTime::default() is unpredictable when it's actually well-defined.
    I could be wrong about whether returning all historical data is the intended behavior. Maybe there are performance implications of potentially querying the entire history.
    Even if querying all history isn't ideal, using epoch start is still a clear and well-defined behavior. If performance is a concern, that should be addressed separately with proper pagination (which this code already has).
    The comment should be deleted because DateTime::default() returning the epoch is actually a reasonable default that provides clear and useful behavior.
3. backend/windmill-api/src/inputs.rs:137
  • Draft comment:
    Ensure that the SQL query parameter ordering matches the binding order, especially with the new 'since' parameter in position $4. It appears correct, but double-check against runtime expectations.
  • Reason this comment was not posted:
    Confidence changes required: 20% <= threshold 50%
    None
4. frontend/src/lib/components/HistoricList.svelte:34
  • Draft comment:
    The added 'discovery' parameter alters the loader behavior. The buffering mechanism using 'items', 'potentialItems', and 'perPageBind' is somewhat convoluted. Consider adding comments or refactoring for clarity.
  • Reason this comment was not posted:
    Confidence changes required: 50% <= threshold 50%
    None
5. frontend/src/lib/components/InfiniteList.svelte:21
  • Draft comment:
    Signature of 'loadInputs' updated to include 'discovery' parameter. Verify that all consumers properly ignore or pass the flag, as seen in SavedInputsPicker and CaptureTable.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
6. frontend/src/lib/components/SavedInputsPicker.svelte:62
  • Draft comment:
    Load function in SavedInputsPicker ignores the 'discovery' parameter (named as '_discovery'). Ensure this is intentional so that discovery-based refresh logic isn’t applied in saved inputs.
  • Reason this comment was not posted:
    Confidence changes required: 20% <= threshold 50%
    None
7. frontend/src/lib/components/triggers/CaptureTable.svelte:70
  • Draft comment:
    Similarly, the local load function for captures accepts a '_discovery' parameter. Confirm that discovery logic is not needed for CaptureTable or that its behavior matches design.
  • Reason this comment was not posted:
    Confidence changes required: 20% <= threshold 50%
    None
8. backend/windmill-api/openapi.yaml:10159
  • Draft comment:
    Consider specifying a format or pattern (e.g. ISO 8601) for the 'since' query parameter to improve API clarity.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 40% vs. threshold = 50%
    This is a reasonable suggestion to improve API documentation by specifying the expected date/time format for the 'since' parameter. Having a clear format specified would help API consumers know exactly what format to use. However, I need to be cautious about keeping comments that aren't strictly necessary.
    The comment is a suggestion for improved documentation rather than pointing out an actual bug or issue. The API would still function without this change. Additionally, the format may be intentionally flexible to accept multiple common date formats.
    While not critical, having clear API documentation is important for usability. Specifying the expected format would prevent confusion and potential errors from API consumers using incorrect date formats.
    While this is a reasonable suggestion for improving API clarity, it's not pointing out a critical issue that must be fixed. The API can function without this change.
9. backend/windmill-api/src/inputs.rs:162
  • Draft comment:
    Clarify the expected date format for 'since' and consider whether using DateTime::default() (as fallback) is appropriate. An explicit value (e.g., UNIX epoch) might improve clarity.
  • Reason this comment was not posted:
    Marked as duplicate.
10. frontend/src/lib/components/HistoricList.svelte:42
  • Draft comment:
    Review the logic merging 'items' and 'potentialItems'. The use of 'perPageBind' to conditionally push potential items could risk race conditions or duplicate ordering; ensure this behavior meets the intended refresh semantics.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
    The code has clear comments explaining why this approach was chosen. The perPageBind check is intentionally designed to prevent unwanted list expansion. While race conditions are theoretically possible, the comment is speculative ("could risk") and doesn't point to a specific, demonstrable issue. The author seems to have thought through the refresh semantics.
    I might be overlooking real race conditions in the async nature of the code. The comment could be pointing to legitimate concerns about data consistency.
    While async code can be tricky, the comment doesn't provide specific evidence of problems, and the code's comments suggest the behavior is intentional and well-thought-out.
    Delete the comment as it's speculative ("could risk") and doesn't point to a specific, demonstrable issue. The code appears to have intentional logic with documented reasoning.
11. frontend/src/lib/components/InfiniteList.svelte:35
  • Draft comment:
    Ensure that all callers of loadInputs pass the correct 'discovery' flag. This change in the loader signature for pagination must be consistently handled to avoid unexpected behavior.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    The comment is asking the PR author to ensure that all callers of a function pass a specific flag correctly. This is similar to asking the author to double-check or verify something, which is against the rules. The comment does not provide a specific suggestion or point out a specific issue, but rather gives a general instruction to ensure consistency, which is not allowed.

Workflow ID: wflow_31DEOoDPtcHe1yqU


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@rubenfiszel rubenfiszel force-pushed the main branch 7 times, most recently from 3a05600 to 3b6585a Compare February 18, 2025 00:54
pyranota and others added 3 commits February 18, 2025 15:33
Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>
Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>
@rubenfiszel rubenfiszel force-pushed the main branch 2 times, most recently from 470be4b to 3188bee Compare February 21, 2025 13:07
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.

2 participants