-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Conversation
There was a problem hiding this 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?
There was a problem hiding this 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.
There was a problem hiding this 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r3.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @manishrjain and @vvbalaji-dgraph)
This adds new metric and unit test for the metric so that txnAborts can be tracked.
Testing
This change is