promhttp: Isolate zstd support and klauspost/compress library use to promhttp/zstd package

[liggitt]

This PR is a proof of concept for package isolation discussed in kubernetes/kubernetes#130569 (comment)

Eventually, zstd support is expected in the Go stdlib, but until then, making zstd support optional lets downstream consumers decide if they want to vendor / link the ~28kloc go+assembly klauspost/compress library into their build or not.

This does change default behavior for library consumers who would now have to opt into zstd support, so I'll let the client_golang maintainers decide if that's acceptable, and if so, how best to manage that change.

xref discussion at https://github.com/prometheus/client_golang/pull/1496/files#r1717633170 as well when this was introduced

[kakkoyun]

LGTM.

We'd be happy to merge this. However, this changes the current default behavior (We support zstd compression since 1.20 if it's specified in compression header).

We need to decided how to communicate this.

Or we will try to find an opt-out method. So please stay tuned for now.

[bwplotka]

Thanks for proposing this, breaking change is one option. One line import should be easy to add app. @kakkoyun offered to release 1.22 with this and clear messaging and it's a solid idea.

cc @mrueg do you think that would work for you? It was mostly done for your use case.

However, there is perhaps one more thing I want to try: vendor https://github.com/klauspost/compress/tree/master/zstd temporarily without ASM code. We can do patch release or 1.22 (as @kakkoyun said 1.22 might be cleaner) and this should allow us to likely not break anyone, we will only make zstd ~15% slower or so, trying now.

[liggitt]

Thanks for considering it and investigating possibilities, appreciate the help in making this module as reasonable to consume as possible

[bwplotka]

The alternative that does not break compatibility is here: #1767

I need to clean the PR, fix tests (old Dagger version), but we could do it. Blocking this PR until we have decision which path we prefer to go.

[mrueg]

Apologies if the PR caused issues downstream. The zstd stdlib issue looked promising, so I had hoped it gets much quicker integrated there. I'm fine with this solution (and slightly would prefer the one that includes the ASM), I can adjust my code easily and I don't believe anyone else is actively using it. Appreciate the work on both PRs!

[bwplotka]

Thanks @mrueg, then let's go this path (instead of #1767) and release 1.22. Will be much easier for everyone. Thanks @liggitt @kakkoyun 💪🏽

QQ is 1.22 release ok from your standpoint @liggitt? I know cadvisor depends on 1.20.5 or so.

[bwplotka]

Are you around tomorrow @kakkoyun to shepherd the release?

[liggitt]

QQ is 1.22 release ok from your standpoint @liggitt? I know cadvisor depends on 1.20.5 or so.

Do you know if there's incompatibilities from ~1.20.5 to 1.22 that would require cadvisor / etc to change usage (the relaxed label name validation in 1.21.0 doesn't seem impactful at first glance)? If not, we're fine with bumping k8s to a newer version of this library. If so, we'll need to check with maintainers of intermediate dependencies like cadvisor to see if they can pick up 1.22 and react and cut tags.

[liggitt]

I also opened a tmp kubernetes PR at kubernetes/kubernetes#130625 to run this change through kubernetes CI ahead of merge / tag here, just to make sure k8s build and e2e's are all happy as well

[bwplotka]

Only the name validation, so it feels 1.22 should work for you well 👍🏽

[kakkoyun]

LGTM

[liggitt]

I also opened a tmp kubernetes PR at kubernetes/kubernetes#130625 to run this change through kubernetes CI ahead of merge / tag here, just to make sure k8s build and e2e's are all happy as well

All the required k8s e2es, unit and integration tests passed on the test PR as well, so that looks good