Skip to content

Conversation

@thomaspaulb
Copy link

@thomaspaulb thomaspaulb commented Jan 8, 2021

Current profiling module only works with workers=0 so it is not useful in production.

This patch adds a separate Python profiling mode 'Per request', which generates a cProfile stats file for each individual HTTP request of the user session that enabled it. This works in multi-worker mode also, and allows to see profiling data for the slow(est) requests only.

Now profiling can also be used in production with low overhead.

image

@moylop260 @etobella

@moylop260
Copy link
Contributor

LGTM

@thomaspaulb thomaspaulb force-pushed the 11.0-profiler-per-request branch 3 times, most recently from b3d4991 to 48a41ff Compare January 10, 2021 02:18
@thomaspaulb thomaspaulb changed the title [ADD] New mode of profiling which works with multiple workers. [11.0][IMP] profiler: 'Per HTTP request' mode, works in multi-worker mode. Jan 10, 2021
@thomaspaulb thomaspaulb changed the title [11.0][IMP] profiler: 'Per HTTP request' mode, works in multi-worker mode. [11.0][IMP] profiler: 'Per HTTP request' mode Jan 10, 2021
Copy link
Contributor

@NL66278 NL66278 left a comment

Choose a reason for hiding this comment

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

👍 LGTM. One nag, but not crucial

@thomaspaulb thomaspaulb force-pushed the 11.0-profiler-per-request branch from 48a41ff to 29b8a06 Compare January 14, 2021 17:36
@thomaspaulb
Copy link
Author

@NL66278 Fixed. Thanks for the review.

Copy link
Contributor

@NL66278 NL66278 left a comment

Choose a reason for hiding this comment

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

👍 LGTM

@etobella
Copy link
Member

I have tried to check it on runbot and it happened somehting weird 😞
1- I created a profiler with the request method
2- I enabled it
3- I did something
4- I disabled it -> The result was as expected

It was great so far. If I tried to enable again the Profiler an error was raised. The same error was raised if I tried to enable a new profiler

Error:
Odoo Server Error

Traceback (most recent call last):
  File "/usr/lib/python3.5/inspect.py", line 1088, in getfullargspec
    sigcls=Signature)
  File "/usr/lib/python3.5/inspect.py", line 2157, in _signature_from_callable
    raise TypeError('{!r} is not a callable object'.format(obj))
TypeError: False is not a callable object

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/.repo_requirements/odoo/odoo/http.py", line 653, in _handle_exception
    return super(JsonRequest, self)._handle_exception(exception)
  File "/.repo_requirements/odoo/odoo/http.py", line 312, in _handle_exception
    raise pycompat.reraise(type(exception), exception, sys.exc_info()[2])
  File "/.repo_requirements/odoo/odoo/tools/pycompat.py", line 87, in reraise
    raise value
  File "/.repo_requirements/odoo/odoo/http.py", line 695, in dispatch
    result = self._call_function(**self.params)
  File "/home/odoo/build/OCA/server-tools/profiler/hooks.py", line 27, in webreq_f
    return webreq_f_origin(*args, **kwargs)
  File "/.repo_requirements/odoo/odoo/http.py", line 344, in _call_function
    return checked_call(self.db, *args, **kwargs)
  File "/.repo_requirements/odoo/odoo/service/model.py", line 97, in wrapper
    return f(dbname, *args, **kwargs)
  File "/.repo_requirements/odoo/odoo/http.py", line 337, in checked_call
    result = self.endpoint(*a, **kw)
  File "/.repo_requirements/odoo/odoo/http.py", line 939, in __call__
    return self.method(*args, **kw)
  File "/.repo_requirements/odoo/odoo/http.py", line 517, in response_wrap
    response = f(*args, **kw)
  File "/home/odoo/OCB-11.0/addons/web/controllers/main.py", line 939, in call_button
    action = self._call_kw(model, method, args, {})
  File "/home/odoo/OCB-11.0/addons/web/controllers/main.py", line 927, in _call_kw
    return call_kw(request.env[model], method, args, kwargs)
  File "/.repo_requirements/odoo/odoo/api.py", line 699, in call_kw
    return call_kw_multi(method, model, args, kwargs)
  File "/.repo_requirements/odoo/odoo/api.py", line 687, in call_kw_multi
    context, args, kwargs = split_context(method, args, kwargs)
  File "/.repo_requirements/odoo/odoo/api.py", line 304, in split_context
    pos = len(getargspec(method).args) - 1
  File "/usr/lib/python3.5/inspect.py", line 1042, in getargspec
    getfullargspec(func)
  File "/usr/lib/python3.5/inspect.py", line 1094, in getfullargspec
    raise TypeError('unsupported callable') from ex
TypeError: unsupported callable

@thomaspaulb
Copy link
Author

@etobella Big thanks for finding this silly bug. I was overwriting the enable() routine with False.

@thomaspaulb
Copy link
Author

I've also removed "attachment_id" from the tree view of requests; with many requests piling up in the list, it would do a lot of name_get requests for each and profile them all, which could be slow.

@OCA-git-bot
Copy link
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). 🤖

@moylop260
Copy link
Contributor

/ocabot merge major

@OCA-git-bot
Copy link
Contributor

What a great day to merge this nice PR. Let's do it!
Prepared branch 11.0-ocabot-merge-pr-1986-by-moylop260-bump-major, awaiting test results.

OCA-git-bot added a commit that referenced this pull request Jan 28, 2021
Signed-off-by moylop260
@OCA-git-bot
Copy link
Contributor

@moylop260 your merge command was aborted due to failed check(s), which you can inspect on this commit of 11.0-ocabot-merge-pr-1986-by-moylop260-bump-major.

After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red.

…erates a stats file for each request separately.
@thomaspaulb thomaspaulb force-pushed the 11.0-profiler-per-request branch from e7e16ef to 50ce2b4 Compare January 28, 2021 17:46
@thomaspaulb
Copy link
Author

@moylop260 Can you try again? It was a timing error in the tests

@moylop260
Copy link
Contributor

/ocabot merge major

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 11.0-ocabot-merge-pr-1986-by-moylop260-bump-major, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 5b3616a into OCA:11.0 Jan 28, 2021
@OCA-git-bot
Copy link
Contributor

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

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.

6 participants