Expose a metric tracking the oldest timestamp of unshipper block in ingester #3705

pracucci · 2021-01-18T15:21:41Z

What this PR does:
Currently, there's no way to alert if any compacted block is not shipped from ingester to storage within a reasonable time. In this PR I'm proposing to introduce a new metric cortex_ingester_oldest_unshipped_block_timestamp_seconds tracking the unix timestamp of the oldest TSDB block not shipped to the storage yet.

Which issue(s) this PR fixes:
N/A

Checklist

Tests updated
Documentation added
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

pstibrany

LGTM, but I'd like to see your take on caching the value instead.

pstibrany · 2021-01-18T16:18:17Z

pkg/ingester/ingester_v2.go

+	// Read the list of shipper blocks. If the shipper metadata file doesn't exist
+	// (eg. because the shipper hasn't run yet) we're considering it as if no block
+	// has been shipped.
+	shipped, err := u.getShippedBlocks()


getShippedBlocks is used twice, each time we build a map from it. Perhaps getShippedBlocks should build and return map directly?

blocksToDelete() doesn't build a map of out getShipperBlocks(). It just iterates over it.

pkg/ingester/ingester_v2.go

pstibrany · 2021-01-18T16:31:21Z

pkg/ingester/ingester_v2.go

+
+	oldest := uint64(0)
+	for _, db := range i.TSDBState.dbs {
+		if ts, err := db.getOldestUnshippedBlockTime(); err == nil && (oldest == 0 || ts < oldest) {


Instead of computing this value on each metric collection, should we cache it and update after each call to the shipper? As it is now, each metric collection needs to read shipper json file for each tenant, which could be slow if there are many tenants.

I can benchmark it and then we can decide if extra complexity is worth it. But agree checking it.

I've written a simple benchmark (commit) and on my MacBook it takes 31μs/op. If you have 1K tenants, it would be 31ms. I've the feeling caching for it, at this stage, would just be premature optimisation.

BenchmarkUserTSDB_getOldestUnshippedBlockTime-12 37930 30898 ns/op 4486 B/op 26 allocs/op

Is this a valid benchmark? On a busy server getOldestUnshippedBlockTime will not benefit from heavy caching of all IO operations as much as it does in this standalone test.

I don't think it would be a premature optimization, just a better approach. For 1K tenants, current approach needs to open/read/close 1000 files on each metric collection (=3000 syscalls). Instead we could simply read one atomic variable per tenant, and update it only when it can actually change. I don't see it as "premature optimization".

+1, I share concern this could be pretty inefficient for large number of tenants.

Ok, got it. I pushed a change to keep the shipped blocks in memory (they're tiny) and reload them each time any block is uploaded. I've also took the opportunity to improve the tests to not use a mocked shipper but the real one backed by a filesystem bucket.

What do you think?

ranton256

Thanks for doing this, functionally, in terms of the metric exported it looks good to me, but I second pstibrany's concern about the cost of calculating the metric at scrape time so would prefer the alternative implementation suggested.

ranton256 · 2021-01-21T18:13:26Z

pkg/ingester/ingester_v2.go

+
+	oldest := uint64(0)
+	for _, db := range i.TSDBState.dbs {
+		if ts, err := db.getOldestUnshippedBlockTime(); err == nil && (oldest == 0 || ts < oldest) {


+1, I share concern this could be pretty inefficient for large number of tenants.

pracucci · 2021-01-22T11:06:53Z

CHANGELOG.md

@@ -68,6 +68,7 @@
 * [ENHANCEMENT] Distributor: change the error message returned when a received series has too many label values. The new message format has the series at the end and this plays better with Prometheus logs truncation. #3718
  - From: `sample for '<series>' has <value> label names; limit <value>`
  - To: `series has too many labels (actual: <value>, limit: <value>) series: '<series>'`
+* [ENHANCEMENT] Ingester: exposed metric `cortex_ingester_oldest_unshipped_block_timestamp_seconds`, tracking the unix timestamp of the oldest TSDB block not shipped to the storage yet. #3705


We've just released 1.7.0-rc.0. Could you rebase master and move the CHANGELOG entry under the master / unreleased section, please?

ranton256

Thanks for the update.

ranton256 · 2021-01-22T19:10:27Z

pkg/ingester/ingester_v2.go

-	shipTicker := time.NewTicker(i.cfg.BlocksStorageConfig.TSDB.ShipInterval)
+	// We add a slight jitter to make sure that if the head compaction interval and ship interval are set to the same
+	// value they don't clash (if they both continuously run at the same exact time, the head compaction may not run
+	// because can't successfully cas the state).


What do you mean by "cas the state"? Not sure if I'm confused about a term I don't know or a typo ?

By cas I mean "Compare And Swap" (we have a function called casState()). Maybe "CAS" (upper case) is more clear? Should I explode it to "compare and swap"?

pstibrany

👍 Thanks!

Also nice cleanup in the tests!

pkg/ingester/ingester_v2.go

…ngester Signed-off-by: Marco Pracucci <marco@pracucci.com>

Signed-off-by: Marco Pracucci <marco@pracucci.com>

pracucci requested a review from pstibrany January 18, 2021 15:21

pull-request-size bot added the size/L label Jan 18, 2021

pstibrany approved these changes Jan 18, 2021

View reviewed changes

pracucci force-pushed the expose-metric-with-oldest-unshipped-block-timestamp branch from ffab7ac to 9447e9a Compare January 21, 2021 14:54

ranton256 reviewed Jan 21, 2021

View reviewed changes

pracucci commented Jan 22, 2021

View reviewed changes

pracucci force-pushed the expose-metric-with-oldest-unshipped-block-timestamp branch from 9447e9a to 6318f48 Compare January 22, 2021 13:16

ranton256 approved these changes Jan 22, 2021

View reviewed changes

pstibrany approved these changes Jan 25, 2021

View reviewed changes

pkg/ingester/ingester_v2.go Show resolved Hide resolved

pracucci added 5 commits January 25, 2021 11:22

Expose a metric tracking the oldest timestamp of unshipper block in i…

3094e0f

…ngester Signed-off-by: Marco Pracucci <marco@pracucci.com>

Added benchmark

9f1bc4f

Signed-off-by: Marco Pracucci <marco@pracucci.com>

Load shipped blocks after each Sync()

e36501e

Signed-off-by: Marco Pracucci <marco@pracucci.com>

Improved tests and logic

571195a

Signed-off-by: Marco Pracucci <marco@pracucci.com>

Updated code comments

2866e6b

Signed-off-by: Marco Pracucci <marco@pracucci.com>

pracucci force-pushed the expose-metric-with-oldest-unshipped-block-timestamp branch from 74f2715 to 2866e6b Compare January 25, 2021 10:26

Fixed tests after rebase

abc75b9

Signed-off-by: Marco Pracucci <marco@pracucci.com>

pracucci merged commit f72ac74 into cortexproject:master Jan 25, 2021

pracucci deleted the expose-metric-with-oldest-unshipped-block-timestamp branch January 25, 2021 11:32

This was referenced Jan 25, 2021

Fixed CHANGELOG entry order #3742

Merged

Added alert CortexIngesterHasUnshippedBlocks grafana/cortex-jsonnet#255

Merged

pstibrany mentioned this pull request Mar 30, 2021

Remove error return value, which was always nil. #4029

Merged

Expose a metric tracking the oldest timestamp of unshipper block in ingester #3705

Expose a metric tracking the oldest timestamp of unshipper block in ingester #3705

Uh oh!

Conversation

pracucci commented Jan 18, 2021

Uh oh!

pstibrany left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ranton256 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ranton256 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pstibrany left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!