Add new metrics. #396

codesome · 2018-09-25T11:55:27Z

Fixes #373.

This is the best I could think of to prevent passing entire head or headMetrics into Checkpoint(..).

Signed-off-by: Ganesh Vernekar [email protected]

krasi-georgiev · 2018-09-25T12:59:36Z

why not put the walTruncationFail in
https://github.com/prometheus/tsdb/blob/a971f52ac8c41ecf8ca54325b312ddab97b1f3aa/wal.go#L58-L61

codesome · 2018-09-25T13:08:11Z

Thanks, didn't know about that. That would cover prometheus_tsdb_wal_truncate_fail, we might still want to pass prometheus_tsdb_checkpoint_delete_fail to the Checkpoint(...).

krasi-georgiev · 2018-09-25T13:08:57Z

yes I am still looking through the code to see if we can simplify that part as well.

1. 'prometheus_tsdb_wal_truncate_fail' for failed WAL truncation. 2. 'prometheus_tsdb_checkpoint_delete_fail' for failed old checkpoint delete. Signed-off-by: Ganesh Vernekar <[email protected]>

simonpasquier · 2018-09-25T12:54:52Z

head.go

@@ -146,10 +148,18 @@ func newHeadMetrics(h *Head, r prometheus.Registerer) *headMetrics {
 		Name: "prometheus_tsdb_wal_truncate_duration_seconds",
 		Help: "Duration of WAL truncation.",
 	})
