Skip to content

New feature / filter - Filter events sent to Sentry#179

Closed
herewithme wants to merge 1 commit intostayallive:masterfrom
BeAPI:feature/filtering
Closed

New feature / filter - Filter events sent to Sentry#179
herewithme wants to merge 1 commit intostayallive:masterfrom
BeAPI:feature/filtering

Conversation

@herewithme
Copy link
Contributor

Sentry allows you to filter events in its SDK:
https://docs.sentry.io/platforms/php/configuration/filtering/

This would be very useful, because sometimes warnings in the project come from third-party plugins and it's not always possible to patch them.

With the addition of a filter, this gives all the possibilities for advanced users.

@herewithme
Copy link
Contributor Author

I forgot to thank you for your work on the plugin :)

@stayallive
Copy link
Owner

You can already do this: https://github.com/stayallive/wp-sentry?tab=readme-ov-file#wp_sentry_options (you can set any option from this hook).

Thanks for taking the time to make a PR but I'd rather not add more options if we can help it 😅

@stayallive stayallive closed this Feb 24, 2024
@herewithme
Copy link
Contributor Author

Hello,

Actually, I find the wp_sentry_options hook quite useless because it executes too late. The vast majority of the code executes before, all the warnings linked to the inclusion of the source code, and all the code that initializes itself, especially in the "plugins_loaded" hook.

I find that there is a certain contradiction in the fact of proposing various means of loading your plugin as soon as possible (config, mu-plugins), and the impossibility of adjusting the Sentry SDK configuration.

With this filter, it's possible to intercept events/errors at an early stage.

Can you reconsider? :)

@herewithme
Copy link
Contributor Author

ping @stayallive (the PR stay closed)

@stayallive
Copy link
Owner

stayallive commented Feb 27, 2024

Hmm. You make a fair point... I do want to explore another option though... how about:

define( 'WP_SENTRY_PHP_OPTIONS', [
    'before_send' => function (\Sentry\Event $event): ?\Sentry\Event {
		return $event;
	},
    // any other options from: https://docs.sentry.io/platforms/php/configuration/options/
] );

This would cleanup a lot of other constant too (I'll keep them to not break other people's apps ofcourse) but could make the configuration a lot more simple without having a constant for every option possible 😅 I believe this was not possible when I created the plugin because of older PHP version but this seems perfectly legal in PHP 7+ which we only support.

What do you think?

@herewithme
Copy link
Contributor Author

Yes, it's a good approach, it's more generic, and there's no impact on performance if the constant isn't defined.

The drawback is that in wp-config.php, you can't use apply_filters.

For my purposes, I could do without it, but I find it impractical.
This could be a first improvement :)

@stayallive
Copy link
Owner

I'm not sure why but I seem to had the impression you were adding a new constant config option... I still think WP_SENTRY_PHP_OPTIONS is a good option but that doesn't make you suggestion an invalid one either. So let's merge it!

I can't push to your branch because it was made inside a org. So I've opened #180 with your original commit and my changes on top of that.

@stayallive stayallive closed this Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

Comments