fix: updates for tsdb blocks handling by tjhop · Pull Request #1928 · prometheus/client_golang

tjhop · 2025-12-15T07:37:13Z

Two small fixes:

I forgot the implementation for *apiClientImpl.Do() already extracts
the response content from the enveloped data field in the response,
so the wrapper struct isn't needed here.
The compaction section of a block has an optional parents field
for tracking block lineage in deeper compaction levels. This slipped
my mind during implementation because I was looking at the example API
response on the API docs, which use a mock response with a single block
that is uncompacted.

I noticed these while working on prometheus/prometheus-mcp#77

Signed-off-by: TJ Hoplock t.hoplock@gmail.com

kakkoyun

Considering these issues slipped through the cracks, I would really appreciate if you could add more comprehensive tests so that we prevent similar future issues.

Thanks in advance 🙏

tjhop · 2025-12-15T18:37:59Z

That's fair, I can expand tests.

Ideally, it would be simpler to just import prometheus itself as a library and use the types already defined for these in prometheus. That would obviously help to keep things in sync and benefit from existing testing there. It becomes a bit awkward though, since prometheus itself also imports the client_golang library (mostly for implementing metrics) and so we'd be importing ourselves and nearing a circular import. It would also blow up dependencies, and I know this library is already heavily used/strictly watched for deps.

Perhaps this is a situation where a little bit of copying is better than a little bit of dependency?

tjhop · 2026-01-30T14:15:08Z

Both of the checks that are failing are for TestGoCollectorAllowList which is unrelated to this PR, so I think we should be good?

@kakkoyun sorry it's been a bit -- if you get some more time, I'd appreciate some fresh eyes please!

tjhop · 2026-03-06T04:25:07Z

Both of the test failures in CI are for collectors/ and are unrelated to the changes here 👍

Cover the edge cases that let the original implementation gaps slip through, per review feedback on prometheus#1928: - empty and null `blocks` lists - a block with every optional stats/compaction field populated, including `parents` lineage - multiple blocks at different compaction levels - missing optional `stats` field deserializing to zero values - a `from-out-of-order` compaction hint (the literal upstream's CompactionHintFromOutOfOrder constant emits) - zero-value time boundaries and non-default `version` values All `inRes` payloads model the post-envelope body, matching what `apiClientImpl.Do()` actually hands to `TSDBBlocks()`. Signed-off-by: TJ Hoplock <t.hoplock@gmail.com>

tjhop · 2026-08-15T05:48:08Z

@kakkoyun @bwplotka @ArthurSens would you mind taking a fresh look at this? Now that 1.24.x is out, this will be a breaking change.

To the point about improving tests, in addition to the expanded test suite and edge cases covered I also used claude to help create a parity test -- its a separate test suite in a separate go mod that uses prometheus as a library with our upstream prom version's code and compares it against client_golang's counterparts. It also wires up a separate CI job to bump prometheus versions and fail loudly if the parity test fails. Thoughts?

bwplotka

Thanks!

