compactor: Consider incrementing either ...group_compactions_total or ...group_vertical_compactions_total not both.

[bwplotka]

This is why metric testing and e2e tests are awesome! I found some unexpected cases that were surprising. One of them is that on vertical compaction we increment both vertical and normal compaction, which is confusing as it looks like in total we had 2 compactions, not one.

I think we should increment just either normal or vertical or just add a label to this metric (better).

Marking as bug, let's fix it.

[bwplotka]

Related TODO:

thanos/test/e2e/compact_test.go

Line 511 in 7c5bea6

// TODO(bwplotka): This is confusing, should be either normal compaction or vertical not both (?)

and discussion: #2462 (review)

[brancz]

I personally think this is the right behavior. Along the lines of total and corresponding error metric where total always includes errors.

[bwplotka]

Thanks. In this case, maybe we should rename / clarify this in HELP (:

[brancz]

Agreed!

[pracucci]

I personally think this is the right behavior. Along the lines of total and corresponding error metric where total always includes errors.

Agree. It's also who TSDB metrics work:

• prometheus_tsdb_compactions_total (always increased)
• prometheus_tsdb_vertical_compactions_total (increased if the overlapping blocks are found during compaction)

[darshanime]

The TODO is already removed in 2654a10

The HELP text is currently:

		Name: "thanos_compact_group_compactions_total",
		Help: "Total number of group compaction attempts that resulted in a new block.",


		Name: "thanos_compact_group_vertical_compactions_total",
		Help: "Total number of group compaction attempts that resulted in a new block based on overlapping blocks.",

Any ideas on whether it change it? One possible alternative can be:

		Name: "thanos_compact_group_compactions_total",
		Help: "Total number of group compaction attempts that resulted in a new block regardless of cause.",


		Name: "thanos_compact_group_vertical_compactions_total",
		Help: "Total number of group compaction attempts that resulted in a new block based on overlapping blocks.",

[stale]

Hello 👋 Looks like there was no activity on this issue for last 30 days.
Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity for next week, this issue will be closed (we can always reopen an issue if we need!). Alternatively, use remind command if you wish to be reminded at some point in future.

[bwplotka]

Looks like this is done after all, thanks all for discussion (: