[dbnode] Add reason tag to bootstrap retries metric #3317

vpranckaitis · 2021-03-03T13:19:23Z

What this PR does / why we need it:

Adds reason tag to bootstrap retries metric.

Special notes for your reviewer:

Does this PR introduce a user-facing and/or backwards incompatible change?:

NONE

Does this PR require updating code package or user-facing documentation?:

NONE

codecov · 2021-03-04T09:12:12Z

Codecov Report

Merging #3317 (61967a8) into master (0068083) will decrease coverage by 0.0%.
The diff coverage is 100.0%.

@@            Coverage Diff            @@
##           master    #3317     +/-   ##
=========================================
- Coverage    72.5%    72.5%   -0.1%     
=========================================
  Files        1099     1099             
  Lines      101616   101627     +11     
=========================================
+ Hits        73679    73684      +5     
- Misses      22861    22862      +1     
- Partials     5076     5081      +5

Flag	Coverage Δ
aggregator	`76.4% <ø> (+<0.1%)`	⬆️
cluster	`84.9% <ø> (ø)`
collector	`84.3% <ø> (ø)`
dbnode	`78.9% <100.0%> (+<0.1%)`	⬆️
m3em	`74.4% <ø> (ø)`
m3ninx	`73.5% <ø> (ø)`
metrics	`20.0% <ø> (ø)`
msg	`74.0% <ø> (-0.3%)`	⬇️
query	`67.4% <ø> (ø)`
x	`80.5% <ø> (+<0.1%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0068083...88f45ee. Read the comment docs.

linasm · 2021-03-04T09:47:24Z

src/dbnode/integration/bootstrap_retries_test.go

+		signalCh <- struct{}{}
+		assert.False(t, setup.DB().IsBootstrapped(), "database should not yet be bootstrapped")


Shouldn't signal come after assert to avoid timing issues?

All these IsBootstrapped() asserts are in between first and last signalCh writes, which should guarantee that all of them are called during bootstrap.

After you send a signal, the code that was waiting for the signal starts executing, but you cannot know whether it will take one nanosecond, or 10 seconds to complete, so depending on that timing, your assert would be checking IsBootstrapped value either before, or after the first bootstrap iteration. Not really sure it this is desirable here.

I do not think it matters much if the assert is done during first bootstrapper pass, just before second pass or in between of them. As long as DB is not marked bootstrapped while bootstrapping is in progress (which should match the time period in between first and last signalCh writes, plus some variable amount of time before and after that), and as long as it's marked bootstrapped once bootstrapping is done, it should be fine.

I guess I could make it so two signals are needed for bootstrapper to unblock. That way I could squeeze the assert in between them and that would guarantee exact execution on n-th bootstraper pass. Though I'm not sure if we would gain anything from this.

But aren't you concerned that you could be advancing the clock at some unpredictable moment? Perhaps even before the first bootstrap pass has calculated the ranges to bootstrap?

My bad, I was assuming the ranges are calculated within bs.Bootstrap(ctx, namespaces, cache) and not before invoking the anonymous fn.
Just make sure to rename noopNone to noopAll in places where it gets NewNoOpAllBootstrapperProvider assigned.

Added double signal for more exact control when clock is advanced 4181359
Refactored noopNone 61967a8

src/dbnode/integration/bootstrap_retries_test.go

linasm · 2021-03-04T09:53:57Z

src/dbnode/integration/bootstrap_retries_test.go

+	require.NotNil(t, retryCounter)
+	assert.Equal(t, int64(1), retryCounter.Value())


I believe it is also worth checking the other value here.

Added unit tests for this: e41c8b7

I'm not sure if it was worth adding another test suite, I think TestBootstrapRetriesDueToError was covering the error type check. What I meant was to check that there was:

one retry because of obsolete range

no retries because of some other errors
(and vice versa in some other tests, depending on the case)

Added additional asserts 5afa0b4

linasm · 2021-03-04T09:56:07Z

src/dbnode/integration/bootstrap_retries_test.go

+	retryCounter := getBootstrapRetriesCounter(testScope, "other")
+	require.NotNil(t, retryCounter)
+	assert.Equal(t, int64(1), retryCounter.Value())


Also check the other value (might be worth extracting some kind of assert helper function).

See #3317 (comment)

Added additional asserts 5afa0b4

src/dbnode/storage/bootstrap_instrumentation.go

linasm · 2021-03-04T15:01:21Z

src/dbnode/integration/bootstrap_retries_test.go

+	require.NotNil(t, retryCounter)
+	assert.Equal(t, int64(1), retryCounter.Value())


I'm not sure if it was worth adding another test suite, I think TestBootstrapRetriesDueToError was covering the error type check. What I meant was to check that there was:

one retry because of obsolete range

no retries because of some other errors
(and vice versa in some other tests, depending on the case)

src/dbnode/integration/bootstrap_retries_test.go

linasm · 2021-03-04T15:16:57Z

src/dbnode/integration/bootstrap_retries_test.go

+		signalCh <- struct{}{}
+		assert.False(t, setup.DB().IsBootstrapped(), "database should not yet be bootstrapped")


After you send a signal, the code that was waiting for the signal starts executing, but you cannot know whether it will take one nanosecond, or 10 seconds to complete, so depending on that timing, your assert would be checking IsBootstrapped value either before, or after the first bootstrap iteration. Not really sure it this is desirable here.

src/dbnode/storage/bootstrap_instrumentation.go

linasm · 2021-03-05T06:45:32Z

src/dbnode/storage/bootstrap_instrumentation_test.go

+			if r, ok := counter.Tags()[reasonTag]; ok {
+				valuesByReason[r] = int(counter.Value())
+			} else {
+				valuesByReason[""] = int(counter.Value())
+			}


Nit:

Suggested change

if r, ok := counter.Tags()[reasonTag]; ok {

valuesByReason[r] = int(counter.Value())

} else {

valuesByReason[""] = int(counter.Value())

}

reason := ""

if r, ok := counter.Tags()[reasonTag]; ok {

reason = r

}

valuesByReason[reason] = int(counter.Value())

05131b5#diff-1535032d3345792a639097a8da6a9103f54aa05624d5068f237caec57f4b0d2cR82-R86

linasm · 2021-03-05T06:47:34Z

src/dbnode/storage/bootstrap_instrumentation_test.go

+	xerrors "github.com/m3db/m3/src/x/errors"
+)
+
+func TestBootstrapFailedMetricReason(t *testing.T) {


As I mentioned in one of the comments above, this test might be an overkill, I'd say testing this indirectly from bootstrap_retries_test.go is adequate. Leaving the final decision to you though.

Gonna leave, in case we add more retry reasons in the future.

On average, writing code "for future" does not pay off :)

linasm

LGTM

* master: (22 commits) Remove deprecated fields (#3327) Add quotas to Permits (#3333) [aggregator] Drop messages that have a drop policy applied (#3341) Fix NPE due to race with a closing series (#3056) [coordinator] Apply auto-mapping rules if-and-only-if no drop policies are in effect (#3339) [aggregator] Add validation in AddTimedWithStagedMetadatas (#3338) [coordinator] Fix panic in Ready endpoint for admin coordinator (#3335) [instrument] Config option to emit detailed Go runtime metrics only (#3332) [aggregator] Sort heap in one go, instead of iterating one-by-one (#3331) [pool] Add support for dynamic, sync.Pool backed, object pools (#3334) Enable PANIC_ON_INVARIANT_VIOLATED for tests (#3326) [aggregator] CanLead for unflushed window takes BufferPast into account (#3328) Optimize StagedMetadatas conversion (#3330) [m3msg] Improve message scan performance (#3319) [dbnode] Add reason tag to bootstrap retries metric (#3317) [coordinator] Enable rule filtering on prom metric type (#3325) Update m3dbnode-all-config.yml (#3204) [coordinator] Include Type in RollupOp.Equal (#3322) [coordinator] Simplify iteration logic of matchRollupTarget (#3321) [coordinator] Add rollup type to remove specific dimensions (#3318) ...

vpranckaitis added 5 commits March 2, 2021 12:16

[dbnode] Add reason tag to bootstrap retries metric

fcd2646

Merge branch 'master' into vilius/retry_metric_reason

26dd3e0

increase obsolete-ranges metric when appropriate

778aed4

validate metric value in tests

8d0c2c1

reorder imports

00328cf

vpranckaitis requested a review from linasm March 4, 2021 09:20

vpranckaitis assigned linasm Mar 4, 2021

linasm reviewed Mar 4, 2021

View reviewed changes

add unit tests

e41c8b7

vpranckaitis requested a review from linasm March 4, 2021 14:14

linasm reviewed Mar 5, 2021

View reviewed changes

linasm removed their assignment Mar 5, 2021

vpranckaitis added 3 commits March 5, 2021 09:18

use xerrors.Is()

5f49665

PR comments

05131b5

additional asserts for metrics

5afa0b4

vpranckaitis requested a review from linasm March 5, 2021 08:20

vpranckaitis assigned linasm Mar 5, 2021

linasm removed their assignment Mar 5, 2021

vpranckaitis added 3 commits March 5, 2021 11:58

advance clock during bootstrapper run

4181359

update comment

e205fbc

refactor some variables

61967a8

vpranckaitis assigned linasm Mar 5, 2021

linasm approved these changes Mar 5, 2021

View reviewed changes

Merge branch 'master' into vilius/retry_metric_reason

88f45ee

vpranckaitis merged commit 3e78412 into master Mar 5, 2021

vpranckaitis deleted the vilius/retry_metric_reason branch March 5, 2021 11:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[dbnode] Add reason tag to bootstrap retries metric #3317

[dbnode] Add reason tag to bootstrap retries metric #3317

vpranckaitis commented Mar 3, 2021

codecov bot commented Mar 4, 2021 •

edited

Loading

linasm Mar 4, 2021

vpranckaitis Mar 4, 2021

linasm Mar 4, 2021

vpranckaitis Mar 5, 2021

linasm Mar 5, 2021

linasm Mar 5, 2021 •

edited

Loading

vpranckaitis Mar 5, 2021

linasm Mar 4, 2021

vpranckaitis Mar 4, 2021

linasm Mar 4, 2021

vpranckaitis Mar 5, 2021

linasm Mar 4, 2021

vpranckaitis Mar 4, 2021 •

edited

Loading

vpranckaitis Mar 5, 2021

linasm Mar 4, 2021

linasm Mar 4, 2021

linasm Mar 5, 2021

vpranckaitis Mar 5, 2021

linasm Mar 5, 2021

vpranckaitis Mar 5, 2021

linasm Mar 5, 2021

linasm left a comment

		signalCh <- struct{}{}
		assert.False(t, setup.DB().IsBootstrapped(), "database should not yet be bootstrapped")

		require.NotNil(t, retryCounter)
		assert.Equal(t, int64(1), retryCounter.Value())

[dbnode] Add reason tag to bootstrap retries metric #3317

[dbnode] Add reason tag to bootstrap retries metric #3317

Conversation

vpranckaitis commented Mar 3, 2021

codecov bot commented Mar 4, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linasm Mar 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vpranckaitis Mar 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linasm left a comment

Choose a reason for hiding this comment

codecov bot commented Mar 4, 2021 •

edited

Loading

linasm Mar 5, 2021 •

edited

Loading

vpranckaitis Mar 4, 2021 •

edited

Loading