Maybe I am not used to not worrying about extra code in the AI era, but I would prefer we don't reimplement block creation or use internal Prometheus DB for niche API like TSDB block API that we didn't notice it was broken (:

I wonder if we can focus on long term testing for those things - for now I am ok to fix it and merge and figure out e2e testing later. There is also #1998 which is an interesting alternative.

If we really need testing, then I'd vote for reflection based solution only (diff of structs).

Cover the edge cases that let the original implementation gaps slip through, per review feedback on prometheus#1928. To keep CI time down, the quirks share a single success case rather than one case each: - an empty `blocks` list - a fully populated block with every optional stats/compaction field, including `parents` lineage and the `from-out-of-order` compaction hint (the literal upstream's CompactionHintFromOutOfOrder constant emits), with stats values past 2^32 so narrowing any uint64 stats field fails the test - a minimal uncompacted block with no `stats` field, deserializing to zero values - unknown block-level and stats-level fields, which the decoder must ignore All `inRes` payloads model the post-envelope body, matching what `apiClientImpl.Do()` actually hands to `TSDBBlocks()`. Assisted-by: Claude Code:claude-fable-5 <noreply@anthropic.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017HR97tymenb3uBtmUA4ZPg Signed-off-by: TJ Hoplock <t.hoplock@gmail.com>

The v1 API TSDB block types are hand-copied from prometheus/prometheus (importing it from the main module would be near-circular, since Prometheus imports client_golang), so nothing ties them to upstream at compile time. Add a standalone test module, api/prometheus/v1/paritytest, that imports both and diffs the struct definitions field by field via reflection: JSON names, full tags (including omitempty), and field types, with the client's deliberate ulid.ULID-to-string mapping canonicalized away. Per review feedback on prometheus#1928, the check is reflection-only; it depends on nothing but the exported type definitions, so upstream refactors of TSDB internals can't break it. Mutation-tested: renaming a client json tag or narrowing a stats field to uint32 fails with a field-level diff. The separate module keeps prometheus/prometheus out of client_golang's module graph; it is a leaf, so consumers are unaffected. It replaces client_golang with the in-tree checkout and pins a prometheus release that dependabot bumps, so upstream type changes fail the parity test in the bump PR. test-parity is deliberately not part of `make test`: compiling the pinned Prometheus against the in-tree client_golang on every PR would couple all of CI to the pin, with incompatibilities surfacing as compile errors in third-party code on unrelated jobs. It runs as a dedicated path-filtered Parity workflow instead -- a single job on the stable Go release, since a struct diff doesn't vary across Go versions -- plus a weekly run against prometheus@latest as a backstop in case dependabot never materializes a bump for the replace-containing module. Assisted-by: Claude Code:claude-fable-5 <noreply@anthropic.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017HR97tymenb3uBtmUA4ZPg Signed-off-by: TJ Hoplock <t.hoplock@gmail.com>

Cover the edge cases that let the original implementation gaps slip through, per review feedback on prometheus#1928. To keep CI time down, the quirks share a single success case rather than one case each: - an empty `blocks` list - a fully populated block with every optional stats/compaction field, including `parents` lineage and the `from-out-of-order` compaction hint (the literal upstream's CompactionHintFromOutOfOrder constant emits), with stats values past 2^32 so narrowing any uint64 stats field fails the test - a minimal uncompacted block with no `stats` field, deserializing to zero values - unknown block-level and stats-level fields, which the decoder must ignore All `inRes` payloads model the post-envelope body, matching what `apiClientImpl.Do()` actually hands to `TSDBBlocks()`. Signed-off-by: TJ Hoplock <t.hoplock@gmail.com>

tjhop · 2026-09-11T03:41:22Z

Thanks @bwplotka, all feedback should be addressed, mind taking another look when time allows?

bwplotka

Thanks!

bwplotka · 2026-09-11T12:18:32Z

Hm, I did break something by changing CHANGELOG? 🤔

Cover the edge cases that let the original implementation gaps slip through, per review feedback on prometheus#1928. To keep CI time down, the quirks share a single success case rather than one case each: - an empty `blocks` list - a fully populated block with every optional stats/compaction field, including `parents` lineage and the `from-out-of-order` compaction hint (the literal upstream's CompactionHintFromOutOfOrder constant emits), with stats values past 2^32 so narrowing any uint64 stats field fails the test - a minimal uncompacted block with no `stats` field, deserializing to zero values - unknown block-level and stats-level fields, which the decoder must ignore All `inRes` payloads model the post-envelope body, matching what `apiClientImpl.Do()` actually hands to `TSDBBlocks()`. Signed-off-by: TJ Hoplock <t.hoplock@gmail.com>

I forgot the implementation for `*apiClientImpl.Do()` already extracts the response content from the enveloped `data` field in the response, so the wrapper struct isn't needed here. Aligns with the upstream `tsdb.BlockMeta` types, while also adding fields missed in the initial implementation and match upstream JSON tags exactly (notably `omitempty` on all stats fields and on sources) so re-marshaling these types produces the same wire format Prometheus emits. Note: This changes the public API surface and is a breaking change, now that 1.24.0 has been officially cut, so changelog has been updated as well. Signed-off-by: TJ Hoplock <t.hoplock@gmail.com>

Cover the edge cases that let the original implementation gaps slip through, per review feedback on prometheus#1928. To keep CI time down, the quirks share a single success case rather than one case each: - an empty `blocks` list - a fully populated block with every optional stats/compaction field, including `parents` lineage and the `from-out-of-order` compaction hint (the literal upstream's CompactionHintFromOutOfOrder constant emits), with stats values past 2^32 so narrowing any uint64 stats field fails the test - a minimal uncompacted block with no `stats` field, deserializing to zero values - unknown block-level and stats-level fields, which the decoder must ignore All `inRes` payloads model the post-envelope body, matching what `apiClientImpl.Do()` actually hands to `TSDBBlocks()`. Signed-off-by: TJ Hoplock <t.hoplock@gmail.com>

The v1 API TSDB block types are hand-copied from prometheus/prometheus (importing it from the main module would be near-circular, since Prometheus imports client_golang), so nothing ties them to upstream at compile time. Add a standalone test module, api/prometheus/v1/paritytest, that imports both and diffs the struct definitions field by field via reflection: JSON names, full tags (including omitempty), and field types, with the client's deliberate ulid.ULID-to-string mapping canonicalized away. Per review feedback on exported type definitions, so upstream refactors of TSDB internals can't break it. Mutation-tested: renaming a client json tag or narrowing a stats field to uint32 fails with a field-level diff. The separate module keeps prometheus/prometheus out of client_golang's module graph; it is a leaf, so consumers are unaffected. It replaces client_golang with the in-tree checkout and pins a prometheus release that dependabot bumps, so upstream type changes fail the parity test in the bump PR. test-parity is deliberately not part of `make test`: compiling the pinned Prometheus against the in-tree client_golang on every PR would couple all of CI to the pin, with incompatibilities surfacing as compile errors in third-party code on unrelated jobs. Signed-off-by: TJ Hoplock <t.hoplock@gmail.com>

Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com> Signed-off-by: TJ Hoplock <t.hoplock@gmail.com>

We have modernize enabled upstream now. The `omitzero` analyzer flags omitempty on nested struct fields as a no-op, but we're doing it explicitly to match upstream, so just skip it. Signed-off-by: TJ Hoplock <t.hoplock@gmail.com>

The apiResponse decode benchmark added on main (prometheus#2118) builds its TSDBBlocksResult case against the pre-fix types, which this branch flattens and renames to match upstream tsdb.BlockMeta. The branch compiled on its own, but CI builds the merge ref, where the benchmark failed to compile: api_bench_test.go:227:3: unknown field Status in struct literal of type TSDBBlocksResult api_bench_test.go:228:9: undefined: TSDBBlocksData api_bench_test.go:229:14: undefined: TSDBBlocksBlockMetadata api_bench_test.go:233:12: undefined: TSDBBlocksStats api_bench_test.go:238:17: undefined: TSDBBlocksCompaction Rewrite the case with TSDBBlockMeta, TSDBBlockStats and TSDBBlockMetaCompaction, dropping the Status/Data envelope that the client already strips before decoding. Signed-off-by: TJ Hoplock <t.hoplock@gmail.com>

Signed-off-by: TJ Hoplock <t.hoplock@gmail.com>

tjhop · 2026-10-04T16:46:27Z

@bwplotka I got CI fixed up in the last few commits, if yo have time to look again!

bwplotka

LGTM prod code

I am not a fan of having that parity test - prom lib is heavy and even on tests and with dependabot there will be some silly PR dep merging. Perhaps automerge + renovate would reduce the pain. Long term we could move those structs to common/model 🤔

Let's go for now!

tjhop · 2026-10-04T19:37:25Z

I don't disagree! Personally, I'd love to see us move towards #1998

tjhop requested review from ArthurSens, bwplotka, kakkoyun and vesari as code owners December 15, 2025 07:37

kakkoyun requested changes Dec 15, 2025

View reviewed changes

tjhop force-pushed the fix/tsdb-blocks-updates branch from 60bdf9b to d78b175 Compare January 29, 2026 21:56

tjhop force-pushed the fix/tsdb-blocks-updates branch from 68fb72f to 4765ce8 Compare March 6, 2026 04:19

kakkoyun approved these changes Mar 9, 2026

View reviewed changes

tjhop force-pushed the fix/tsdb-blocks-updates branch from 4765ce8 to 076f4ef Compare August 15, 2026 04:06

tjhop force-pushed the fix/tsdb-blocks-updates branch from 076f4ef to 4d95e8b Compare August 15, 2026 04:07

tjhop force-pushed the fix/tsdb-blocks-updates branch from 4d95e8b to df2c55a Compare August 15, 2026 04:28

bwplotka requested changes Aug 18, 2026

View reviewed changes

tjhop force-pushed the fix/tsdb-blocks-updates branch from e070a27 to 20e414e Compare August 28, 2026 03:32

tjhop force-pushed the fix/tsdb-blocks-updates branch from 20e414e to be62d9b Compare August 28, 2026 03:35

tjhop requested a review from bwplotka September 11, 2026 03:40

bwplotka approved these changes Sep 11, 2026

View reviewed changes

Comment thread CHANGELOG.md Outdated

bwplotka enabled auto-merge September 11, 2026 10:02

bwplotka disabled auto-merge September 11, 2026 12:18

tjhop force-pushed the fix/tsdb-blocks-updates branch from fb370b9 to 75fdd92 Compare October 4, 2026 02:17

tjhop and others added 7 commits October 3, 2026 23:24

Update CHANGELOG.md

d8da8e9

Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com> Signed-off-by: TJ Hoplock <t.hoplock@gmail.com>

chore: go fmt updates for tutorials

9abf0e2

Signed-off-by: TJ Hoplock <t.hoplock@gmail.com>

tjhop force-pushed the fix/tsdb-blocks-updates branch from 75fdd92 to 9abf0e2 Compare October 4, 2026 03:37

bwplotka approved these changes Oct 4, 2026

View reviewed changes

bwplotka merged commit 9da3fcc into prometheus:main Oct 4, 2026
15 checks passed

kakkoyun mentioned this pull request Oct 5, 2026

Cut v1.25.0-rc.0 #2154

Merged

This was referenced Oct 12, 2026

Update module github.com/prometheus/client_golang to v1.25.0 (main) stackitcloud/cloud-provider-stackit#1772

Merged

Update module github.com/prometheus/client_golang to v1.25.0 - autoclosed stackitcloud/machine-controller-manager-provider-stackit#217

Closed

Conversation

tjhop commented Dec 15, 2025

Uh oh!

kakkoyun left a comment

Choose a reason for hiding this comment

Uh oh!

tjhop commented Dec 15, 2025

Uh oh!

tjhop commented Jan 30, 2026

Uh oh!

tjhop commented Mar 6, 2026

Uh oh!

tjhop commented Aug 15, 2026

Uh oh!

bwplotka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tjhop commented Sep 11, 2026

Uh oh!

bwplotka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bwplotka commented Sep 11, 2026

Uh oh!

tjhop commented Oct 4, 2026

Uh oh!

bwplotka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tjhop commented Oct 4, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants