📖 document exposed metrics

[djzager]

Update documentation for the metrics package to enumerate all exposed
metrics with an explanation of what they capture.

Fixes #262

[k8s-ci-robot]

Hi @djzager. 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 /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.

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.

[djzager]

I see that the original comment was referring to the pkg/webhook/internal/metrics package but it already documents the exposed metrics.

I could follow the same pattern as pkg/webhook/internal/metrics but that would require exporting a bunch of stuff that is currently not exported.

[vincepri]

/ok-to-test
/assign @DirectXMan12
/lgtm

[djzager]

Looking around the project at the other metrics packages (ie. controller metrics and webhook metrics) I feel that I took the wrong approach in this PR.

What I would like to do would be to export the variables in this package where metrics are defined (making them var where they are now hidden inside a function) and add the appropriate comments.

@DirectXMan12 @vincepri WDYT?

[vincepri]

That sounds good to me as well

[djzager]

went ahead and followed what was being done in kubernetes metrics

[djzager]

Went ahead and left it as 2 commits to make it easier to go back to the original if necessary. If this works I'll rebase and squash.

[DirectXMan12]

Hey, so I'm sorry to jump in a bit later here, but:

What I would like to do would be to export the variables in this package where metrics are defined (making them var where they are now hidden inside a function) and add the appropriate comments.

Due to the way this is currently structured, it means we have out metrics compatibility boundary not at the metrics we expose, but via the exact method we expose them by. For instance, under this model, switching to OpenTelemetry is necessarily a breaking change, even if we expose the same metrics in the same format with the same names, since the Go types change. Considering that the Go types are mostly an implementation detail, that seems suboptimal.

Should be easy to fix by moving the actual variables into an internal package, like the other referenced examples.

EDIT: looked a the linked examples, clarified what I said a bit.

[djzager]

Due to the way this is currently structured, it means we have out metrics compatibility boundary not at the metrics we expose, but via the exact method we expose them by. For instance, under this model, switching to OpenTelemetry is necessarily a breaking change, even if we expose the same metrics in the same format with the same names, since the Go types change. Considering that the Go types are mostly an implementation detail, that seems suboptimal.

My idea of making unexported vars exported made me uneasy, but I didn't really understand why. Thank you for the explanation.

I have updated the metrics package to expose the subsystems and specific keys used without exporting the metrics themselves.

I would be tempted to make similar changes in https://github.com/kubernetes-sigs/controller-runtime/blob/master/pkg/webhook/internal/metrics/metrics.go and https://github.com/kubernetes-sigs/controller-runtime/blob/master/pkg/internal/controller/metrics/metrics.go but I suspect making exported things unexported would also be a breaking change. It's also possible that pkg/internal/controller/metrics and pkg/webhook/internal/metrics are fundamentally different than pkg/metrics but I'm not certain. Hoping to gain better understanding of what changes make sense and when.

[DirectXMan12]

It's also possible that pkg/internal/controller/metrics and pkg/webhook/internal/metrics are fundamentally different than pkg/metrics but I'm not certain

Ah, yes, your intuition is correct here :-). In Go, and package with /internal/ in the path as visibility limited to the parent package of /internal (as well as any subpackages of the parent package). So anything in /pkg/webhook/internal/... can only be accessed by things in /pkg/webhook/.... This is enforced by the compiler's import logic: https://golang.org/doc/go1.4#internalpackages

[DirectXMan12]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: DirectXMan12, djzager

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 [DirectXMan12]

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

[shawn-hurley]

Should we cancel the hold on this?

[vincepri]

/hold cancel