Tracking of SQL queries

alexandre67fr commented 2 years ago

If WordPress query log is enabled (constant SAVEQUERIES), send query logs to Sentry.
This is really useful to debug queries that take too much time, are too complex, or can cause errors.

stayallive commented 2 years ago

Is it required to enable SAVEQUERIES though?

As in can't we hook into the query exection?

It's explicitly mentioned this has a performance impact so I'd rather not advice enabling this if we can solve it another way.

alexandre67fr commented 2 years ago

Hey @stayallive

You're absolutely right that it has a huge performance impact, but it still can be useful in many situations. Especially if website has many plugins, and one wants to know which queries are slow.

Yes, it's required to enable SAVEQUERIES in order to get the execution time of each SQL query. I didn't find any other hook or filter which allows to track execution time.

However, we're not enabling it, but only detecting if it's enabled or not.

In order to prevent any performance issues, I've added a new commit, which introduces a new constant WP_SENTRY_TRACK_SQL.

By default, it's disabled, meaning that no SQL queries are tracked, and the new PHP class is not loaded
If one wants to see SQL log in Sentry, adding define( 'WP_SENTRY_TRACK_SQL', true ); in wp-config.php is required
If one wants to see SQL log in Sentry, as well as execution time for each SQL query, adding define( 'WP_SENTRY_TRACK_SQL', true ); and define( 'SAVEQUERIES', true ); in wp-config.php is required

What do you think?

ocean90 commented 2 years ago

I don't think that Sentry is a good place for logging all the SQL queries. But what I'm using in a few projects is this extension, which only logs query errors and doesn't require SAVEQUERIES:

// Capture SQL errors by wpdb.
add_action(
    'shutdown',
    function(): void {
        global $EZSQL_ERROR;

        if ( empty( $EZSQL_ERROR ) || ! is_array( $EZSQL_ERROR ) || ! function_exists( 'wp_sentry_safe' ) ) {
            return;
        }

        foreach ( $EZSQL_ERROR as $error ) {
            wp_sentry_safe(
                function ( \Sentry\State\HubInterface $client ) use ( $error ): void {
                    $client->withScope(
                        function ( \Sentry\State\Scope $scope ) use ( $client, $error ): void {
                            $scope->setFingerprint( [ $error['error_str'] ] );
                            $scope->setExtra( 'query', $error['query'] );
                            $client->captureMessage(
                                'SQL Error: ' . $error['error_str'],
                                \Sentry\Severity::error()
                            );
                        }
                    );
                }
            );
        }
    }
);

alexandre67fr commented 2 years ago

Hi @ocean90

It’s true that Sentry is not a good place for SQL logging. This is why the feature proposed here is optional. You must define a constant in wp-config.php to enable this feature.

Nevertheless, I’ve seen many production websites with premium plugins which do really useless or slow queries. This is why this feature can be useless.

Once again, this is inspired by Sentry package for Laravel. In the Laravel package, SQL logging is enabled by default.

—-

However, your suggestion to add SQL logs when there’s a fatal error seems interesting. It might be nice to have an additional option for this too. We’d have either log every si gel query or only queries with errors.

kkmuffme commented 2 years ago

Nevertheless, I’ve seen many production websites with premium plugins which do really useless or slow queries. This is why this feature can be useless.

You need a proper profiler like Blackfire or Tideways for that. Logging those in Sentry isn't the right tool for this. If all you have is a hammer, everything looks like a nail ;-)

===

@ocean90 by default WP/wpdb error_log() via wpdb->print_error, which, if you have correctly set up your server, will end up in Sentry via the PHP logger anyway? Could you point out the advantage of your snippet, as I am not sure, I fully understand it.

jasongill commented 2 years ago

I also agree with @alexandre67fr and will be switching to his branch; it's very helpful to be able to track query performance in Sentry, and while there is a performance impact when enabled, the benefits outweigh that slight hit. We intend to just turn this on during troubleshooting or debugging sessions, or possibly just on a small percentage of random pageloads to get a sampling of query issues.

My only suggestion would be to split this into two constants - for example, WP_SENTRY_TRACK_SQL and WP_SENTRY_TRACK_SQL_PERFORMANCE which could automatically enable SAVEQUERIES. That way, the benefits and downsides of both options could be documented and users could pick which level they want - the basic query logging, or the potentially slower query logging + query execution time logging. Or, make WP_SENTRY_TRACK_SQL accept multiple values perhaps (not sure what would be more of the "WordPress way").

Great work @alexandre67fr!

@stayallive please do consider merging this PR; I understand that you may not see Sentry as the ideal place for these queries to be sent, and perhaps it is not - but Sentry supports it and works quite well when you send queries, and being able to track the performance of those queries all in one tool is a big benefit of Sentry. Why not want to support all of the features that their platform allows for, especially when this PR is so simple (no major change to the codebase required, luckily). Thanks for your work on this plugin, really appreciate your contributions!

stayallive commented 2 years ago

For the record. I know very well what Sentry is capable of since I also contribute to the PHP, Laravel and Symfony SDKs and we have full performance tracing capabilities there.

I am not disputing query tracing (or any kind of performance tracing) is a bad thing. It should just be implemented with as little overhead as possible (and yes, there is always a little overhead of course) and I don't see why there isn't a lighter weight solution for tracking queries and the execution time and why setting SAVEQUERIES is needed. Until I have the time to look into this better or someone does the research I am not willing to merge it yet at this point.

Thanks everyone for commenting and sharing your ideas and findings, this will for sure make the final implementation a good one 🤘

androidacy-user commented 1 year ago

Just fyi, testing this patch out on wordpress 6.0.3 causes wordpress to crash very early on - early enough that WP_DEBUG doesn't do anything

gregfr commented 5 months ago

Now that Sentry introduced metrics (https://sentry.io/resources/launch-week-mar24-day1), how does it impact this feature?

stayallive commented 5 months ago

No impact. This feature and it's implementation is dependent on the WordPress code and the impact of tracking SQL queries on the site it's running on.

Metrics can be used but it's more likely to be just implemented as performance tracing which has been available for some time like other Sentry SDK's do.

But in both cases queries need to be tracked so not sure how metrics would impact that at all.

gregfr commented 5 months ago

Thanks for the fast answer! I was able to activate the metrics, I'll keep on digging :)

stayallive commented 2 months ago

Tracing is now released, so I am closing this PR since it's implemented via there: https://github.com/stayallive/wp-sentry/releases/tag/v7.17.0

Thanks again for the discussions and taking the time to open the PR <3

stayallive / wp-sentry

Tracking of SQL queries #127