-
Notifications
You must be signed in to change notification settings - Fork 444
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
Add concurrency limits observability #3586
Conversation
Deploying windmill with Cloudflare Pages
|
baed65d
to
97eb76b
Compare
44529f6
to
ea4165d
Compare
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
7bc2932
to
42aceed
Compare
There was a problem hiding this 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 42aceed in 3 minutes and 30 seconds
More details
- Looked at
4104
lines of code in52
files - Skipped
0
files when reviewing. - Skipped posting
0
drafted 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider handling input changes more reactively without using a timeout to avoid potential race conditions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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 6477171 in 2 minutes and 5 seconds
More details
- Looked at
427
lines of code in11
files - Skipped
0
files when reviewing. - Skipped posting
0
drafted 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.
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 401166d in 2 minutes and 23 seconds
More details
- Looked at
7
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ Changes requested. Incremental review on 4f9b606 in 2 minutes and 25 seconds
More details
- Looked at
61
lines of code in4
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted 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_idx
before 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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 df7d2c7 in 2 minutes and 21 seconds
More details
- Looked at
72
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. frontend/src/lib/components/runs/JobLoader.svelte:194
- Draft comment:
ThefetchConcurrencyIntervals
function is consistently called withundefined
for thestartedAfter
parameter. 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.
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 a51a5b0 in 2 minutes and 30 seconds
More details
- Looked at
259
lines of code in5
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. frontend/src/lib/components/FlowMetadata.svelte:1
- Draft comment:
The PR description mentions adding a new tablecustom_concurrency_key
for 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:
ConcurrentJobsChart
for 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_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.ConcurrentJobsChart
for visualizing job concurrency.custom_concurrency_key
.Generated with ❤️ by ellipsis.dev