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

[12.0] [IMP] sentry: migrate sentry-raven to new api sentry-sdk #1942

Merged
merged 1 commit into from
Jun 28, 2021

Conversation

fernandahf
Copy link
Contributor

@fernandahf fernandahf commented Nov 30, 2020

Description of the issue/feature this PR addresses:

Current behavior before PR: events sent for sentry-raven with context, excluding loggers, exceptions and filtering sense data. In fact, Sentry warns about migrate to new api:

image

Desired behavior after PR is merged: events sent for sentry-sdk with context, excluding loggers, exceptions and filtering sensitive data, show events in sentry sdk without warning about update api. Sentry just shows sentry-sdk version without warning migrate.

image

--
I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr

@fernandahf fernandahf changed the title [12.0] Migrate sentry-raven to new api sentry-sdk [12.0] [IMP] sentry: migrate sentry-raven to new api sentry-sdk Nov 30, 2020
sentry/__init__.py Outdated Show resolved Hide resolved
sentry/__init__.py Outdated Show resolved Hide resolved
sentry/__manifest__.py Show resolved Hide resolved
@moylop260
Copy link
Contributor

Could you check if we can remove raven entirely as dependency, please?

@moylop260
Copy link
Contributor

@barsi @naglis @versada
We are migrating the new api of sentry
Could you check it, please?

It is still WIP but if you can get a look here we will grateful

@moylop260
Copy link
Contributor

For the record,
A good improvement I saw using sentry-sdk instead of sentry-raven is it is adding a tag with the hash of the trace

  • Screen Shot 2020-11-30 at 20 18 57

I had needed before to have this value and I just use the following PR to add it manually:

Now it is auto-generated.

@moylop260
Copy link
Contributor

moylop260 commented Dec 1, 2020

For the record,
Another improvement:

  • Before of this PR USER: IP Address 127.0.0.1
  • After this PR USER: IP Address "REAL-PUBLIC-IP"

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.

Thanks for this great and needed work.
Didn't had time to test it yet, some remarks from the code review.

sentry/__init__.py Outdated Show resolved Hide resolved
sentry/const.py Outdated Show resolved Hide resolved
sentry/processor.py Outdated Show resolved Hide resolved
sentry/__init__.py Outdated Show resolved Hide resolved
@fernandahf fernandahf force-pushed the 12.0-migsentrynewapi-fer branch 8 times, most recently from fa685db to 12d0d72 Compare December 3, 2020 02:37
sentry/logutils.py Outdated Show resolved Hide resolved
sentry/processor.py Outdated Show resolved Hide resolved
sentry/__init__.py Outdated Show resolved Hide resolved
@fernandahf fernandahf force-pushed the 12.0-migsentrynewapi-fer branch 7 times, most recently from 2d31c18 to 585ade3 Compare December 8, 2020 15:19
@max3903 max3903 added this to the 12.0 milestone Feb 4, 2021
@hoangtrann hoangtrann mentioned this pull request Mar 25, 2021
@hoangtrann
Copy link

Any updates on this guys?

@moylop260
Copy link
Contributor

@hoangtrann

We are waiting for approves

I mean, we need almost 2 approves to install and configure this module with this new change and do feedback or approve.

Fell free to test it, please

@ryantran-novobi
Copy link

I have deployed and ran this PR successfully on Odoo instance running on Odoo.sh

ping @moylop260 @hparfr @fernandahf

@kirtangajjar
Copy link

I tested this PR. It works fine on my local. I'm able to see the errors in sentry, however, I'm unable to see anything in the performance screen. Is it expected?

@hoangtrann
Copy link

image

Not sure if this is relavant, but how could we enhance the presentation of model object in sentry, like sale.order(124292,) or sale.order(124292, 124293)

@fernandahf @moylop260

@fernandahf
Copy link
Contributor Author

@kirtangajjar

What is the performance screen? I searched but didn't find it.

Regards.

@kirtangajjar
Copy link

@fernandahf Check the below screenshot.

Screenshot from 2021-04-20 11-02-41

Here are the link to official docs:

https://docs.sentry.io/product/performance/
https://docs.sentry.io/product/performance/getting-started/

@kirtangajjar
Copy link

@fernandahf Did you get time to look at it?

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.

Thanks

@themreza
Copy link

What's the status here?

@fernandahf
Copy link
Contributor Author

@themreza

We are waiting for approves

I mean, we need almost 1 approve to install and configure this module with this new change and do feedback or approve.

Fell free to test it, please

@themreza
Copy link

themreza commented Jun 7, 2021

I tested the code in this branch on Odoo 14 and I can confirm that issues are being correctly sent to Sentry.

image

Although the performance feature is sending data, it's really vague and rather pointless.

image

I intentionally ran some excessively slow requests to see the trace, ideally starting from the HTTP request all the way to the database query or the compute methods responsible, but no details were included:

image

Also, there is no frontend tracking via JavaScript:

image

I'm not sure if it's a bug, but when I added sentry_traces_sample_rate to odoo.conf, the parsed value was str rather than float.

image

P.S.: It wasn't working at first which threw me off for a while, but I later figured out it was because I was running Odoo via PyCharm's debug mode and it didn't send anything.

@themreza
Copy link

themreza commented Jun 8, 2021

I figured out the frontend issue and performance tracking.

With a simple template it's possible to inject Sentry's JavaScript bundle:

<?xml version="1.0" encoding="UTF-8"?>
<odoo>
    <template id="assets_backend" name="sentry_javascript_assets" inherit_id="web.assets_backend">
        <xpath expr="." position="inside">
            <script
                    src="https://browser.sentry-cdn.com/6.5.1/bundle.tracing.min.js"
                    integrity="sha384-TkhX8dCdk4BiV9tdHwvmIHWhdvw+LGo0SrMWTTBZ2T/sAUnxQWPuRiXELaaiZT6A"
                    crossorigin="anonymous">
            </script>
            <script>
                Sentry.init({
                  dsn: "YOUR_DSN",

                  // This enables automatic instrumentation (highly recommended), but is not
                  // necessary for purely manual usage
                  integrations: [new Sentry.Integrations.BrowserTracing()],

                  // To set a uniform sample rate
                  tracesSampleRate: 0.2
                });
            </script>
        </xpath>
    </template>
</odoo>

image

I believe if we enable propagate_traces option, we can link the server-sided traces to frontend ones.

But still, the backend traces are not offering a detailed breakdown of what the request consisted of.

@ryantran-novobi
Copy link

@moylop260 would you mind taking a look at this one? I believe that all the discussions are resolved and it's waiting for your approval so far

@moylop260
Copy link
Contributor

@ryantran-novobi

Let's do it

@moylop260
Copy link
Contributor

/ocabot merge major

@OCA-git-bot
Copy link
Contributor

This PR looks fantastic, let's merge it!
Prepared branch 12.0-ocabot-merge-pr-1942-by-moylop260-bump-major, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit a5eb384 into OCA:12.0 Jun 28, 2021
@OCA-git-bot
Copy link
Contributor

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

@moylop260 moylop260 deleted the 12.0-migsentrynewapi-fer branch June 29, 2021 00:26
@codeagencybe
Copy link
Member

Is this issue already resolved and ready to use now on v14?
I just downloaded the latest version and installed and it reports to me I need to upgrade the SDK.
But this issue seems to have resolved that issue?
Any feedback please?

@fernandahf
Copy link
Contributor Author

Hi @codeagencybe

It's not in forward-ported yet, but there is one PR related to v14.0

#1976

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