Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sql: add descriptor dumps to sentry reports during validation issues #57436

Open
jordanlewis opened this issue Dec 3, 2020 · 8 comments
Open
Labels
A-schema-descriptors Relating to SQL table/db descriptor handling. C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)

Comments

@jordanlewis
Copy link
Member

jordanlewis commented Dec 3, 2020

Sentry reports about failed descriptor validations are very unactionable. For example, see #57376.

Unactionable sentry reports are the worst. We should improve this situation by adding a zipped, anonymized descriptor payload to the report.

Sentry reports have a maximum payload size of 200 kilobytes, however, so we have to be judicious.

@knz, can you please explain how we can add an arbitrary payload to a sentry report at error report time?

Jira issue: CRDB-3534

@jordanlewis jordanlewis added the C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) label Dec 3, 2020
@jlinder jlinder added the T-sql-schema-deprecated Use T-sql-foundations instead label Jun 16, 2021
@postamar
Copy link
Contributor

postamar commented Mar 8, 2022

Weren't we going to use something called event-based logging or something like that to deal with this?

@ajwerner
Copy link
Contributor

ajwerner commented Mar 8, 2022

I think event based logging is totally orthogonal to this. This is about crash reports having more info. At least, I don't think anybody else was thinking about the two being related.

@knz
Copy link
Contributor

knz commented Mar 9, 2022

can you please explain how we can add an arbitrary payload to a sentry report at error report time?

After #77525 is merged, one can simply create a custom error wrapper with some descriptor payload(s), then implement a SafeFormatError that includes these descriptors when formatting the error verbosely.

I would not recommend including all the descriptors in the error btw. Error objects flow through many places in the system and the last thing we want is significant memory usage in objects that are not accounted for...

@postamar
Copy link
Contributor

Agreed. We'll want to redact descriptor protos to keep them below a constant upper bound in byte size, as should be the case for any user-facing message really. We can hope that most descriptors are in the low kilobyte range to begin with, which will make these sentry reports at least sometimes useful, which is better than what we have now (i.e. nothing).

Do we still see a lot of validation issues in sentry these days? I don't think we do. Let's get this done with a very crude descriptor proto truncation scheme and if validation issues suddenly start popping up again we can backport more clever truncation schemes if need be.

@ajwerner
Copy link
Contributor

What would be cool would be to attach objects to errors and could have some method to encode and redact those objects if they hit a crash report.

As much as I'd like this, it's a bit of a task, so moving to the backlog.

@postamar
Copy link
Contributor

We need to do something here, at least dump the current descriptor undergoing validation.

@ajwerner
Copy link
Contributor

One is give me the string representation as an error detail because that thing is redacted but only semi-structured. The other is give me the redacted proto.

@ajwerner
Copy link
Contributor

Another note here is, if we've collected cross-reference descriptors, let's get those too.

@ajwerner ajwerner self-assigned this Nov 15, 2022
@ajwerner ajwerner removed their assignment Apr 6, 2023
@exalate-issue-sync exalate-issue-sync bot added T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) and removed T-sql-schema-deprecated Use T-sql-foundations instead labels May 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-schema-descriptors Relating to SQL table/db descriptor handling. C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)
Projects
None yet
Development

No branches or pull requests

5 participants