docs: correct metric names in topics/metrics.md to match exported Prometheus names#2038
Conversation
|
Welcome @mlushpenko! |
|
Hi @mlushpenko. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
|
/ok-to-test |
aramase
left a comment
There was a problem hiding this comment.
Thanks for the PR!
/lgtm
/approve
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2038 +/- ##
=======================================
Coverage 22.07% 22.07%
=======================================
Files 57 57
Lines 3198 3198
=======================================
Hits 706 706
Misses 2400 2400
Partials 92 92 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
The OpenTelemetry Prometheus exporter appends `_total` to counter instruments (see open-telemetry/opentelemetry-go#3360), so the metrics exposed at /metrics do not match the names listed in this document. - Align the metric names in the table and sample output with the names the driver actually registers in pkg/secrets-store/stats_reporter.go. - Add the provider tag to rotation_reconcile_total and rotation_reconcile_error_total, which the reporter already sets. - Fix the k8s secret sync histogram name (k8s_secret_duration_sec). - Note that metrics only appear on driver pods that have served a mount/unmount/rotation, which can be confusing when port-forwarding. Refs kubernetes-sigs#1937 Signed-off-by: Maksym Lushpenko <iviakciivi@gmail.com>
3cd94b8 to
c379326
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aramase, mlushpenko The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
Fixes #1937
What's wrong today
docs/book/src/topics/metrics.md(rendered at https://secrets-store-csi-driver.sigs.k8s.io/topics/metrics) lists metric names that do not match what the driver actually exports on/metrics. Following open-telemetry/opentelemetry-go#3360, the OpenTelemetry Prometheus exporter appends_totalto counter instruments, so the counter names on the wire end with_totalrather than beginning withtotal_.The e2e test at
test/bats/e2e-provider.batsalready asserts the real names:…which contradicts the docs.
What this PR changes
Docs-only. Updates
docs/book/src/topics/metrics.md:Counter names — match what the Prometheus exporter emits:
total_node_publishnode_publish_totaltotal_node_unpublishnode_unpublish_totaltotal_node_publish_errornode_publish_error_totaltotal_node_unpublish_errornode_unpublish_error_totaltotal_sync_k8s_secretsync_k8s_secret_totaltotal_rotation_reconcilerotation_reconcile_totaltotal_rotation_reconcile_errorrotation_reconcile_error_totalHistogram name — fixed typo; the instrument is registered as
k8s_secret_duration_secinpkg/secrets-store/stats_reporter.go, notsync_k8s_secret_duration_sec.Tags on
rotation_reconcile_total/rotation_reconcile_error_total— added theprovidertag, which the reporter already sets atstats_reporter.go:146and:155but which was missing from the docs.Sample output block — regenerated with the current names.
Two clarifications added:
_totalsuffix behavior (so future readers understand why the exported names differ from the instrument names registered in code).No code changes. Metric names on the wire are unchanged.
Which issue this PR fixes
Fixes #1937
Special notes for your reviewer
None.
Does this PR introduce a user-facing change?