+	m.walTruncateFail = prometheus.NewCounter(prometheus.CounterOpts{
+		Name: "prometheus_tsdb_wal_truncate_fail",


prometheus_tsdb_wal_truncate_failed_total rather

simonpasquier · 2018-09-25T13:01:28Z

head.go

 	m.samplesAppended = prometheus.NewCounter(prometheus.CounterOpts{
 		Name: "prometheus_tsdb_head_samples_appended_total",
 		Help: "Total number of appended samples.",
 	})
+	m.checkpointDeleteFail = prometheus.NewCounter(prometheus.CounterOpts{
+		Name: "prometheus_tsdb_checkpoint_delete_fail",


prometheus_tsdb_checkpoint_deletions_failed_total

simonpasquier · 2018-09-25T13:03:15Z

head.go

 	m.samplesAppended = prometheus.NewCounter(prometheus.CounterOpts{
 		Name: "prometheus_tsdb_head_samples_appended_total",
 		Help: "Total number of appended samples.",
 	})
+	m.checkpointDeleteFail = prometheus.NewCounter(prometheus.CounterOpts{
+		Name: "prometheus_tsdb_checkpoint_delete_fail",
+		Help: "Number of times deletion of old checkpoint failed.",


"Total number of checkpoint deletions that failed."

simonpasquier · 2018-09-25T13:03:26Z

head.go

@@ -146,10 +148,18 @@ func newHeadMetrics(h *Head, r prometheus.Registerer) *headMetrics {
 		Name: "prometheus_tsdb_wal_truncate_duration_seconds",
 		Help: "Duration of WAL truncation.",
 	})
+	m.walTruncateFail = prometheus.NewCounter(prometheus.CounterOpts{
+		Name: "prometheus_tsdb_wal_truncate_fail",
+		Help: "Number of times WAL truncation failed.",


"Total number of WAL truncations that failed."

simonpasquier · 2018-09-25T13:05:53Z

checkpoint.go

@@ -277,12 +279,14 @@ func Checkpoint(logger log.Logger, w *wal.WAL, m, n int, keep func(id uint64) bo
 		// Leftover segments will just be ignored in the future if there's a checkpoint
 		// that supersedes them.
 		level.Error(logger).Log("msg", "truncating segments failed", "err", err)
+		walTruncationFail.Add(float64(1))


I would move the metric to the wal.Truncate() function and add another metric (eg prometheus_tsdb_checkpoints_failed_total) in head.go that would be increased when this function returns a non-nil error.

simonpasquier · 2018-09-25T13:05:59Z

checkpoint.go

 	}
 	if err := DeleteCheckpoints(w.Dir(), n); err != nil {
 		// Leftover old checkpoints do not cause problems down the line beyond
 		// occupying disk space.
 		// They will just be ignored since a higher checkpoint exists.
 		level.Error(logger).Log("msg", "delete old checkpoints", "err", err)
+		checkpointDeleteFail.Add(float64(1))


you could use checkpointDeleteFail.Inc()

simonpasquier · 2018-09-25T13:24:07Z

My review clashed with @krasi-georgiev, I'll do a second pass on the updated code...

simonpasquier · 2018-09-25T13:24:37Z

checkpoint.go

@@ -283,6 +284,7 @@ func Checkpoint(logger log.Logger, w *wal.WAL, m, n int, keep func(id uint64) bo
 		// occupying disk space.
 		// They will just be ignored since a higher checkpoint exists.
 		level.Error(logger).Log("msg", "delete old checkpoints", "err", err)
+		checkpointDeleteFail.Add(float64(1))


you could use checkpointDeleteFail.Inc()

simonpasquier · 2018-09-25T13:25:19Z

wal/wal.go

@@ -530,13 +535,15 @@ func (w *WAL) Segments() (m, n int, err error) {
 func (w *WAL) Truncate(i int) error {
 	refs, err := listSegments(w.dir)
 	if err != nil {
+		w.truncateFail.Add(float64(1))


w.truncateFail.Inc()

simonpasquier · 2018-09-25T13:25:38Z

wal/wal.go

 		return err
 	}
 	for _, r := range refs {
 		if r.n >= i {
 			break
 		}
 		if err := os.Remove(filepath.Join(w.dir, r.s)); err != nil {
+			w.truncateFail.Add(float64(1))


simonpasquier · 2018-09-25T13:29:17Z

I'd add another metric (eg prometheus_tsdb_checkpoints_failed_total) in head.go that would be increased whenever Checkpoint() returns a non-nil error.

simonpasquier · 2018-09-25T14:01:54Z

head.go

+	})
+	m.checkpointsFail = prometheus.NewCounter(prometheus.CounterOpts{
+		Name: "prometheus_tsdb_checkpoints_failed_total",
+		Help: "Total number of create checkpoint that failed.",


(nit) "Total number of checkpoints that failed."

krasi-georgiev · 2018-09-25T14:22:30Z

I think the w.Truncate(..) and DeleteCheckpoints(...) should be move from Checkpoint to head.Truncate(...)

https://github.com/prometheus/tsdb/blob/a971f52ac8c41ecf8ca54325b312ddab97b1f3aa/checkpoint.go#L275-L286

This way the Checkpoint func's purpose is to only creating the checkpoints and deleting old checkpoints and truncating the wal is left to the caller.

It also solves the problem with the metrics.

codesome · 2018-09-25T14:29:40Z

I don't see anything wrong in what you proposed. Should I wait for another ack or shall I go ahead with this?

krasi-georgiev · 2018-09-25T14:33:45Z

it is an easy change so I would say go ahead and if anyone objects it is easy to revert.

krasi-georgiev · 2018-09-25T15:29:33Z

ping me when ready for another review.

codesome · 2018-09-25T16:23:52Z

ping @krasi-georgiev

krasi-georgiev · 2018-09-25T18:41:38Z

wal/wal.go

 		return err
 	}
 	for _, r := range refs {
 		if r.n >= i {
 			break
 		}
 		if err := os.Remove(filepath.Join(w.dir, r.s)); err != nil {
+			w.truncateFail.Inc()


not a deal breaker, but to make it more bulletproof against further code changes how about adding this somewhere in the beginning:

defer func() { if err!=nil{ w.truncateFail.Inc() } }()

this way even if we add more blocks that might return an error wouldn't have to change the logic for the metrics.

Thought about that :) I modified now.

head.go

checkpoint.go

gouthamve · 2018-09-27T03:48:34Z

Sorry, for jumping in late for the review, this generally LGTM, but I think we need to add the total number of attempts for each failure too. Metrics are cheap, and shouldn't be a problem to add a couple of more if they are going to make debugging easy.

See: https://prometheus.io/docs/practices/instrumentation/#failures

When reporting failures, you should generally have some other metric representing the total number of attempts. This makes the failure ratio easy to calculate.

Further, we don't have a metric for see the number of failures for creating a Checkpoint and I think that should be added too. I am not fully awake yet, so, feel free to let me know if I got something wrong anywhere.

simonpasquier · 2018-09-27T07:11:20Z

👍 to what @gouthamve says. As I've written before, it is not just about checkpoint deletions that fail. We have blind spots on head.Truncate() and this PR needs to solve them all,

codesome · 2018-09-27T08:32:30Z

From what I understand, we are looking at adding these in addition to the currents ones

prometheus_tsdb_wal_truncate_attempt_total
prometheus_tsdb_head_truncate_attempt_total
prometheus_tsdb_checkpoint_creation_failed_total
prometheus_tsdb_checkpoint_creation_attempt_total

Am I correct?

simonpasquier · 2018-09-27T08:55:12Z

@codesome yes. By convention, metric names are foo_failed_total and foo_total (instead of foo_attempt_total).

prometheus_tsdb_wal_truncations_total
prometheus_tsdb_head_truncations_total
prometheus_tsdb_checkpoint_creations_failed_total
prometheus_tsdb_checkpoint_creations_total

codesome · 2018-09-27T09:19:04Z

Updated. Now it contains metrics for

Head truncation fail and total
WAL truncation fail and total
Checkpoint creation fail and total
Checkpoint deletion fail and total

simonpasquier · 2018-09-27T14:14:03Z

head.go

+		Help: "Total number of checkpoint deletions attempted.",
+	})
+	m.checkpointCreationFail = prometheus.NewCounter(prometheus.CounterOpts{
+		Name: "prometheus_tsdb_checkpoint_creation_failed_total",


(nit) prometheus_tsdb_checkpoint_creations_failed_total

simonpasquier · 2018-09-27T14:14:14Z

head.go

+		Help: "Total number of checkpoint creations that failed.",
+	})
+	m.checkpointCreationTotal = prometheus.NewCounter(prometheus.CounterOpts{
+		Name: "prometheus_tsdb_checkpoint_creation_total",


(nit) prometheus_tsdb_checkpoint_creations_total

simonpasquier · 2018-09-27T14:14:47Z

2 small comments otherwise 👍

krasi-georgiev · 2018-09-27T18:19:25Z

LGTM
@gouthamve or @simonpasquier feel free to merge once your comments are addressed.

Signed-off-by: Ganesh Vernekar <[email protected]>

codesome · 2018-09-30T08:47:58Z

I was wondering if there is some bug in travis test. Error says ./db_test.go:1323:9: cannot assign 1 values to 2 variables, but I can see only 1304 lines in db_test.go.

It is failing similarly in #398

PS: Test passes in my system with no build fails.

krasi-georgiev · 2018-09-30T14:12:26Z

when I clone the pr using
https://help.github.com/articles/checking-out-pull-requests-locally/

I do see 1323, but it is as it should be : err := db.compact()

maybe try rebasing?

Signed-off-by: Ganesh Vernekar <[email protected]>

codesome · 2018-09-30T14:55:15Z

Yup rebasing fixed it. Didn't know that tests are performed this way.

simonpasquier · 2018-10-01T14:00:16Z

@codesome thanks for the work!

Add new metrics.

632dfb3

1. 'prometheus_tsdb_wal_truncate_fail' for failed WAL truncation. 2. 'prometheus_tsdb_checkpoint_delete_fail' for failed old checkpoint delete. Signed-off-by: Ganesh Vernekar <[email protected]>

codesome force-pushed the new-metrics branch from 2a38e2a to 632dfb3 Compare September 25, 2018 13:21

simonpasquier reviewed Sep 25, 2018

View reviewed changes

codesome force-pushed the new-metrics branch from 0678984 to ff0c907 Compare September 25, 2018 14:16

codesome force-pushed the new-metrics branch from ff0c907 to 231eb41 Compare September 25, 2018 16:22

krasi-georgiev reviewed Sep 25, 2018

View reviewed changes

codesome force-pushed the new-metrics branch 2 times, most recently from 80d6431 to 4115dc5 Compare September 26, 2018 13:39

codesome force-pushed the new-metrics branch from 4115dc5 to 92a5a0f Compare September 27, 2018 09:17

simonpasquier reviewed Sep 27, 2018

View reviewed changes

Fix review comments

61b000e

Signed-off-by: Ganesh Vernekar <[email protected]>

codesome force-pushed the new-metrics branch from 92a5a0f to 61b000e Compare September 28, 2018 09:31

Merge remote-tracking branch 'upstream/master' into new-metrics

2638b58

Signed-off-by: Ganesh Vernekar <[email protected]>

simonpasquier approved these changes Oct 1, 2018

View reviewed changes

simonpasquier merged commit 043e3bb into prometheus-junkyard:master Oct 1, 2018

codesome deleted the new-metrics branch October 1, 2018 14:01

simonpasquier mentioned this pull request Oct 4, 2018

WAL truncation fails in weird ways under load prometheus/prometheus#4578

Closed

codesome mentioned this pull request Oct 11, 2018

Bring back old functionality of Checkpoint() #406

Closed

Add new metrics. #396

Add new metrics. #396

Conversation

codesome commented Sep 25, 2018

krasi-georgiev commented Sep 25, 2018 • edited Loading

codesome commented Sep 25, 2018

krasi-georgiev commented Sep 25, 2018

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

simonpasquier commented Sep 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonpasquier commented Sep 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krasi-georgiev commented Sep 25, 2018

codesome commented Sep 25, 2018

krasi-georgiev commented Sep 25, 2018

krasi-georgiev commented Sep 25, 2018

codesome commented Sep 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gouthamve commented Sep 27, 2018

simonpasquier commented Sep 27, 2018

codesome commented Sep 27, 2018

simonpasquier commented Sep 27, 2018

codesome commented Sep 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonpasquier commented Sep 27, 2018

krasi-georgiev commented Sep 27, 2018

codesome commented Sep 30, 2018

krasi-georgiev commented Sep 30, 2018

codesome commented Sep 30, 2018

simonpasquier commented Oct 1, 2018

krasi-georgiev commented Sep 25, 2018 •

edited

Loading