Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: fallback to gauge for protofmt-based negotiations #2270

Merged
merged 10 commits into from
Mar 27, 2024

Conversation

rexagod
Copy link
Member

@rexagod rexagod commented Dec 13, 2023

What this PR does / why we need it: Fallback to gauge metric type when the negotiated content-type is protofmt-based, since Prometheus' protobuf machinery does not recognize all OpenMetrics types (info and statesets in this context).

How does this change affect the cardinality of KSM: 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 #2248

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Dec 13, 2023
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 13, 2023
@rexagod
Copy link
Member Author

rexagod commented Dec 13, 2023

Need to test this.

@rexagod
Copy link
Member Author

rexagod commented Dec 13, 2023

Verified using the following configurations. Error logs were reported without this patch, but none when this was applied.

Details

Prometheus error log

ts=2023-12-13T16:01:02.547Z caller=scrape.go:1418 level=debug component="scrape manager" scrape_pool=kube-state-metrics target=http://127.0.0.1:8080/metrics msg="Append failed" err="invalid metric type \"info\""

KSM CRS Configuration

	  ...
      metrics:
        - name: "foo_info"
          help: "foo_help"
          each:
            type: Info
            info:
              path: [metadata]
              labelsFromPath:
                "name": [name]

Prometheus Configuration

global:
  evaluation_interval: 15s
  scrape_interval:     15s
  scrape_protocols: [PrometheusProto]
scrape_configs:
  - job_name: 'kube-state-metrics'
    static_configs:
      - targets: ['127.0.0.1:8080']

Generated CRS Metric (from Prometheus UI)

kube_customresource_foo_info{customresource_group="contoso.com", customresource_kind="MyPlatform", customresource_version="v1alpha1", instance="127.0.0.1:8080", job="kube-state-metrics", name="test-dotnet-app"} 1

@rexagod rexagod marked this pull request as ready for review December 13, 2023 16:10
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 13, 2023
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Dec 13, 2023
@rexagod rexagod changed the title fallback to gauge for protofmt-based negotiations fix: fallback to gauge for protofmt-based negotiations Dec 13, 2023
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 14, 2023
@logicalhan
Copy link
Member

/triage accepted
/assign @dgrisonnet

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Dec 14, 2023
@rexagod
Copy link
Member Author

rexagod commented Feb 20, 2024

@dgrisonnet Yes, I believe we talked about dropping metric-standard lock-ins, and implementing a one-fits-all mechanism (probably on top of gauges) that allows us to generate an exposition format that is ingestible by most metric backends, if not all.

@rexagod
Copy link
Member Author

rexagod commented Feb 20, 2024

(friendly ping @mrueg @dgrisonnet to /lgtm)

@rexagod rexagod mentioned this pull request Mar 12, 2024
@remram44
Copy link

2.11.0 introduced a regression, as metrics include stateset entries which are refused by Prometheus. This worked correctly on 2.10 so I'm stuck until this MR is merged.

rexagod added 2 commits March 26, 2024 23:47
… to `gauge` for `protofmt`-based negotiations
…allback to `gauge` for `protofmt`-based negotiations
@dgrisonnet
Copy link
Member

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 27, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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,rexagod]

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

@dgrisonnet
Copy link
Member

/unhold

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 27, 2024
@k8s-ci-robot k8s-ci-robot merged commit fa5eb67 into kubernetes:main Mar 27, 2024
12 checks passed
@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Mar 27, 2024
@Efrat19
Copy link

Efrat19 commented Mar 28, 2024

Hey @rexagod ! is this also meant to fix #1973 replicating in 2.110??

@rexagod
Copy link
Member Author

rexagod commented Mar 28, 2024

It should, yes.

@LaikaN57
Copy link
Contributor

Hi folks, is this change targeted to be in the release-2.11 cut (which has the k8s 1.28 support of the client libs)?

@dgrisonnet Looks like this fix got merged in too late for v2.11.0. How do we feel about cutting a new release soon™ to get people unblocked? (Thanks in advance.)

@LaikaN57
Copy link
Contributor

LaikaN57 commented Apr 2, 2024

Hi folks, is this change targeted to be in the release-2.11 cut (which has the k8s 1.28 support of the client libs)?

@dgrisonnet Looks like this fix got merged in too late for v2.11.0. How do we feel about cutting a new release soon™ to get people unblocked? (Thanks in advance.)

@rexagod bump ^

@rexagod
Copy link
Member Author

rexagod commented Apr 2, 2024

I'll put #2335 up for review today.

@rexagod
Copy link
Member Author

rexagod commented Apr 2, 2024

Sharing here for visibility: #2335 (comment).

@seruman
Copy link

seruman commented Dec 19, 2024

hi @rexagod,

i'm probably missing something but should not it also fallback to gauge on text/plain?

with --enable-feature=native-histograms prometheus (2.52.0) sends;

Accept: application/vnd.google.protobuf;proto=io.prometheus.client.MetricFamily;encoding=delimited;q=0.5,application/openmetrics-text;version=1.0.0;q=0.4,application/openmetrics-text;version=0.0.1;q=0.3,text/plain;version=0.0.4;q=0.2,*/*;q=0.1

but handler falls-back to text/plain here;

contentType := expfmt.NegotiateIncludingOpenMetrics(r.Header)
// We do not support protobuf at the moment. Fall back to FmtText if the negotiated exposition format is not FmtOpenMetrics See: https://github.com/kubernetes/kube-state-metrics/issues/2022.
if contentType.FormatType() != expfmt.TypeOpenMetrics {
contentType = expfmt.NewFormat(expfmt.TypeTextPlain)
}

thus exporter responds with type info that makes prometheus unhappy ; invalid metric type "info".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Info and StateSet types cause errors when scrapped by Prometheus