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).

bwplotka · 2026-08-18T11:33:17Z

+
+// writeBlock writes a block with float and native histogram samples to
+// dir and returns its ULID.
+func writeBlock(t *testing.T, dir string, seed int64) ulid.ULID {


This is likely an overkill -- tons of complexity that can change anytime (Prom often adds breaking changes to those e.g. TSDB internal methods).
This will also not catch the other bug around "data" parsing.

Here it would be enough to compare if structs match (eg. .reflection) or even regenerate struct with some go generator and put that into a test, but instead I wonder - should we invest in using OpenAPI spec (PoC) instead?

Agreed it's above and beyond, the goal was to more functionally exercise parity, but I hear where you're coming from. I can drop the commit and rework to use reflection for now -- I didn't know about the openapi PoC and would vote to focus on that as the better long term effort 👍

bwplotka · 2026-08-18T11:33:57Z

 	}

 	if gotErr {
 		fmt.Println("Got this response from ", fmt.Sprintf("http://localhost:%v", WhatsupPort), ":", metrics)


Let's minimize unrelated changes

Totally fair. this was added purely to get CI to pass and kept in a separate commit for the go fmt. I'll drop the commit 👍

bwplotka · 2026-08-18T11:38:48Z

+			},
+		},
+
+		// Multiple blocks at different compaction levels.


Do we care about this case? Nothing changes from the API standpoint here - less tests to maintain the better

bwplotka · 2026-08-18T11:39:19Z

+			},
+		},
+
+		// Null blocks field (server returns null instead of empty array).


Do we need to test Go unmarshalling here? We don't do this for other API endpoints.

bwplotka · 2026-08-18T12:11:34Z

+		// expected literal below a compile error and the decode a
+		// truncation failure.
+		{
+			do:        doTSDBBlocks(),


Can we have a single case that have all of those quirks? (CI time is important)

bwplotka · 2026-08-18T12:55:54Z

+							},
+						},
+						"version": 1,
+						// Note: no "stats" field


Again, could be in one case if we want

bwplotka · 2026-08-18T12:55:58Z

+			},
+		},
+
+		// Block with the compaction hint Prometheus's OOO feature emits


bwplotka · 2026-08-18T12:56:13Z

+			},
+		},
+
+		// Block with zero-value time boundaries (edge case).


Perhaps too much? Could be removed?

bwplotka · 2026-08-18T12:56:35Z

+			},
+		},
+
+		// Block with different version values.


Again, not really changing anything - should be part of a single case

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>

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>

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 added 3 commits August 27, 2026 23:33

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

Uh oh!

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!

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants