Summary
TestMetricTxnAborts reads its baseline metric value with an unretried fetchMetrics, while the assertion that follows uses retryableFetchMetrics. Prometheus metric export is asynchronous, so the baseline read can land before dgraph_txn_aborts_total has been published and the test fails with a message that reads like a missing metric rather than a race.
The code
dgraph/cmd/alpha/metrics_test.go:
func TestMetricTxnAborts(t *testing.T) {
metricName := "dgraph_txn_aborts_total"
...
require.NoError(t, commitWithTs(mr1, false))
require.Error(t, commitWithTs(mr2, false))
metrics := fetchMetrics(t, metricName) // <-- not retried
... second round of mutations ...
require.NoError(t, retryableFetchMetrics(t, map[string]int{
metricName: metrics[metricName] + 1, // <-- retried
}))
}The asymmetry looks unintentional: the same value is fetched twice, once without tolerance for propagation delay and once with.
Note also that dgraph_txn_aborts_total does not exist on the /metrics endpoint until the first abort is actually exported, so on a fresh cluster the baseline read is the most likely of the two to race — it is the one that has to wait for the counter to appear at all, not merely to increment.
Observed failure
--- FAIL: TestMetricTxnAborts (0.02s)
metrics_test.go:153: the required metric 'dgraph_txn_aborts_total' was not found
Line 153 is inside fetchMetrics, reached from the unretried call. It passes on re-run, which is consistent with a propagation race rather than a functional problem. The preceding require.Error(t, commitWithTs(mr2, false)) passes, so the aborting commit did happen.
Seen once in CI on a fork of this repo; green on re-run of the same commit.
Suggested fix
Use the retrying variant for the baseline too, so the test tolerates propagation in both reads:
require.NoError(t, retryableFetchMetrics(t, map[string]int{metricName: 1}))
metrics := fetchMetrics(t, metricName)Or have fetchMetrics treat "metric absent" as retryable when the caller is establishing a baseline. Either way the intent is that the test waits for the counter to exist before reading it.
TestMetricTxnCommits and TestMetricTxnDiscards have the same shape and are presumably exposed to the same race, though we have only observed the aborts variant failing.
I'm happy to open a PR for this if it's welcome.
Summary
TestMetricTxnAbortsreads its baseline metric value with an unretriedfetchMetrics, while the assertion that follows usesretryableFetchMetrics. Prometheus metric export is asynchronous, so the baseline read can land beforedgraph_txn_aborts_totalhas been published and the test fails with a message that reads like a missing metric rather than a race.The code
dgraph/cmd/alpha/metrics_test.go:The asymmetry looks unintentional: the same value is fetched twice, once without tolerance for propagation delay and once with.
Note also that
dgraph_txn_aborts_totaldoes not exist on the/metricsendpoint until the first abort is actually exported, so on a fresh cluster the baseline read is the most likely of the two to race — it is the one that has to wait for the counter to appear at all, not merely to increment.Observed failure
Line 153 is inside
fetchMetrics, reached from the unretried call. It passes on re-run, which is consistent with a propagation race rather than a functional problem. The precedingrequire.Error(t, commitWithTs(mr2, false))passes, so the aborting commit did happen.Seen once in CI on a fork of this repo; green on re-run of the same commit.
Suggested fix
Use the retrying variant for the baseline too, so the test tolerates propagation in both reads:
Or have
fetchMetricstreat "metric absent" as retryable when the caller is establishing a baseline. Either way the intent is that the test waits for the counter to exist before reading it.TestMetricTxnCommitsandTestMetricTxnDiscardshave the same shape and are presumably exposed to the same race, though we have only observed the aborts variant failing.I'm happy to open a PR for this if it's welcome.