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.

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

[14.0][MIG] sentry: migrate sentry-raven to new api sentry-sdk #2254

Merged
merged 5 commits into from
Sep 20, 2022

Conversation

paradoxxxzero
Copy link
Contributor

This is a migration done by cherry-picking #1942 on top of current 14.0.
This superseeds #2223

Copy link
Contributor

@legalsylvain legalsylvain left a comment

Choose a reason for hiding this comment

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

partial review.

thanks for porting this must-have update.

sentry/__manifest__.py Outdated Show resolved Hide resolved
"Odoo Community Association (OCA)",
"Odoo Community Association (OCA),"
"Vauxoo",
"maintainers": ["barsi", "naglis", "versada", "moylop260", "fernandahf"],
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there are no rules regarding maintainers quantity, but maybe 5 maintainers is a little too much ?

CC : @OCA/community-maintainers

@paradoxxxzero
Copy link
Contributor Author

Thanks for the partial review.

I added some more fixes as explained in commits, most notably I added one more wsgi patch because as it is the SentryWsgiMiddleware was simply ignored by odoo. This hopefully will fix the performance monitoring.
Of course I'll rebase after review if it is deemed necessary.

@andreschenkels
Copy link
Member

I reviewed the module and it seems to be working fine. Nice information and no longer depending on the raven module!
Please merge

Copy link
Contributor

@hparfr hparfr left a comment

Choose a reason for hiding this comment

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

I'm using it in production since weeks

LGTM

@hparfr
Copy link
Contributor

hparfr commented Mar 18, 2022

Can you please rebase ?

@andreschenkels
Copy link
Member

@legalsylvain @paradoxxxzero can we move this forward? This should also be ported to v13 and v15.

can I help in any way?

@dnplkndll
Copy link

using the traces_sample_rate can context be added to the transaction to reproduce the slow responses?

for instance a use case is if you have known long running calls, like to a shipping api, would like to filter the results to call not made with that certain action. Error traces have all the data. the performance samples do not.

POST /web/dataset/call_button

image

would this be a good place to have an option like inherit_id="web.assets_backend"

                <script
                    src="https://browser.sentry-cdn.com/6.16.1/bundle.min.js"
                    integrity_no="sha384-WkFzsrcXKeJ3KlWNXojDiim8rplIj1RPsCbuv7dsLECoXY8C6Cx158CMgl+O+QKW"
                    crossorigin="anonymous"
                ></script>
                <script
                    src="https://browser.sentry-cdn.com/6.16.1/bundle.tracing.min.js"
                    integrity_no="sha384-hySah00SvKME+98UjlzyfP852AXjPPTh2vgJu26gFcwTlZ02/zm82SINaKTKwIX2"
                    crossorigin="anonymous"
                ></script>
                <script>
                Sentry.init({
                    dsn: "<t t-esc="request.env['ir.config_parameter'].sudo().get_param('ken_erp.sentry_dsn', '14.0')",
                    release: "<t t-esc="request.env['ir.config_parameter'].sudo().get_param('ken_erp.version', '14.0')"/>",
                    integrations: [new Sentry.Integrations.BrowserTracing()],
                    tracesSampleRate: <t t-esc="request.env['ir.config_parameter'].sudo().get_param('ken_erp.sentry_sample_Rate', '0.2')"/>,
                    sendClientReports: false,
                });
                </script>

@thapakazi
Copy link

+1, what needs to be done on this to get it merged ?

@dreispt
Copy link
Member

dreispt commented May 24, 2022

We need a green TravisCI build. That's blocking this right now.

@paradoxxxzero paradoxxxzero force-pushed the 14.0-mig-sentry-sdk branch from 14f83bf to c01547c Compare May 30, 2022 14:55
fernandahf and others added 4 commits May 30, 2022 16:56
Because post_load is called after odoo.service.server start
It has already registered the unpatched   odoo.service.wsgi_server.application
So patch it here too.
This enables wsgi performance reporting with sentry_traces_sample_rate
@paradoxxxzero paradoxxxzero force-pushed the 14.0-mig-sentry-sdk branch from c01547c to 4c413fa Compare May 30, 2022 15:03
@hparfr
Copy link
Contributor

hparfr commented Jun 20, 2022

Can we merge ?

@dnplkndll
Copy link

@paradoxxxzero do you have any idea how to get the header set? there are a number of integration examples now. But not clear to me the best approach here.

https://develop.sentry.dev/sdk/performance/#header-sentry-trace

@hparfr
Copy link
Contributor

hparfr commented Sep 19, 2022

@schiggi @dnplkndll @thapakazi

Can you please mark your review status on this PR ?

@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). 🤖

@sebastienbeau
Copy link
Member

/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 14.0-ocabot-merge-pr-2254-by-sebastienbeau-bump-patch, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit a82db86 into OCA:14.0 Sep 20, 2022
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 4917899. 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.

10 participants