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

feat: Add the http return code to metric api_processed_total #31227

Merged
merged 1 commit into from Apr 2, 2024

Conversation

vipul-21
Copy link
Contributor

@vipul-21 vipul-21 commented Mar 7, 2024

Adding the return code to the cilium_api_limiter_processed_requests_total based on the error/success of the API call. It helps in filtering the metric based on http code as labeloutcome of fail is generic.
Tested the changes via grafana and prometheus:
image

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Mar 7, 2024
@vipul-21
Copy link
Contributor Author

vipul-21 commented Mar 7, 2024

Couple of things I need feedback on:

  1. In case of upgrade of cilium image, the old metrics will not have return_code. So during the transition phase, we will be return_code as empty for old cilium agents. Should we return a default code (unknown) in case of that ? or filter based on the labeloutcome similar to what I added for etcd package.
  2. Error code for failure is 500 in the PR based on labeloutcome. Is that okay ?

@vipul-21 vipul-21 marked this pull request as ready for review March 7, 2024 22:57
@vipul-21 vipul-21 requested review from a team as code owners March 7, 2024 22:57
pkg/rate/api_limiter.go Outdated Show resolved Hide resolved
Copy link
Contributor

@lambdanis lambdanis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One request, can you document the change in the Changed Metrics in Documentation/operations/upgrade.rst?

In case of upgrade of cilium image, the old metrics will not have return_code. So during the transition phase, we will be return_code as empty for old cilium agents. Should we return a default code (unknown) in case of that ?

I'm not sure I understand. You mean to backport the default code to older Cilium versions? I think it's fine if the new agents just start exposing this new label.

@lambdanis lambdanis added dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. release-note/minor This PR changes functionality that users may find relevant to operating Cilium. labels Mar 25, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Mar 25, 2024
@vipul-21 vipul-21 force-pushed the httpcode branch 2 times, most recently from 41bc3ca to 25d6cec Compare March 25, 2024 22:06
Documentation/operations/upgrade.rst Outdated Show resolved Hide resolved
@vipul-21 vipul-21 force-pushed the httpcode branch 2 times, most recently from 6c52d48 to 0bcd25d Compare March 26, 2024 17:23
@vipul-21 vipul-21 requested a review from lambdanis March 26, 2024 17:24
@lambdanis lambdanis removed the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Mar 26, 2024
Copy link
Contributor

@chancez chancez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems reasonable.

Copy link
Contributor

@lambdanis lambdanis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

docs good, thanks!

@vipul-21
Copy link
Contributor Author

/test

Copy link
Contributor

@marseel marseel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable for kvstore.

I think that it was actually a wrong decision for us to reuse api_limiter for kvstore package, but it was the closest thing that we had to implement logic we needed at that time 😿

Signed-off-by: Vipul Singh <vipul21sept@gmail.com>
@julianwiedmann
Copy link
Member

/test

@julianwiedmann julianwiedmann added the area/metrics Impacts statistics / metrics gathering, eg via Prometheus. label Apr 2, 2024
@julianwiedmann julianwiedmann added this pull request to the merge queue Apr 2, 2024
Merged via the queue into cilium:main with commit 76867e2 Apr 2, 2024
62 checks passed
@maintainer-s-little-helper maintainer-s-little-helper bot added ready-to-merge This PR has passed all tests and received consensus from code owners to merge. labels Apr 2, 2024
@vipul-21
Copy link
Contributor Author

vipul-21 commented Apr 2, 2024

@julianwiedmann I want to backport this to cilium version 1.13 and above.
I have a draft PR for v1.13, but can you help adding the backport labels for 1.14 and 1.15 ?
slack conversation: https://cilium.slack.com/archives/C2B917YHE/p1712076138237509

@julianwiedmann
Copy link
Member

As @joestringer noted in the slack thread:

the backporting process and critera are here: https://docs.cilium.io/en/stable/contributing/release/backports/#backport-process . At a glance it looks like this is typically something we wouldn't bother backporting and rather include in a newer release.

@joestringer
Copy link
Member

@tamilmani1989 pointed out that this could be useful for debugging, so in line with the "Debug tool improvements" reason in backporting I think it's reasonable to at least backport to the current stable release.

@joestringer joestringer added the needs-backport/1.15 This PR / issue needs backporting to the v1.15 branch label Apr 3, 2024
@nbusseneau nbusseneau mentioned this pull request Apr 10, 2024
13 tasks
@nbusseneau nbusseneau added backport-pending/1.15 The backport for Cilium 1.15.x for this PR is in progress. and removed needs-backport/1.15 This PR / issue needs backporting to the v1.15 branch labels Apr 10, 2024
@github-actions github-actions bot added backport-done/1.15 The backport for Cilium 1.15.x for this PR is done. and removed backport-pending/1.15 The backport for Cilium 1.15.x for this PR is in progress. labels Apr 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/metrics Impacts statistics / metrics gathering, eg via Prometheus. backport-done/1.15 The backport for Cilium 1.15.x for this PR is done. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants