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

Send CID for sentry events. #5625

Merged
merged 8 commits into from
Jun 10, 2020
Merged

Send CID for sentry events. #5625

merged 8 commits into from
Jun 10, 2020

Conversation

parasssh
Copy link
Contributor

@parasssh parasssh commented Jun 10, 2020

Using the file system to share the CID info between processes.


This change is Reviewable

Docs Preview: Dgraph Preview

Copy link
Contributor

@jarifibrahim jarifibrahim left a comment

Choose a reason for hiding this comment

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

:lgtm: Minor comments.

Reviewable status: 0 of 5 files reviewed, 4 unresolved discussions (waiting on @manishrjain, @parasssh, and @vvbalaji-dgraph)


x/sentry_integration.go, line 108 at r1 (raw file):

}

// WriteCidFile writes the CID to a well-known location so it can be read and sent to Sentry on panic.

1000 char


x/sentry_integration.go, line 115 at r1 (raw file):

	err := ioutil.WriteFile(filePath, []byte(cid), 0644)
	if err != nil {

if err := ...


x/sentry_integration.go, line 121 at r1 (raw file):

}

// readAndRemoveCidFile reads the file from a well-known location so it can be read and sent to Sentry on panic.

100 characters


x/sentry_integration.go, line 151 at r1 (raw file):

func PanicHandler(out string) {
	cid := readAndRemoveCidFile()
	if cid != "" {

if cid := ...

Copy link
Contributor

@martinmr martinmr left a comment

Choose a reason for hiding this comment

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

:lgtm: Minor comments

Reviewed 5 of 5 files at r1.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @manishrjain, @parasssh, and @vvbalaji-dgraph)


dgraph/cmd/zero/raft.go, line 511 at r1 (raw file):

				if err == nil {
					glog.Infof("CID set for cluster: %v", id)
					x.WriteCidFile(id)

is it always true that the cluster id will be id? What if another zero already proposed a different cluster ID and this proposal is ignored?

I think it'd be better here to read the cid directly from the cluster state, just to be safe.


x/sentry_integration.go, line 34 at r1 (raw file):

filePath

rename this to cidPath or something more specific.


x/sentry_integration.go, line 116 at r1 (raw file):

Infof

this should be a warning. Same for the other ones.

Copy link
Contributor

@jarifibrahim jarifibrahim left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @manishrjain, @parasssh, and @vvbalaji-dgraph)


x/sentry_integration.go, line 105 at r1 (raw file):

	})

	filePath = os.TempDir() + "/" + "dgraph-" + subcmd + "-cid-sentry" // e.g. /tmp/dgraph-alpha-cid-sentry

100 char


x/sentry_integration.go, line 108 at r1 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

1000 char

100 char

Copy link
Contributor Author

@parasssh parasssh left a comment

Choose a reason for hiding this comment

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

Dismissed @martinmr from a discussion.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @jarifibrahim, @manishrjain, @martinmr, and @vvbalaji-dgraph)


dgraph/cmd/zero/raft.go, line 511 at r1 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

is it always true that the cluster id will be id? What if another zero already proposed a different cluster ID and this proposal is ignored?

I think it'd be better here to read the cid directly from the cluster state, just to be safe.

Not needed. The proposeAndWait( ) will only allow proposals from the leader to go through.


x/sentry_integration.go, line 34 at r1 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…
filePath

rename this to cidPath or something more specific.

Done.


x/sentry_integration.go, line 105 at r1 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

100 char

Done.


x/sentry_integration.go, line 108 at r1 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

100 char

Done.


x/sentry_integration.go, line 115 at r1 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

if err := ...

Done.


x/sentry_integration.go, line 116 at r1 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…
Infof

this should be a warning. Same for the other ones.

Done.


x/sentry_integration.go, line 121 at r1 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

100 characters

Done.


x/sentry_integration.go, line 151 at r1 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

if cid := ...

Done.

Copy link
Contributor Author

@parasssh parasssh left a comment

Choose a reason for hiding this comment

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

Dismissed @martinmr from 2 discussions.
Reviewable status: 4 of 5 files reviewed, 5 unresolved discussions (waiting on @jarifibrahim, @manishrjain, @martinmr, and @vvbalaji-dgraph)

@parasssh parasssh merged commit 9765e3f into master Jun 10, 2020
@parasssh parasssh deleted the paras/sentry_cid branch June 10, 2020 23:09
parasssh pushed a commit that referenced this pull request Jun 10, 2020
(cherry picked from commit 9765e3f)
parasssh pushed a commit that referenced this pull request Jun 11, 2020
dna2github pushed a commit to dna2fork/dgraph that referenced this pull request Jul 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants