prometheus / common

Go libraries shared across Prometheus components and libraries.
Apache License 2.0
259 stars 306 forks source link

Consolidate content type and content encoding functions in prometheus/common #652

Open mrueg opened 2 weeks ago

mrueg commented 2 weeks ago

I recently added zstd support to client_golang in https://github.com/prometheus/client_golang/pull/1496 which uses an internalized version of the archived repository: https://github.com/golang/gddo/blob/master/httputil/negotiate.go

I see that https://github.com/prometheus/common/blob/2d5ba4a4a8854b224d06df286a191f710fc65785/expfmt/encode.go#L67 was recently changed to use https://github.com/munnerz/goautoneg for contentType logic in https://github.com/prometheus/common/pull/625

Would you be open to include the logic for content encoding implemented in client_go in here and replace goautoneg with ggdo/httputil (we could either consume from the archived repository or internalize it into this repository; we took the second approach for client_golang)?

This likely would help with consolidating the library for content encoding as well as content type and allow reusing parts of it in prometheus/prometheus as well (e.g. in https://github.com/prometheus/prometheus/issues/13866)

CC: @bwplotka

ArthurSens commented 2 weeks ago

Content type/encoding negotiation seems to be getting importance in the prometheus ecosystem, used in instrumentation libraries and very soon in PRW senders and receivers.

I think it makes sense to have this logic in common to avoid having several implementations of the same thing in the ecosystem 👍

With that said, I don't have enough context to tell if it's fine to replace goautoneg with something else. Maybe @SuperQ or @gotjosh ?

gotjosh commented 2 weeks ago

I think @bwplotka should have an opinion here, but I'm unsure if he'll ever get to see this.

Consolidating usages can only be a good thing -- but I'm unsure which approach is best to be honest. On the one hand, big projects such as Kubernetes use [munnerz/goautoneg](https://github.com/munnerz/goautoneg). Why did we deviate from that implementation, to begin with?

mrueg commented 2 weeks ago

I think @bwplotka should have an opinion here, but I'm unsure if he'll ever get to see this.

Consolidating usages can only be a good thing -- but I'm unsure which approach is best to be honest. On the one hand, big projects such as Kubernetes use [munnerz/goautoneg](https://github.com/munnerz/goautoneg). Why did we deviate from that implementation, to begin with?

Autoneg unfortunately only takes care of the accept header and not the accept encoding header. I still hope content type and encoding negotiation will be part of the stdlib some day. https://github.com/golang/go/issues/19307

gotjosh commented 2 weeks ago

If autoneg is missing something then I'd say we port over golang_client's implementation here and replace than everywhere.

bwplotka commented 2 weeks ago

I am generally supportive of this, we need this in client_golang, but we can't really expose it to 3rd party callers due to scope and stability guarantees.

Common is 0.x, so we could experiment.

There is a counter argument or question.. who else would use it than client_golang, in the Prometheus ecosystem. "It" being auto negotiating accept headers in RFC 9110 style.

@ArthurSens not sure if I agree with PRW. We abandoned the auto negotiation idea so we only need to parse "one element" of accept header (so request content enc/type in receiver to understand what's being send), so I don't think we will this particular function we had in mind.

I think @mrueg mentioned KSM. If that's true (we can use it 1:1),then to me it's a strong enough use case.

I think it's fine to vendor this simple package, looks simple enough.

ArthurSens commented 2 weeks ago

If client_golang is the only user of this, I don't see why we should move the logic to common, to be honest. Let's see if Manuel has a use case for it somewhere else :)

mrueg commented 1 week ago

kube-state-metrics is using the gzip part here https://github.com/kubernetes/kube-state-metrics/blob/main/pkg/metricshandler/metrics_handler.go#L200 I would like to extend it with zstd support (as ksm creates out a lot of timeseries) Ideally I'd reuse it from prometheus/common as ksm is not using client_golang to set up the metricsserver.

In addition, there's prometheus/prometheus which might want to reuse it as well for encoding negotiation

ArthurSens commented 5 days ago

Ideally I'd reuse it from prometheus/common as ksm is not using client_golang to set up the metricsserver.

I'm not sure if I understand this part (probably because I'm not familiar with ksm codebase), the problem you want to avoid is adding another dependency in ksm? I can see client_golang on the go.mod file, so I'm a bit confused.

I'm pretty sure that the phrase above is not your real concern, but I couldn't think of anything else 😅, could you elaborate a bit?

In addition, there's prometheus/prometheus which might want to reuse it as well for encoding negotiation

Yeah, I can see this happening too! Could we defer the decision once Prometheus commits to encoding negotiation? :)