stayallive / wp-sentry

A (unofficial) WordPress plugin reporting PHP and JavaScript errors to Sentry.
https://wordpress.org/plugins/wp-sentry-integration/
MIT License
300 stars 48 forks source link

Adding HttpClient and other integrations #169

Closed cwgw closed 3 months ago

cwgw commented 10 months ago

First of all, thank you for all of the work on this plugin!

I'm interested in adding Sentry's HttpClient integration, but I'm not sure how best to go about it. Do you have any recommendations?

It looks like there isn't currently a good way to add integrations to the plugin's Sentry.init() call. Right now my best ideas for adding an integration are:

  1. Disable the plugin's JS initialization with wp_sentry_hook()and re-implement it myself
  2. Try to use Sentry.getCurrentHub().getClient().addIntegration() as soon as possible after the plugin's initialization

Neither of these options are great. Please do let me know if I'm missing something.

I've seen this subject raised in a handful of issues and discussion topics. I know it's tough to strike a balance between allowing user customizations and providing good defaults. But, I wanted to throw out an idea:

+ var options = typeof wp_sentry_options_hook === "function"
+   ? wp_sentry_options_hook(wp_sentry)
+   : wp_sentry;
+ 
+ Sentry.init(options);
- Sentry.init(wp_sentry);

What if we added a second hook that could modify the object passed to Sentry.init()? This feels like a solidly idiomatic solution for WordPress.

stayallive commented 10 months ago

Hi @cwgw, I'm open to this idea, but you know you also need to require another JS file not bundled in this plugin for that to work?

From the documentation you linked the httpclient.min.js is required for this to work and we don't ship that at this moment. The bunde.tracing.min.js we do ship already of course.

<script
  src="https://browser.sentry-cdn.com/7.74.0/bundle.tracing.min.js"
  integrity="sha384-W2JRP8xn/TuxrYMKju0sE0xobDLaSAQMHidDScxIziQYpGZxKq4+aUb6Z4RQmNEF"
  crossorigin="anonymous"
></script>
<script
  src="https://browser.sentry-cdn.com/7.74.0/httpclient.min.js"
  integrity="sha384-0Yo7MAMkjdUI1H3W/1Qr9b+IkMTy/4DYvs/09VumWi1I0dU6Dn2QhKbM/PBDUXUl"
  crossorigin="anonymous"
></script>

<script>
  Sentry.init({
    dsn: "https://533069e2ff3f4aaa8defbdbac62c3184@o1296477.ingest.sentry.io/4505528807915520",
    integrations: [new Sentry.Integrations.HttpClient()],

    // This option is required for capturing headers and cookies.
    sendDefaultPii: true,
  });
</script>

So I'm open to adding a hook like that since it might be useful in other ways to but I'm not sure that change alone (adding the hook) would help you immediately.

pattonwebz commented 9 months ago

I was also looking at how I might get this added and found there was no straightforward method.

Adding the filter/hook to allow updating options directly doesn't seem to solve this as other work would be needed (including the script firstly but also adjustments to the integrations handling in the js would be required as well). Working through that isn't something I could likely make a PR for right now for my current task but I may try to cycle back at the weekend and figure it out.

kkmuffme commented 8 months ago

It's already easily possible to add this now via function wp_sentry_hook and setting wp_sentry.integrations there, isn't it?

Then you just need to swap out the wp-sentry bundled JS with your JS bundle and you should be good to go.

Am I missing the point or problem?

pattonwebz commented 8 months ago

It's already easily possible to add this now via function wp_sentry_hook and setting wp_sentry.integrations there, isn't it?

Then you just need to swap out the wp-sentry bundled JS with your JS bundle and you should be good to go.

Am I missing the point or problem?

Sadly, it isn't quite so simple since right after you could set the integrations on that hook, they would be overwritten by the casting of wp_sentry.integrations to an empty array just afterwards https://github.com/stayallive/wp-sentry/blob/4c50a711e643df1002dcb81f2342b0513dc05441/public/wp-sentry-browser.wp.js#L29.

stayallive commented 8 months ago

I would not be opposed to moving the wp_sentry_hook to after the integrations init. However wp-sentry-browser.wp.js is not loaded separately (although... that could maybe also be an option...) so replacing the Sentry bundle also removes that code (we append that file to each Sentry bundle to prevent adding 2 JS file request).

So maybe a solution is 2 fold. The first is moving the wp_sentry_hook call to after setting the integrations array. The second is giving an option to not load the bundled Sentry SDK but allow you to set your own bundle file or CDN build and add the wp-sentry-browser.wp.js from the plugin instead. Not sure this solves all the issues but could be an option to make this possible with minimal effort?

kkmuffme commented 8 months ago

@pattonwebz oh right, this is new. I'm using a slightly older version and this change was introduced 10 months ago, so in the version I was using it worked fine (see git blame)

@stayallive could you perhaps change this back so wp_sentry.integrations (and in fact anything else that COULD be set by wp_sentry_hook) is not overriden, if it is set already bc wp_sentry_hook?

Just checking ! in before initiating .integrations = []; would be sufficient and make it work fine - like it did in earlier versions.

stayallive commented 8 months ago

Sure, I just pushed v7.3.1 which contains: ad28d94f5acd7401c70c69c962d4d4282096e632 allowing you to modify the integrations array from inside the hook.

gregfr commented 6 months ago

Greetings! how can I use this integration? I'm not sure if I have to provide the wp_sentry_hook or change wp_sentry.integrations, or something else? Thanks in advance Regards