Skip to content

Add shard label to proxy_request_duration_seconds - #4516

Merged
kcp-ci-bot merged 1 commit into
kcp-dev:mainfrom
AuricX:chore/4502-proxy-metrics-shard-url
Nov 9, 2026
Merged

kcp-ci-bot merged 1 commit into
kcp-dev:mainfrom
AuricX:chore/4502-proxy-metrics-shard-url

Conversation

@AuricX

@AuricX AuricX commented Oct 29, 2026

Copy link
Copy Markdown
Contributor

Summary

Adds a shard label to proxy_request_duration_seconds, so front-proxy latency can be split per backend shard, and removes the TODO(csams) in pkg/proxy/metrics/metrics.go.

The shard is resolved inside the latency middleware (by lookup.WithClusterResolver, on a derived context), so WithLatencyTracking can't read it from its own request. It now uses the same ShardNameHolder pattern as WithBodyTracking: it stores a holder in the request context before calling the next handler, and promhttp.WithLabelFromCtx reads the name from it after the handler returns. Requests the front-proxy serves itself (/metrics, /readyz) get shard="".

WithShardNameHolder now returns the existing holder when the context already has one. Otherwise the holder created by WithBodyTracking would shadow the latency one and the latency metric would never see the shard name. A small ShardNameHolderFrom getter is added so the metrics package can read the holder.

The TODO mentions the shard URL, but the label uses the shard name to match kcp_backend_rq_body_bytes, kcp_backend_rs_body_bytes and kcp_listener_tls_connection_error. Cardinality grows by the number of shards. The metric is ALPHA, and existing queries that aggregate by (code, method) keep working.

TestWithLatencyTracking wraps a handler in the real middleware order (WithLatencyTracking(WithBodyTracking(...))) and checks one observation lands on shard="shard-1", or on shard="" when no shard is set. It fails without this change because the metric has no shard label, and it also fails if the holder isn't reused.

What Type of PR Is This?

/kind feature

Related Issue(s)

Part of #4502

Release Notes

The front-proxy `proxy_request_duration_seconds` metric now has a `shard` label.

@kcp-ci-bot kcp-ci-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has signed the DCO. kind/feature Categorizes issue or PR as related to a new feature. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 29, 2026
@kcp-ci-bot

Copy link
Copy Markdown
Contributor

Hi @AuricX. Thanks for your PR.

I'm waiting for a kcp-dev member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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-sigs/prow repository.

@ntnn ntnn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

/lgtm
/approve
/ok-to-test

Thank you!

@kcp-ci-bot kcp-ci-bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. lgtm Indicates that a PR is ready to be merged. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 30, 2026
@kcp-ci-bot

Copy link
Copy Markdown
Contributor

LGTM label has been added.

DetailsGit tree hash: ad2ae193dd616a696b9df8661ca0fff585dc7232

@kcp-ci-bot kcp-ci-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 30, 2026
@AuricX

AuricX commented Oct 30, 2026

Copy link
Copy Markdown
Contributor Author

/retest

1 similar comment
@mjudeikis

Copy link
Copy Markdown
Contributor

/retest

@kcp-ci-bot kcp-ci-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 3, 2026
@mjudeikis

Copy link
Copy Markdown
Contributor

This needs rebase now. Please rebase and convert to read. Moving to draft in the meantime

@mjudeikis
mjudeikis marked this pull request as draft November 3, 2026 11:23
@kcp-ci-bot kcp-ci-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 3, 2026
Signed-off-by: AuricX <auricx.business@gmail.com>
@AuricX
AuricX force-pushed the chore/4502-proxy-metrics-shard-url branch from 91f035d to c7659c0 Compare November 8, 2026 14:46
@kcp-ci-bot kcp-ci-bot removed the lgtm Indicates that a PR is ready to be merged. label Nov 8, 2026
@kcp-ci-bot
kcp-ci-bot requested a review from ntnn November 8, 2026 14:46
@kcp-ci-bot kcp-ci-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 8, 2026
@AuricX

AuricX commented Nov 8, 2026

Copy link
Copy Markdown
Contributor Author

@mjudeikis I rebased onto main, the only conflict came from pkg/proxy/lookup being merged into pkg/server/proxy so ShardNameHolderFrom lives there now and the metrics package imports it from there, tests and lint pass locally. @ntnn the push dropped your lgtm, please take another look when you have some time.

@AuricX
AuricX marked this pull request as ready for review November 8, 2026 14:47
@kcp-ci-bot kcp-ci-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 8, 2026
@mjudeikis

Copy link
Copy Markdown
Contributor

/lgtm
/approve

@kcp-ci-bot kcp-ci-bot added the lgtm Indicates that a PR is ready to be merged. label Nov 9, 2026
@kcp-ci-bot

Copy link
Copy Markdown
Contributor

LGTM label has been added.

DetailsGit tree hash: df5575b8ba9ddf326b97565db22a28b5ffe11d6f

@kcp-ci-bot

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mjudeikis, ntnn

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

The pull request process is described here

Details Needs approval from an approver in each of these files:

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

@kcp-ci-bot
kcp-ci-bot merged commit 8e237f1 into kcp-dev:main Nov 9, 2026
19 checks passed
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. dco-signoff: yes Indicates the PR's author has signed the DCO. kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants