Add concurrency limits observability #3586
Conversation
Deploying windmill with
|
| Latest commit: |
aeeab53
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://f488e4e9.windmill.pages.dev |
| Branch Preview URL: | https://concurrency-limits-observabi.windmill.pages.dev |
wendrul
force-pushed
the
concurrency-limits-observability
branch
from
baed65d
to
97eb76b
Compare
rubenfiszel
force-pushed
the
main
branch
2 times, most recently
from
44529f6
to
ea4165d
Compare
wendrul
force-pushed
the
concurrency-limits-observability
branch
from
195380f
to
c922507
Compare
Add a new table custom_concurrency_key for storing concurrency keys on a per job basis. Add the field to the job payload to populate the table on creation of a new job.
Be more explicit about the nature of the option variable by calling it a more appropriate name.
FlowValue field concurrency_key must be named concurrency_key for serialization purposes.
Whether it is a Completed job or a queued one, add a ways to get the associated concurrency key
Add a line with a link on the FlowMetadata that goes to the concurrency_groups page. add endpoint to get the concurrency_key of a job
migration + change all related querrys
The limit makes more sense if we cut the older rows, so order by started_at
Factor the arg interpolation logic into a function and finish the processing of concurrency key before insertion
the intervals endpoint now also gets the concurrencyt key information for all jobs
Create a ToggleButtonMore and put elements into a dropdown
wendrul
force-pushed
the
concurrency-limits-observability
branch
from
7bc2932
to
42aceed
Compare
wendrul
changed the title
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to 42aceed in 3 minutes and 30 seconds
More details
- Looked at
4104lines of code in52files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_taqIAaZRFKukeEdF
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.
⌛ 1 day left in your free trial, upgrade for $20/seat/month or contact us.
| class="!h-[32px] py-1 !text-xs !w-64" | ||
| bind:value={displayedConcurrencyKey} | ||
| on:keydown={(e) => { | ||
| if (concurrencyKeyTimeout) { |
There was a problem hiding this comment.
Consider handling input changes more reactively without using a timeout to avoid potential race conditions.
There was a problem hiding this comment.
This is done in the same manner as the label filter. If we want to change it it would fit better in a separate PR to address this
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 6477171 in 2 minutes and 5 seconds
More details
- Looked at
427lines of code in11files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_bafZq7l05tKe8wdH
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
⌛ 1 day left in your free trial, upgrade for $20/seat/month or contact us.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 401166d in 2 minutes and 23 seconds
More details
- Looked at
7lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. backend/ee-repo-ref.txt:1
- Draft comment:
The PR description details several changes and additions, but the diff only shows an update to a reference file. Can you confirm if the implementation of the features described (new table, frontend components, etc.) is handled in another repository or if there might be missing files in this PR? - Reason this comment was not posted:
Confidence of 0% on close inspection, compared to threshold of 50%.
Workflow ID: wflow_dPOjSuH89kIkNJh4
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
⌛ 1 day left in your free trial, upgrade for $20/seat/month or contact us.
Instead of renaming the custom_concurrency_key_ended atble, we create a new table and we will delete custom_concurrency_key_ended in the future when it is no longer linked to any jobs
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 4f9b606 in 2 minutes and 25 seconds
More details
- Looked at
61lines of code in4files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. backend/migrations/20240515093517_concurrency_key_observability.down.sql:2
- Draft comment:
The down migration script should also include a command to drop the indexconcurrency_key_ended_at_idxbefore dropping the table to ensure a clean rollback.
DROP INDEX IF EXISTS concurrency_key_ended_at_idx;
DROP TABLE IF EXISTS concurrency_key;
- Reason this comment was not posted:
Confidence of 50% on close inspection, compared to threshold of 50%.
Workflow ID: wflow_1uLSFz163sV1Duo5
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.
⌛ 1 day left in your free trial, upgrade for $20/seat/month or contact us.
| key VARCHAR(255) NOT NULL, | ||
| ended_at TIMESTAMP WITH TIME ZONE, | ||
| job_id uuid NOT NULL, | ||
| PRIMARY KEY (job_id) |
There was a problem hiding this comment.
Consider modifying the primary key to include both 'key' and 'job_id' to ensure uniqueness and data integrity across different jobs.
| PRIMARY KEY (job_id) | |
| PRIMARY KEY (key, job_id) |
Instead of filtering jobs in the backend on a startedAfter value, limit the query to 1000 and query all possible towards the past to get a good context for the graph in most sane situations
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on df7d2c7 in 2 minutes and 21 seconds
More details
- Looked at
72lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. frontend/src/lib/components/runs/JobLoader.svelte:194
- Draft comment:
ThefetchConcurrencyIntervalsfunction is consistently called withundefinedfor thestartedAfterparameter. If this is intentional to fetch records from the beginning, consider documenting this explicitly to avoid confusion. - Reason this comment was not posted:
Confidence of 0% on close inspection, compared to threshold of 50%.
Workflow ID: wflow_0msEQkOv1s3XiqB6
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
⌛ 1 day left in your free trial, upgrade for $20/seat/month or contact us.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on a51a5b0 in 2 minutes and 30 seconds
More details
- Looked at
259lines of code in5files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. frontend/src/lib/components/FlowMetadata.svelte:1
- Draft comment:
The PR description mentions adding a new tablecustom_concurrency_keyfor storing concurrency keys on a per job basis. However, there is no evidence in the provided code snippets of any backend changes related to the creation or management of this table. Please ensure that the backend changes are included and properly integrated with the frontend. - Reason this comment was not posted:
Confidence of 0% on close inspection, compared to threshold of 50%.
Workflow ID: wflow_KFDXYa5ysOGNpXf5
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
⌛ 1 day left in your free trial, upgrade for $20/seat/month or contact us.
per job basis. Add the field to the job payload to populate the table on
creation of a new job.
Summary:
This PR adds backend and frontend support for observing and managing job concurrency limits through new data structures, visualizations, and enhanced filtering capabilities.
Key points:
ConcurrentJobsChartfor visualizing job concurrency.Generated with ❤️ by ellipsis.dev
Summary:
This PR adds comprehensive backend and frontend support for managing job concurrency limits, including new data structures, visualizations, and enhanced filtering capabilities.
Key points:
custom_concurrency_keyfor storing concurrency keys on a per job basis. Add the field to the job payload to populate the table on creation of a new job.ConcurrentJobsChartfor visualizing job concurrency.custom_concurrency_key.Generated with ❤️ by ellipsis.dev