Skip to content

queue_job: add exec time to view some stats#309

Merged
OCA-git-bot merged 1 commit into
OCA:13.0from
camptocamp:13-add-exec_time
Mar 4, 2021
Merged

queue_job: add exec time to view some stats#309
OCA-git-bot merged 1 commit into
OCA:13.0from
camptocamp:13-add-exec_time

Conversation

@simahawk

@simahawk simahawk commented Feb 8, 2021

Copy link
Copy Markdown
Contributor

No description provided.

Comment thread queue_job/models/queue_job.py Outdated
@simahawk

simahawk commented Feb 9, 2021

Copy link
Copy Markdown
Contributor Author

@guewen I guess the test is wrong here because date_started in theory should be there always if you are calling set_done, right? It should call set_started before. See https://travis-ci.com/github/OCA/queue/jobs/481537133#L1082

@guewen

guewen commented Feb 9, 2021

Copy link
Copy Markdown
Member

I guess the test is wrong here because date_started in theory should be there always if you are calling set_done, right? It should call set_started before

It's not wrong, for this test, the initial state started had no impact on set_done. It's a unit test, it doesn't test the full flow.
You added a new dependency (date_started) in this unit, so you have to be sure the initial state of the test set it correctly.
You shouldn't call set_started before, because then the test would test set_started and set_done.
You only need to set job_a.date_started = xxx as this is the initial state you miss.

@simahawk

simahawk commented Feb 9, 2021

Copy link
Copy Markdown
Contributor Author

better?

@guewen guewen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, thanks!! ❤️

@sebalix sebalix left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Small comment, approving

Comment thread queue_job/models/queue_job.py Outdated

@etobella etobella left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Code Review

@simahawk simahawk force-pushed the 13-add-exec_time branch 2 times, most recently from b84fc86 to 14862e2 Compare February 10, 2021 09:35
Comment thread queue_job/models/queue_job.py Outdated
<field name="exec_time" type="measure" />
</graph>
</field>
</record>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think a pivot view gives better information:

<pivot string="Queue Job">
    <field name="exec_time" type="measure"/>
    <field name="job_function_id" type="row"/>
    <field name="date_done" type="col" interval="week"/>
</pivot>

Example:
Example of pivot view

Note: in this example, I used group_operator="avg" on the field, which I'm not sure we want.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The average is really cool on this view as we can see if it grows over time...

@simahawk simahawk Feb 10, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

looks good. At least is a great start!

@guewen

guewen commented Feb 10, 2021

Copy link
Copy Markdown
Member

Pushed a fixup for pivot view + group by avg by default. I added a mention in string and help for the avg to prevent confusion.

@OCA-git-bot

Copy link
Copy Markdown
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@guewen

guewen commented Mar 4, 2021

Copy link
Copy Markdown
Member

Hi @simahawk :)

I squashed my commit and rebased, can I remove the work in progress label? I'd like to merge if the build is green.

@guewen

guewen commented Mar 4, 2021

Copy link
Copy Markdown
Member

I squashed my commit and rebased, can I remove the work in progress label? I'd like to merge if the build is green.

Oh no sorry, I couldn't push it because I do no longer have push writes here 😆
Could you do it?

@simahawk

simahawk commented Mar 4, 2021

Copy link
Copy Markdown
Contributor Author

welcome back! :)
It's done!

Comment thread queue_job/migrations/13.0.3.6.0/pre-migration.py
@guewen

guewen commented Mar 4, 2021

Copy link
Copy Markdown
Member

/ocabot merge nobump

@OCA-git-bot

Copy link
Copy Markdown
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 13.0-ocabot-merge-pr-309-by-guewen-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 4e7d0ef into OCA:13.0 Mar 4, 2021
@OCA-git-bot

Copy link
Copy Markdown
Contributor

Congratulations, your PR was merged at 9bf3f24. Thanks a lot for contributing to OCA. ❤️

guewen pushed a commit to guewen/queue that referenced this pull request Oct 24, 2022
lmignon pushed a commit to acsone/queue that referenced this pull request Nov 16, 2022
lmignon pushed a commit to acsone/queue that referenced this pull request Nov 16, 2022
lmignon pushed a commit to acsone/queue that referenced this pull request Nov 16, 2022
lmignon pushed a commit to acsone/queue that referenced this pull request Nov 16, 2022
bizzappdev pushed a commit to BizzAppDev-Systems/queue that referenced this pull request Nov 23, 2023
bizzappdev pushed a commit to BizzAppDev-Systems/queue that referenced this pull request Nov 23, 2023
nguyenminhchien pushed a commit to nguyenminhchien/queue that referenced this pull request Nov 25, 2023
yibudak pushed a commit to yibudak/queue that referenced this pull request Nov 27, 2023
QuocDuong1306 pushed a commit to QuocDuong1306/queue that referenced this pull request Sep 19, 2024
QuocDuong1306 pushed a commit to QuocDuong1306/queue that referenced this pull request Sep 24, 2024
thienvh332 pushed a commit to thienvh332/queue that referenced this pull request Oct 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants