-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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] sentry module #761
[ADD] sentry module #761
Conversation
Travis error are: 2017-03-06 08:32:46,628 5693 ERROR openerp_test odoo.modules.module: Module auth_signup_verify_email: 0 failures, 2 errors 2017-03-06 08:32:38,806 5693 INFO openerp_test odoo.addons.mass_editing.tests.test_mass_editing: ` Test if related actions are removed when mass editing record is unlinked. 2017-03-06 08:32:38,893 5693 WARNING openerp_test odoo.models: ir.actions.act_window.create() includes unknown fields: auto_refresh ==== I use with success v8 version of this module in production |
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.
❤️ it! Few minor comments, but otherwise LGTM
.travis.yml
Outdated
@@ -34,7 +34,7 @@ install: | |||
- export PATH=${HOME}/maintainer-quality-tools/travis:${PATH} | |||
- travis_install_nightly | |||
# Install libraries that require specific development headers, but not during lint test | |||
- if ! [ "$LINT_CHECK" = "1" ]; then pip install pymssql MySQL-python pyodbc; fi | |||
- if ! [ "$LINT_CHECK" = "1" ]; then pip install pymssql MySQL-python pyodbc raven; fi |
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.
Please move these to requirements.txt
sentry/README.rst
Outdated
=========== | ||
|
||
Bugs are tracked on `GitHub Issues | ||
<https://github.com/OCA/maintainer-tools/issues>`_. In case of trouble, please |
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.
s/maintainer-tools/server-tools
sentry/__manifest__.py
Outdated
{ | ||
'name': 'Sentry', | ||
'summary': 'Report Odoo errors to Sentry', | ||
'version': '10.0.0.1.0', |
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.
10.0.1.0.0
'odoo.exceptions.UserError', | ||
'odoo.exceptions.ValidationError', | ||
'odoo.exceptions.Warning', | ||
'odoo.exceptions.except_orm', |
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.
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.
Oh and odoo.exceptions.QWebException
Take a look at the below though - maybe we could leverage something like this to make the module a bit more future-ready?
>>> from odoo import exceptions
>>> dir(exceptions)
['AccessDenied', 'AccessError', 'DeferredException', 'MissingError', 'QWebException', 'RedirectWarning', 'UserError', 'ValidationError', 'Warning', '__builtins__', '__doc__', '__file__', '__name__', '__package__', '_logger', 'currentframe', 'except_orm', 'frame_codeinfo', 'logging']
>>> [e for e in dir(exceptions) if not e.startswith('_')]
['AccessDenied', 'AccessError', 'DeferredException', 'MissingError', 'QWebException', 'RedirectWarning', 'UserError', 'ValidationError', 'Warning', 'currentframe', 'except_orm', 'frame_codeinfo', 'logging']
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.
Oh and
odoo.exceptions.QWebException
Hm, QWebException
does not look like a user error so maybe it would make sense to receive such errors?. Also, it's weird that QWebException
is also defined in addons/base/ir/ir_qweb/qweb.py so I guess we should ignore both (if we decide to ignore).
Take a look at the below though - maybe we could leverage something like this to make the module a bit more future-ready?
Do you mean that we should analize the names in odoo.exceptions
and look for Exception
subclasses? IDK, the list of ignored exceptions is configurable, so one could add any new exceptions to the list in the config file. Also, analizing the exceptions at load time would feel a bit magic, i.e. you wouldn't really know which exceptions are ignored. What do you think?
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.
Hmmm I didn't realize there were ignored exceptions here. Is there a reason we don't want to catch all exceptions raised by Odoo?
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.
After thinking for a bit (and drinking a bit of coffee), I think actually the better question is this:
What is the definition of a "User Error" in the context of this app , and how can they be changed ?
Edit: they can be changed with sentry_ignored_exceptions
in Odoo conf. My bad on that miss.
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.
Odoo uses these exceptions as part of flow control and these exceptions are integrated more nicely with the client, eg. in Odoo 8, if you try to add a line to a SO before setting the partner in the SO form, you will get an except_orm
informing about the partner not being set. I think getting such exceptions in Sentry would cause more noise than it would do good, so these are ignored by default (but, as you've noticed, it's configurable with sentry_ignored_exceptions
).
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.
Thanks for the explanation @naglis - works for me!
@tedsalmon please review |
@lasley @tedsalmon any possibilities to merge it soon? We've other nice module based on this that would like to push too. |
@bealdav could help and give review, would like to close this PR. |
👍 |
@bealdav - please use the Github review feature located in the "Files Changed" tab to indicate an official review. I assume the 👍 meant no suggestions, and an approval for merge? |
It makes me so happy to see this merged in OCA. Thank you for the contribution @naglis |
Thanks a lot @naglis |
This merging is provoking an error starting Odoo server if you don't have raven library because of this line: https://github.com/OCA/server-tools/blob/10.0/sentry/const.py#L37 |
Hi @naglis - I am still getting this error. Traceback (most recent call last): |
Thanks @naglis |
* [ADD] sentry module * [FIX] updated sentry module according to PR comments
* [ADD] sentry module * [FIX] updated sentry module according to PR comments
* [ADD] sentry module * [FIX] updated sentry module according to PR comments
* [ADD] sentry module * [FIX] updated sentry module according to PR comments
* [ADD] sentry module * [FIX] updated sentry module according to PR comments
* [ADD] sentry module * [FIX] updated sentry module according to PR comments
* [ADD] sentry module * [FIX] updated sentry module according to PR comments
* [ADD] sentry module * [FIX] updated sentry module according to PR comments
* [ADD] sentry module * [FIX] updated sentry module according to PR comments
* [ADD] sentry module * [FIX] updated sentry module according to PR comments
* [ADD] sentry module * [FIX] updated sentry module according to PR comments
* [ADD] sentry module * [FIX] updated sentry module according to PR comments
* [ADD] sentry module * [FIX] updated sentry module according to PR comments
* [ADD] sentry module * [FIX] updated sentry module according to PR comments
* [ADD] sentry module * [FIX] updated sentry module according to PR comments
Syncing from upstream OCA/server-tools (12.0)
* [ADD] sentry module * [FIX] updated sentry module according to PR comments
* [ADD] sentry module * [FIX] updated sentry module according to PR comments
Hello all,
as discussed in HBEE/odoo_sentry#5, we would like to contribute the Sentry error reporting integration module to OCA.
I have made the following changes in comparison to our fork:
sentry_report_user_errors
was removed).sentry_exclude_loggers
).SanitizeOdooCookiesProcessor
to sanitize sensitive cookies (session_id
) in Odoo side (see also [FIX] fix security issue, as the module leak the cookies into sentry versada/odoo_sentry#6). Also added the ability to configure which processors to use in Odoo config file.sentry_client_dsn
config key renamed to simplysentry_dsn
.Sentry
middleware in order to report errors in HTTP controllers more nicely. (AFAICT, this also requires thesentry
module to be added toserver_wide_modules
list, otherwise, the wrapping does not work.)@barsi, I have included your name as one of the authors in
__manifest__.py
andREADME.rst
but I was not sure if I should include it in the copyright notices in each Python file. Please let me know if you would like me to add it.Regarding Odoo versions 8.0 and 9.0 -- I will be happy to provide backports to both of these versions if/once this is merged.