cockroachdb / errors

Go error library with error portability over the network
Apache License 2.0
2.07k stars 66 forks source link

Drop fork and update to latest sentry-go #92

Closed jhchabran closed 2 years ago

jhchabran commented 2 years ago

Fixes #36 Fixes #41

This PR drops the fork and updates to the latest stable sentry-go release (v0.12.0).

Because that's quite a jump, even though there are no breaking changes, I did some manual tests, to make sure everything still looks correct (see the folds below).

Tests are green locally.

Quick script I used to QA `cmd/qa/main.go` ```go package main import ( "fmt" "log" "os" "time" "github.com/cockroachdb/errors" "github.com/getsentry/sentry-go" ) func Foo(i int) { Bar(i + 1) } func Bar(i int) { Baz(i + 1) } func Baz(i int) { _, err := os.Open("non-existing.ext") e := errors.Wrap(err, "Doing some QA") errors.ReportError(e) } func Event() { _, err := os.Open("different-non-existing.ext") e := errors.Wrap(err, "Doing some QA") event, extraDetails := errors.BuildSentryReport(e) fmt.Printf("%#v\n", extraDetails) sentry.WithScope(func(scope *sentry.Scope) { scope.SetExtras(extraDetails) sentry.CaptureEvent(event) }) } func main() { err := sentry.Init(sentry.ClientOptions{}) if err != nil { log.Fatalf("sentry.Init: %s", err) } Foo(2) Event() defer sentry.Flush(5 * time.Second) fmt.Println("bye") } ```
Screenshots image image

This change is Reviewable

cockroach-teamcity commented 2 years ago

CLA assistant check
All committers have signed the CLA.

knz commented 2 years ago

Ok, this change is amazing. I had feared that by upgrading the dependency, we'd need to also change the API inside the errors library. I'm glad this is not the case.

That being said, before we merge this we will also want to test it out with CockroachDB and our Sentry account, to verify the issues get filed accurately. I will schedule some work on that direction and keep you updated.

jhchabran commented 2 years ago

Ok, this change is amazing. I had feared that by upgrading the dependency, we'd need to also change the API inside the errors library. I'm glad this is not the case.

Thank you! It really went nicely, though I have to mention that I pulled my hairs on the tests because I had cloned the repo under a different name git clone ... newname which broke them because they do depend on the base folder being named errors. I'm writing this down here just in case does the same mistake as me 😵‍💫

That being said, before we merge this we will also want to test it out with CockroachDB and our Sentry account, to verify the issues get filed accurately. I will schedule some work on that direction and keep you updated.

I totally understand and I'm really grateful for you taking the time to do this soon. I know that testing these things can be really annoying, so feel free to reach out if I can help with anything!

EDIT:

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @jhchabran)

@knz This seems to imply I have something else to do. Is that the case? I'm not sure to see what else I could do here!

knz commented 2 years ago

This seems to imply I have something else to do. Is that the case? I'm not sure to see what else I could do here!

nah you're good.

jhchabran commented 2 years ago

@knz I just noticed while reading the changelog that sentry-go officially supports go starting from v1.15 (see changelog) Looking at the versions being tested in CI for cockroach/errors goes back to Go v1.13.0 If that's not an option, we're stuck with sentry-go v0.10.0 (which is still an improvement).

Is that okay or is that too big of a jump for the users? I'll reflect that decision with another commit.

knz commented 2 years ago

CI says it's fine apparently? image

knz commented 2 years ago

This works as per https://github.com/cockroachdb/cockroach/issues/76439