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

deps: evaluate a possible upgrade to the newest sentry-go #76439

Closed
knz opened this issue Feb 11, 2022 · 3 comments · Fixed by #76498
Closed

deps: evaluate a possible upgrade to the newest sentry-go #76439

knz opened this issue Feb 11, 2022 · 3 comments · Fixed by #76498
Assignees
Labels
A-error-handling Error messages, error propagations/annotations C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-server-and-security DB Server & Security

Comments

@knz
Copy link
Contributor

knz commented Feb 11, 2022

Currently CockroachDb files issues to Sentry.io using an outdated version of sentry-go (SDK 0.6.1).

The filing of issues happens automatically via the errors library.
This week, an external contributor volunteered to upgrade the sentry-go dependency in errors: cockroachdb/errors#92

Before we accept this PR, we need to qualify this inside CockroachDB:

  1. create a build of CockroachDB with the proposed patch in errors.
  2. point the build to our dev instance in Sentry.io
  3. trigger a reportable error
  4. check in the Sentry UI that the error report is 1) actually filed and 2) that the metadata is preserved just as well as in the previous SDK version.
@knz knz added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) A-error-handling Error messages, error propagations/annotations labels Feb 11, 2022
@blathers-crl blathers-crl bot added the T-server-and-security DB Server & Security label Feb 11, 2022
@knz
Copy link
Contributor Author

knz commented Feb 11, 2022

I have associated this issue to the server team but I think the ownership is unclear.
@jtsiros

@rail
Copy link
Member

rail commented Feb 14, 2022

I took a look at this using Bazel - it's easier to pin things with it. This my current diff:
https://gist.githubusercontent.com/rail/d347a796d7ecea31bda94587f2e453b8/raw

I ran go get github.com/getsentry/[email protected], then replaced the references to our fork in the go files, and ran ./dev generate bazel. Then I tweaked DEPS.bazel and pointed the errors module to the fork from the PR.

I generated the binary using ./dev buld short.

To trigger a crash I rand the following:

export COCKROACH_CRASH_REPORTS=https://[email protected]/api/sentrydev/v2/1111
echo "select crdb_internal.force_panic('testing');" | ./cockroach demo --insecure

The Sentry issue is here: https://sentry.io/organizations/cockroach-labs/issues/3012238368/?project=161792

@knz can you take a look and see if there is something that looks wrong?

At least the setry-go SDK version looks sane in the issue:
image

@knz
Copy link
Contributor Author

knz commented Feb 14, 2022

Yes this looks good. Amazing, thanks @rail

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-error-handling Error messages, error propagations/annotations C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-server-and-security DB Server & Security
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants