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

Indicate dev environment in Sentry events #5051

Merged
merged 13 commits into from
Mar 30, 2020
Merged

Conversation

parasssh
Copy link
Contributor

@parasssh parasssh commented Mar 27, 2020

Description.

Check for git commit hash in the version string to infer dev environment for Sentry. We now have 4 possible sentry enviroments:

  1. prod-enterprise
  2. prod-oss
  3. dev-enterprise
  4. dev-oss

GitHub Issue or Jira number.

https://dgraph.atlassian.net/browse/DGRAPH-1164

Other components or 3rd party tools affected (or regression areas).

Affected releases.

20.07

Changelog tags.

Please indicate if this is a breaking change.

Please indicate if this is an enterprise feature.

Please indicate if documentation needs to be updated.

Please indicate if end to end testing is needed.


This change is Reviewable

@parasssh parasssh changed the title Paras/sentry version checked Indicate dev environment in Sentry events Mar 27, 2020
@parasssh parasssh marked this pull request as ready for review March 28, 2020 00:26
@parasssh parasssh requested a review from a team as a code owner March 28, 2020 00:26
x/init.go Outdated
@@ -103,6 +104,16 @@ func Version() string {
return dgraphVersion
}

// DevVersion returns true if the version string contains a git commit hash.
// e.g.

Choose a reason for hiding this comment

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

File is not gofmt-ed with -s (from gofmt)

Suggested change
// e.g.
// e.g.

Copy link
Contributor

@danielmai danielmai left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 3 files at r1.
Reviewable status: 1 of 3 files reviewed, 4 unresolved discussions (waiting on @danielmai, @manishrjain, and @parasssh)


x/init.go, line 110 at r1 (raw file):

_

This error is for validating the regexp pattern. Better to use regexp.MustCompile and then use (* RexExp).MatchString: https://golang.org/pkg/regexp/#Regexp.MatchString. That way, the error doesn't to be ignored/checked.


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

		env += "-enterprise"
	} else {
		env += "-oss"

This looks different than the versions in the PR description. When it's not a dev version, then the env is prefixed with a "-".

DevVersion: true DevVersion: false
ee: true dev-enterprise -enterprise
ee: false dev-oss -oss

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

		CaptureSentryException(err)
		panic(err)
	}

This is no longer needed?

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 @danielmai from 3 discussions.
Reviewable status: 1 of 4 files reviewed, 2 unresolved discussions (waiting on @danielmai, @golangcibot, and @manishrjain)


x/init.go, line 108 at r2 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

File is not gofmt-ed with -s (from gofmt)

// e.g.

Done.


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

Previously, danielmai (Daniel Mai) wrote…

This looks different than the versions in the PR description. When it's not a dev version, then the env is prefixed with a "-".

DevVersion: true DevVersion: false
ee: true dev-enterprise -enterprise
ee: false dev-oss -oss

I noticed that. Fixed.
Now, we have
{prod|dev}{enterprise|oss}

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.

Reviewable status: 1 of 4 files reviewed, 2 unresolved discussions (waiting on @danielmai, @golangcibot, and @manishrjain)


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

Previously, parasssh wrote…

I noticed that. Fixed.
Now, we have
{prod|dev}{enterprise|oss}

I mean
{prod|dev}-{enterprise|oss}

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.

Reviewable status: 1 of 4 files reviewed, 2 unresolved discussions (waiting on @danielmai, @golangcibot, and @manishrjain)


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

Previously, danielmai (Daniel Mai) wrote…

This is no longer needed?

I moved it to x/error.go. Also we dont need to explicitly call CaptureSentryException() because the panic handler does that now.

@parasssh parasssh requested a review from danielmai March 28, 2020 00:52
Copy link
Contributor

@manishrjain manishrjain left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 3 files at r1, 1 of 2 files at r2, 2 of 2 files at r3.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @golangcibot and @parasssh)


x/init.go, line 113 at r3 (raw file):

func DevVersion() (matched bool) {
	pattern := `-g[[:xdigit:]]{7,}` // -g followed by a commit-hash of min. 7 hex digits.
	return (regexp.MustCompile(pattern).MatchString(dgraphVersion))

Make the MustCompile outside and in a var, so it gets initialized once.

x/init.go Outdated
@@ -103,6 +104,15 @@ func Version() string {
return dgraphVersion
}

var versionRe *regexp.Regexp = regexp.MustCompile(`-g[[:xdigit:]]{7,}`) // -g followed by a commit-hash of min. 7 hex digits.

Choose a reason for hiding this comment

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

line is 125 characters (from lll)

x/init.go Outdated
@@ -103,6 +104,16 @@ func Version() string {
return dgraphVersion
}

// pattern for dev version = min. 7 hex digits of commit-hash.
var versionRe *regexp.Regexp = regexp.MustCompile(`-g[[:xdigit:]]{7,}`)

Choose a reason for hiding this comment

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

File is not gofmt-ed with -s (from gofmt)

Suggested change
var versionRe *regexp.Regexp = regexp.MustCompile(`-g[[:xdigit:]]{7,}`)
var versionRe *regexp.Regexp = regexp.MustCompile(`-g[[:xdigit:]]{7,}`)

@parasssh parasssh merged commit 7e72785 into master Mar 30, 2020
parasssh pushed a commit that referenced this pull request Mar 30, 2020
@joshua-goldstein joshua-goldstein deleted the paras/sentry_version_checked branch August 11, 2022 20:34
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.

4 participants