Allow embedding SQL query bindings (parameters) in SQL query breadcrumbs and spans #932
Conversation
|
|
||
| - Allow embedding SQL query bindings (parameters) in SQL query breadcrumbs and spans. | ||
|
|
||
| To enable this feature, update your `config/sentry.php` file or set the `SENTRY_TRACE_SQL_BINDINGS_ENABLED` and `SENTRY_BREADCRUMBS_SQL_BINDINGS_ENABLED` environment variables to `embed`. |
There was a problem hiding this comment.
I considered the addition of an additional boolean configuration option for this, but found that this complicated the code and made it unclear which option had precedence over the existing options. However, I could see reverting back to making the options just a boolean and simply adopting the new behaviour?
| @@ -58,7 +58,7 @@ | |||
| // Capture SQL queries as breadcrumbs | |||
| 'sql_queries' => env('SENTRY_BREADCRUMBS_SQL_QUERIES_ENABLED', true), | |||
|
|
|||
| // Capture SQL query bindings (parameters) in SQL query breadcrumbs | |||
| // Capture bindings in SQL query breadcrumbs. `true` or `append` shows them separate, `embed` inlines them | |||
There was a problem hiding this comment.
I shortened the prior part of the description to fit everything on a single short line still.
|
I am not sure what to do about the failing integration test in https://github.com/getsentry/sentry-laravel/actions/runs/10077108575/job/27858895008?pr=932. |
There was a problem hiding this comment.
This breaks our Querries Insights Module, as it only works with parameterized queries.
We could debate adding the raw query to span data, but I currently don't see it that useful given the amount of changes required.
|
I think it would be massively useful to aid readability. I personally find it hard to decipher what is going on in the following example:
The inlined version is much more readable to me. update `mll_app_exa_type_hdr_link` set `expt_valid_to` = 20240724140225, `expt_change_user` = 'bfranke' where `expt_id` = '62A2C0AB-D321-4C6F-9675-4EF2AC77BD34' and `expt_valid_to` = 99991231235555Of course, this problem compounds as queries get more complicated and the number of bindings grows. Queries Insights works with spans right? I would be more interested in breadcrumbs anways, maybe we could just make the change there and omit the extra option for spans? |
|
This is something we can fix in the product, we do not need to add more logic to the SDK. The data is already there. cc @gggritso |
|
There are individual differences depending on the database or its configuration in how bindings are embedded, which is why the code that builds the raw SQL involves the |
|
There are also concerns with server-side Pii stripping. Scrubbing individual data attributes is much easier than scrubbing a string, and I'm not sure we currently scrub breadcrumb descriptions. So a lot of reasons why I don't feel too happy about this change. |
|
Are bindings such as the ones in the screenshot in #932 (comment) scrubbed? Seems difficult, given it is in the form of a list without keys that could indicate sensitive data. |
|
Understood. Thanks for providing detailed feedback and discussing this pull request with me. I might try formulating a feature request over at https://github.com/getsentry/sentry. |
|
We already looking into this, will hack something together next week. It’s a bit more involved but we want to have this feature 🙂 |
I added a similar feature to Laravel in laravel/framework#52192.