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

Drop fork and update to latest sentry-go #92

Merged
merged 1 commit into from
Feb 14, 2022

Conversation

jhchabran
Copy link
Contributor

@jhchabran jhchabran commented Feb 10, 2022

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

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
Copy link
Member

cockroach-teamcity commented Feb 10, 2022

CLA assistant check
All committers have signed the CLA.

Copy link

@bobheadxi bobheadxi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice, thanks for extensively manual testing this!

@knz
Copy link
Contributor

knz commented Feb 10, 2022

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.

Copy link
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 8 of 8 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @jhchabran)

@jhchabran
Copy link
Contributor Author

jhchabran commented Feb 11, 2022

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!

@cockroachdb cockroachdb deleted a comment from huzhao37 Feb 11, 2022
@knz
Copy link
Contributor

knz commented Feb 11, 2022

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
Copy link
Contributor Author

jhchabran commented Feb 11, 2022

@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
Copy link
Contributor

knz commented Feb 11, 2022

CI says it's fine apparently?
image

@knz
Copy link
Contributor

knz commented Feb 14, 2022

This works as per cockroachdb/cockroach#76439

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update to Sentry SDK v0.7.0 Requires use of github.com/cockroachdb/sentry-go
4 participants