Set OpenMetrics content header

[mrueg]

What this PR does / why we need it:

This change will expose metrics in openmetrics' format (basically just adding the right content header), as otherwise info and stateset type metrics won't be recognized by prometheus.

See: https://github.com/prometheus/common/blob/main/expfmt/encode.go#L86

See: #1973

How does this change affect the cardinality of KSM: (increases, decreases or does not change cardinality)
None

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #1973

[k8s-ci-robot]

This issue is currently awaiting triage.

If kube-state-metrics contributors determine this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[dgrisonnet]

Can we really support OpenMetrics without changing the output of kube-state-metrics? The OpenMetrics format is slighly different from the Prometheus one that we currently hardcode

[mrueg]

@dgrisonnet good idea to check it again.

https://www.robustperception.io/checking-openmetrics-output-is-valid/

I used the check above and added a # EOF to our custom metrics writing path.

Edit:
I've disabled OpenMetrics on the telemetry exporter due to prometheus/client_golang#829

[dgrisonnet]

I wonder if we should really do that by default. I would only expect us to modify the content type when an OpenMetrics option is enabled. Otherwise we could break existing proxies only allowing text/plain content type.

[mrueg]

moved it to the main thread, since this change is outdated.

[mrueg]

I've changed the implementation to use the expfmt negotiation.

You can test it with:
curl -v --header "Accept: application/openmetrics-text"

curl -v --header "Accept:text/plain"

Be aware that this can still generate invalid prometheus format (see #1973) in case the client requests application/text and info/stateset metrics are requested. This might be more difficult to solve as we would need to go through all metrics before writing them and replace the metric type based on the client's request.

[CatherineF-dev]

Once encountered some issues with open-telemetry + openmetrics. Maybe add a feature-gate for OpenMetrics content header?
(In remembering which tricky issue)

[mrueg]

Once encountered some issues with open-telemetry + openmetrics. Maybe add a feature-gate for OpenMetrics content header? (In remembering which tricky issue)

I would like to avoid adding that if possible as the client already has a way to negotiate by presenting the txtFmt Accept Header.

[dgrisonnet]

Nice one. I was gonna suggest to use expfmt negociation but you already made that change.

We should add a simple e2e test as a follow-up.

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dgrisonnet, mrueg

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [dgrisonnet,mrueg]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[Joseph-Irving]

I just thought I'd mention this issue prometheus/prometheus#12121 as I think it may be related to this PR.

[CatherineF-dev]

Could you help test the issue will be gone without this PR? cc @Joseph-Irving

[CatherineF-dev]

Or could provide steps to reproduce this issue? Thanks!

[Joseph-Irving]

So I can reproduce this just on minikube with the standard helm prometheus, chart https://github.com/prometheus-community/helm-charts/tree/main/charts/prometheus

I ran

helm install prometheus prometheus-community/prometheus --set server.extraFlags={"enable-feature=native-histograms"} --version=19.7.2

This deploys kube-state-metrics version: registry.k8s.io/kube-state-metrics/kube-state-metrics:v2.8.0
And Prometheus version: quay.io/prometheus/prometheus:v2.41.0
With the --enable-feature=native-histograms set.

This Prometheus fails to scrape Kube-state-metrics with the error: proto: wrong wireType = 0 for field Metric
I rebuilt Kube-state-metrics without this PR and it started working as normal.

As I mentioned in the Prom issue prometheus/prometheus#12121 (comment) when this feature is turned on it changes the headers for Prometheus scrapes.

[Joseph-Irving]

So I think the issue, is that when you enable Native Histograms Prometheus is sending headers with a content type of:

content type: %v application/vnd.google.protobuf; proto=io.prometheus.client.MetricFamily; encoding=delimited

Which we're then mirroring in our headers:

resHeader.Set("Content-Type", string(contentType))

But we're not actually changing our response so we're replying with invalid protobuf hence the error from Prometheus.

I did this as a simple workaround and it fixed the issue:

if contentType == expfmt.FmtProtoDelim {
    resHeader.Set("Content-Type", `text/plain; version=`+"0.0.4")
} else {
    resHeader.Set("Content-Type", string(contentType))
}

However I'm not sure this is the right approach, is it intended that kube-state-metrics always responds with content headers that match the request? Or was this just a change for Open Metrics? In which case perhaps the header should only get changed if the contentType == expfmt.FmtOpenMetric?

[Joseph-Irving]

I can create a new issue for this if desired btw, sorry for hijacking this old PR

[CatherineF-dev]

Yes, could you create a new issue? Thx!

[Joseph-Irving]

Ok I've opened a new issue #2022