Skip to content

tsdb: add count of histogram samples to block stats #16824

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Jul 8, 2025
Merged

tsdb: add count of histogram samples to block stats #16824

merged 2 commits into from
Jul 8, 2025

Conversation

afhassan
Copy link
Contributor

@afhassan afhassan commented Jul 3, 2025 •
edited
Loading

Currently TSDB metadata tracks the following stats

type BlockStats struct {
	NumSamples          uint64 `json:"numSamples,omitempty"`
	NumSeries           uint64 `json:"numSeries,omitempty"`
	NumChunks           uint64 `json:"numChunks,omitempty"`
	NumTombstones       uint64 `json:"numTombstones,omitempty"`
}

Native histogram samples have different properties compared to normal float/int samples, and can vary a lot in size. While they are counted as samples in numSamples, it is useful to have a separate metric that tracks how many native histograms samples there are in a block.

This PR introduces a numHistogramSamples stat without touching any of the other existing stats.

	NumHistogramSamples uint64 `json:"numHistogramSamples,omitempty"`

@afhassan
add numHistogramSamples to block stats
6d77b47 
Signed-off-by: Ahmed Hassan <afayekhassan@gmail.com>
@afhassan afhassan marked this pull request as ready for review July 3, 2025 17:34
@afhassan afhassan requested a review from jesusvazquez as a code owner July 3, 2025 17:34
@harry671003
Copy link
Contributor

LGTM! It'll be good to keep track of Native histogram samples in blocks separately.

@beorn7 beorn7 requested review from beorn7 and krajorama and removed request for jesusvazquez July 4, 2025 18:59
bwplotka
bwplotka approved these changes Jul 7, 2025
View reviewed changes
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.

LGTM, but I would wait for others e.g. @bboreham or @beorn7 to double check

e.g. I would prefer numSamples breaking change to be only float samples in general, but if we comment things right it's fine for an additive change for now I presume.

@krajorama
Copy link
Member

LGTM, but I would wait for others e.g. @bboreham or @beorn7 to double check

e.g. I would prefer numSamples breaking change to be only float samples in general, but if we comment things right it's fine for an additive change for now I presume.

In my head "Sample" is a generic word, can be float sample or native histogram sample, so I don't think it's technically wrong. Which also means that I think the numbers should look like this:

NumSamples
NumFloatSamples
NumHistogramSamples

or breaking:

NumFloatSamples
NumHistogramSamples

But you can get float samples with a simple subtract and also people generally don't care, it's more about getting info on native histograms.

Note: I don't think we even documented these block stats anyway.

krajorama
krajorama approved these changes Jul 7, 2025
View reviewed changes
Copy link
Member

@krajorama krajorama left a comment

Choose a reason for hiding this comment

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

approve with nit

@afhassan
add NumFloatSamples to TSDB block stats
01be7bf 
Signed-off-by: Ahmed Hassan <afayekhassan@gmail.com>
beorn7
beorn7 approved these changes Jul 8, 2025
View reviewed changes
Copy link
Member

@beorn7 beorn7 left a comment

Choose a reason for hiding this comment

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

Thank you.

@beorn7
Copy link
Member

beorn7 commented Jul 8, 2025

I like the explicit trinity of NumSamples/NumFloatSamples/NumHistogramSamples. No breaking change, and no misunderstandings possible after having see the three names.

@beorn7 beorn7 merged commit d8c9218 into prometheus:main Jul 8, 2025
27 checks passed
@krajorama krajorama mentioned this pull request Jul 9, 2025
Merged
krajorama added a commit to grafana/mimir that referenced this pull request Jul 10, 2025
@mimir-vendoring @mimir-github-bot
@krajorama @charleskorn
[main] Update mimir-prometheus to 832330026356 (#12032)
9d54133 
## Manual follow-ups

@krajorama : I've updated the engine tests from upstream
(prometheus/prometheus#16725). There was one
that has a different error message with MQE so I only added it to *ours*
tests. Maybe we want to fix the error message instead? Commented.

@krajorama : I've followed up the block stats changes
(prometheus/prometheus#16824) by adding the
necessary code in Mimir and also more tests. I've followed
BlockStats.NumSamples to find all occurrences. Even if I wasn't
successful , we don't actually use the new fields for anything critical
- so this should be safe.

## Update mimir-prometheus dependency

*This PR was automatically created by the
[update-vendored-mimir-prometheus.yml](https://github.com/grafana/mimir/blob/main/.github/workflows/update-vendored-mimir-prometheus.yml)
workflow.*

### Details:
- **Source branch/ref**:
[`main`](https://github.com/grafana/mimir-prometheus/tree/main)
- **Previous commit**:
[`609b9c7c04c1`](grafana/mimir-prometheus@609b9c7c04c1)
- **New commit**:
[`8323300263566d178c6d571d985924fbd4270225`](grafana/mimir-prometheus@8323300)
- **Changes**:
[`609b9c7c04c1...8323300263566d178c6d571d985924fbd4270225`](grafana/mimir-prometheus@609b9c7...8323300)

---------

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
Co-authored-by: mimir-github-bot[bot] <mimir-github-bot[bot]@users.noreply.github.com>
Co-authored-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
Co-authored-by: Charles Korn <charles.korn@grafana.com>
@machine424
Copy link
Member

machine424 commented Aug 11, 2025 •
edited
Loading

do we want to expose this on /api/v1/status/tsdb as well?

prometheus/web/api/v1/api.go

Line 1724 in 294f36e

// HeadStats has information about the TSDB head.

maybe as a part of the new /status/tsdb/blocks #16695 as it's about persistent blocks.

Well, it's already the case, it's exposed on /status/tsdb/blocks. You can ignore this.

@bboreham bboreham mentioned this pull request Aug 13, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@beorn7 beorn7 beorn7 approved these changes

@bwplotka bwplotka bwplotka approved these changes

@krajorama krajorama krajorama approved these changes

+1 more reviewer

@harry671003 harry671003 harry671003 approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@afhassan @harry671003 @krajorama @beorn7 @machine424 @bwplotka