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

feature: dgraph_txn_aborts metric for prometheus #6171

Merged
merged 9 commits into from
Aug 14, 2020
Merged

Conversation

darkn3rd
Copy link
Contributor

@darkn3rd darkn3rd commented Aug 12, 2020

This adds new metric and unit test for the metric so that txnAborts can be tracked.

Testing

cd dgraph/dgraph/cmd/alpha
go test -v -run TestMetricTxnAborts

This change is Reviewable

@github-actions github-actions bot added the area/integrations Related to integrations with other projects. label Aug 12, 2020
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 11 files at r2.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @darkn3rd, @manishrjain, and @vvbalaji-dgraph)


dgraph/cmd/alpha/metrics_test.go, line 39 at r2 (raw file):

ts

What's ts, here? I don't see it set in this test. You should


dgraph/cmd/alpha/metrics_test.go, line 54 at r2 (raw file):

	txnAbort1, _ := strconv.Atoi(txnAbort.(string))
	require.True(t, ok, "the required metric %s is not found", requiredMetric)

These two lines should be switched. If ok is false, then the strconv panics since the metric isn't there:

$ go test -v -run TestMetricTxnAborts
[Decoder]: Using assembly version of decoder
=== RUN   TestMetricTxnAborts
--- FAIL: TestMetricTxnAborts (0.13s)
panic: interface conversion: interface {} is nil, not string [recovered]
	panic: interface conversion: interface {} is nil, not string

goroutine 53 [running]:
testing.tRunner.func1.1(0x199b600, 0xc000656030)
	/usr/local/go/src/testing/testing.go:940 +0x2f5
testing.tRunner.func1(0xc0003e3b00)
	/usr/local/go/src/testing/testing.go:943 +0x3f9
panic(0x199b600, 0xc000656030)
	/usr/local/go/src/runtime/panic.go:969 +0x166
github.com/dgraph-io/dgraph/dgraph/cmd/alpha.TestMetricTxnAborts(0xc0003e3b00)
	/home/dmai/go/src/github.com/dgraph-io/dgraph/dgraph/cmd/alpha/metrics_test.go:54 +0xb22
testing.tRunner(0xc0003e3b00, 0x1bbb808)
	/usr/local/go/src/testing/testing.go:991 +0xdc
created by testing.(*T).Run
	/usr/local/go/src/testing/testing.go:1042 +0x357
exit status 2
FAIL	github.com/dgraph-io/dgraph/dgraph/cmd/alpha	0.295s

dgraph/cmd/alpha/metrics_test.go, line 119 at r2 (raw file):

		matches := metricRegex.FindAllString(line, -1)
		if len(matches) > 0 {
			metricsMap[matches[0]] = matches[1]

Why was this change needed?

Copy link
Contributor Author

@darkn3rd darkn3rd 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, 3 unresolved discussions (waiting on @danielmai, @manishrjain, and @vvbalaji-dgraph)


dgraph/cmd/alpha/metrics_test.go, line 39 at r2 (raw file):

Previously, danielmai (Daniel Mai) wrote…
ts

What's ts, here? I don't see it set in this test. You should

It is set to 0 by default, and mutationWithTs() checks if non-zero and append it to the /mutation params.


dgraph/cmd/alpha/metrics_test.go, line 54 at r2 (raw file):

Previously, danielmai (Daniel Mai) wrote…
	txnAbort1, _ := strconv.Atoi(txnAbort.(string))
	require.True(t, ok, "the required metric %s is not found", requiredMetric)

These two lines should be switched. If ok is false, then the strconv panics since the metric isn't there:

$ go test -v -run TestMetricTxnAborts
[Decoder]: Using assembly version of decoder
=== RUN   TestMetricTxnAborts
--- FAIL: TestMetricTxnAborts (0.13s)
panic: interface conversion: interface {} is nil, not string [recovered]
	panic: interface conversion: interface {} is nil, not string

goroutine 53 [running]:
testing.tRunner.func1.1(0x199b600, 0xc000656030)
	/usr/local/go/src/testing/testing.go:940 +0x2f5
testing.tRunner.func1(0xc0003e3b00)
	/usr/local/go/src/testing/testing.go:943 +0x3f9
panic(0x199b600, 0xc000656030)
	/usr/local/go/src/runtime/panic.go:969 +0x166
github.com/dgraph-io/dgraph/dgraph/cmd/alpha.TestMetricTxnAborts(0xc0003e3b00)
	/home/dmai/go/src/github.com/dgraph-io/dgraph/dgraph/cmd/alpha/metrics_test.go:54 +0xb22
testing.tRunner(0xc0003e3b00, 0x1bbb808)
	/usr/local/go/src/testing/testing.go:991 +0xdc
created by testing.(*T).Run
	/usr/local/go/src/testing/testing.go:1042 +0x357
exit status 2
FAIL	github.com/dgraph-io/dgraph/dgraph/cmd/alpha	0.295s

Got it. Will fix this.


dgraph/cmd/alpha/metrics_test.go, line 119 at r2 (raw file):

Previously, danielmai (Daniel Mai) wrote…
		matches := metricRegex.FindAllString(line, -1)
		if len(matches) > 0 {
			metricsMap[matches[0]] = matches[1]

Why was this change needed?

The original code only got the keys. I needed the value for testing, so I added this code to get the values.

Copy link
Contributor Author

@darkn3rd darkn3rd 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: 2 of 3 files reviewed, 3 unresolved discussions (waiting on @danielmai, @manishrjain, and @vvbalaji-dgraph)


dgraph/cmd/alpha/metrics_test.go, line 39 at r2 (raw file):

Previously, darkn3rd (Joaquin Menchaca) wrote…

It is set to 0 by default, and mutationWithTs() checks if non-zero and append it to the /mutation params.

Done.


dgraph/cmd/alpha/metrics_test.go, line 54 at r2 (raw file):

Previously, darkn3rd (Joaquin Menchaca) wrote…

Got it. Will fix this.

Done.


dgraph/cmd/alpha/metrics_test.go, line 119 at r2 (raw file):

Previously, darkn3rd (Joaquin Menchaca) wrote…

The original code only got the keys. I needed the value for testing, so I added this code to get the values.

Done.

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.

:lgtm:

Reviewed 1 of 1 files at r3.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @manishrjain and @vvbalaji-dgraph)

@darkn3rd darkn3rd merged commit deb70a3 into master Aug 14, 2020
@darkn3rd darkn3rd deleted the joaquin/txn_abort branch August 14, 2020 22:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/integrations Related to integrations with other projects.
Development

Successfully merging this pull request may close these issues.

None yet

2 participants