-
Notifications
You must be signed in to change notification settings - Fork 4.1k
sql: clean up propagation of sql statement into a sentry report #44308
Description
First commit in #43301 fixed an issue that some sentry reports didn't contain the statement that caused the errors because we relied on the contexts to have the correct tags set, and incorrect context was used. This whole "statement propagation infrastructure" is very fragile.
Here is Andrei's description of the problem:
"Having something as important as including the statement in the Sentry reports depend on what context res.Close() is called with does not make sense. It's magic for no reason and fragile for no reason. The caller to res.Close() might as well put structured information in the error for the purposes of the report - for example by using the errors.With<foo>(err) patterns.
Also notice that currently because of the context stuff the reports don't make a distinction between errors encountered by execution of a statement or portal vs errors encountered by a prepare. If we make this caller construct some structured info, it'd be trivial to make that distinction.
On log.Fatal(), on the other hand, I think the magic is justified to get as much info as possible. On panics, I similarly believe the magic is justified, except that it looks to me like on the one panic handler that matters, the magic will actually not work because the context there is not the one with the statement in it. So again I think some structured solution needs to be find. For example recovering panics earlier, somewhere in conn_executor_exec.go, and panicing again with an error that has the statement in it."
Jira issue: CRDB-5238
Metadata
Metadata
Assignees
Labels
Type
Projects
Status