Skip to content

Add downsampling metrics#1244

Merged
bwplotka merged 4 commits intothanos-io:masterfrom
monzo:mattrco/downsample-metrics
Jun 12, 2019
Merged

Add downsampling metrics#1244
bwplotka merged 4 commits intothanos-io:masterfrom
monzo:mattrco/downsample-metrics

Conversation

@mattrco
Copy link
Contributor

@mattrco mattrco commented Jun 11, 2019

When compact goes into its downsampling step, no app metrics are reported, which makes our dashboard look a bit bare for several hours (sometimes days if we have a backlog). Adding a couple of metrics means we can alert if no compactions or downsamples have happened in x period.

This is a little less clean than I'd like due to the two callers of downsampleBucket having slightly different contexts, however I think it's a reasonable compromise. Let me know what you think!

The metrics are labelled in the same way as compact metrics for ease of comparison.

Changes

Add thanos_compact_downsample_total and thanos_compact_downsample_failures_total downsampling metrics.

Verification

Tests are passing, will run with real workloads.

Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it makes total sense (: Thanks.

Let's move to MustRegister and then LGTM. 👍

@mattrco
Copy link
Contributor Author

mattrco commented Jun 12, 2019

Thanks for reviewing, have updated to use MustRegister 👍

Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, one nit, but not a blocker (:

}
}

type DownsampleMetrics struct {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it can be private, right?

Copy link
Member

@GiedriusS GiedriusS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor nit. Let me know if it makes sense. I think doing like I suggested makes the code base cleaner - there's less chance that we will have clashes between different "subsystems" who might try to register the same metrics

}
}()

metrics := newDownsampleMetrics(reg)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we use extprom.WrapRegistererWithPrefix("thanos_compact_downsample", reg) here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it makes sense but with thanos_compact prefix, but not a blocker.

func newDownsampleMetrics(reg *prometheus.Registry) *DownsampleMetrics {
m := new(DownsampleMetrics)

m.downsamples = prometheus.NewCounterVec(prometheus.CounterOpts{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then we would only have total here

@bwplotka
Copy link
Member

Merging in this form, we can improve that later once we actually need to reuse this component.

@bwplotka bwplotka merged commit a6dfe63 into thanos-io:master Jun 12, 2019
@bwplotka
Copy link
Member

Thanks @mattrco !

@mattrco mattrco deleted the mattrco/downsample-metrics branch June 18, 2019 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants