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.

Sign up for GitHub

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

Jump to bottom

[TaskProcessing] Add start, stop and schedule time to tasks #46359

Merged
merged 5 commits into from
Jul 23, 2024
Merged

[TaskProcessing] Add start, stop and schedule time to tasks #46359

merged 5 commits into from
Jul 23, 2024

Conversation

julien-nc
Copy link
Member

@julien-nc julien-nc commented Jul 8, 2024 •
edited
Loading

  • Check the values are set everywhere it makes sense during the task lifecycle
  • Decide how to use those values. Should there be an occ command to generate a report? An API endpoint?
    • new taskprocessing:task:list occ command
    • new taskprocessing:task:stats occ command

Both commands accept those filters:

  • userId
  • task type
  • custom ID
  • status
  • scheduled after
  • ended before

@julien-nc julien-nc added this to the Nextcloud 30 milestone Jul 8, 2024
@julien-nc julien-nc force-pushed the enh/task-processing-more-datetimes branch 3 times, most recently from 8c78b7f to 6c2c31a Compare July 9, 2024 10:17
@marcelklehr
Copy link
Member

Should there be an occ command to generate a report?

Can we hook into the system report command? If not, I'd vote for a separate command

@julien-nc julien-nc force-pushed the enh/task-processing-more-datetimes branch from 6c2c31a to 431cc79 Compare July 15, 2024 11:31
github-advanced-security[bot]
github-advanced-security bot found potential problems Jul 15, 2024
View reviewed changes
lib/private/TaskProcessing/Manager.php Fixed Show fixed Hide fixed
lib/private/TaskProcessing/Manager.php Fixed Show fixed Hide fixed
core/Command/TaskProcessing/ListCommand.php
parent::__construct();
}

protected function configure() {

Check notice

Code scanning / Psalm

MissingReturnType Note

Method OC\Core\Command\TaskProcessing\ListCommand::configure does not have a return type, expecting void
core/Command/TaskProcessing/Statistics.php
parent::__construct();
}

protected function configure() {

Check notice

Code scanning / Psalm

MissingReturnType Note

Method OC\Core\Command\TaskProcessing\Statistics::configure does not have a return type, expecting void
@julien-nc julien-nc force-pushed the enh/task-processing-more-datetimes branch 2 times, most recently from 9a6183a to f69cf17 Compare July 15, 2024 11:44
@julien-nc
Copy link
Member Author

Let's make occ commands. Updates are in the PR initial comment.

marcelklehr
marcelklehr reviewed Jul 16, 2024
View reviewed changes
core/Command/TaskProcessing/Statistics.php
$averageUserWaitingTime = (int)($totalUserWaitingTime / $userWaitingTimeCount);
$stats['Average user waiting time'] = $averageUserWaitingTime;
$stats['User waiting time count'] = $userWaitingTimeCount;
}
Copy link
Member

Choose a reason for hiding this comment

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

Mmh, using SQL directly may be faster and a bit more idomatic, perhaps. Not sure if it's possible, though.

Copy link
Member

Choose a reason for hiding this comment

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

bah, we can do that if it becomes relevant, I guess. This is probably good enough for now

@marcelklehr
Copy link
Member

In our brainstorm we also had the idea to maybe record the I/O size. E.g. we could record the byte size of the JSON encoded input, as well as the output. What do you think?

@julien-nc
Copy link
Member Author

In our brainstorm we also had the idea to maybe record the I/O size. E.g. we could record the byte size of the JSON encoded input, as well as the output. What do you think?

Fine by me. So we can take the size of the JSON but we also need the size of the input/output files, right?
It can then become very expensive to compute.

@marcelklehr
Copy link
Member

Perhaps let's leave the files out for now?

@julien-nc julien-nc force-pushed the enh/task-processing-more-datetimes branch 2 times, most recently from cee79b5 to 5c8bbd9 Compare July 22, 2024 11:07
@marcelklehr
Copy link
Member

Both commands accept those filters:

Perhaps also an app filter?

@julien-nc julien-nc force-pushed the enh/task-processing-more-datetimes branch from 2076e99 to 0f2da49 Compare July 23, 2024 09:07
@julien-nc julien-nc marked this pull request as ready for review July 23, 2024 09:08
marcelklehr
marcelklehr reviewed Jul 23, 2024
View reviewed changes
lib/private/TaskProcessing/Db/TaskMapper.php
$qb->andWhere($qb->expr()->isNotNull('ended_at'));
$qb->andWhere($qb->expr()->lt('ended_at', $qb->createPositionalParameter($endedBefore, IQueryBuilder::PARAM_INT)));
}
return array_values($this->findEntities($qb));
Copy link
Member

Choose a reason for hiding this comment

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

Should we have db indeces for these?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think since the amount of task is limited by the cleanup bg job, it's fine. Feel free to add indices in this branch if you think it's necessary.

@julien-nc
feat(taskprocessing): add start, stop and schedule time to tasks
df086a8 
Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
@julien-nc
feat(taskprocessing): add occ commands to list tasks and compute stats
c120a64 
Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
@julien-nc
feat(taskprocessing): add stats about input/output sizes
af21f7d 
Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
@julien-nc
feat(taskprocessing): add appId filter to taskprocessing occ commands
df11aa9 
Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
@julien-nc
fix(taskprocessing): build autoloader files and fix psalm issue
c004f53 
Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
@julien-nc julien-nc force-pushed the enh/task-processing-more-datetimes branch from 70f7a73 to c004f53 Compare July 23, 2024 15:13
bigcat88
bigcat88 approved these changes Jul 23, 2024
View reviewed changes
Copy link
Member

@bigcat88 bigcat88 left a comment

Choose a reason for hiding this comment

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

great changes, you will be able to see statistics on average how long each task was executed. nice!

@julien-nc julien-nc enabled auto-merge July 23, 2024 15:36
@julien-nc julien-nc merged commit f9d4bec into master Jul 23, 2024
167 checks passed
@julien-nc julien-nc deleted the enh/task-processing-more-datetimes branch July 23, 2024 16:55
@blizzz blizzz mentioned this pull request Jul 24, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marcelklehr marcelklehr marcelklehr approved these changes

@bigcat88 bigcat88 bigcat88 approved these changes

@kyteinsky kyteinsky Awaiting requested review from kyteinsky

Assignees
No one assigned
Labels
enhancement feature: text processing
Projects
None yet
Milestone
Nextcloud 30
Development

Successfully merging this pull request may close these issues.
3 participants
@julien-nc @marcelklehr @bigcat88