metrics: add utility to record metrics for sidecar

[Nikhil-Ladha]

Enable prometheus metrics for the sidecar using the csi-lib-utils metrics package. The httpServer for metrics can be enable by adding the httpEndpoint arg to the sidecar. The server will record metrics for operations like GetMetadataAllocated, GetMetadataDelta in the format
snapshot_metadata_controller_operation_total_seconds_bucket{driver_name="",operation_name="",operation_status="",target_snapshot="", base_snapshot="",le="0.1"}, for GetMetaAllocated operations "base_snapshot"
value will be empty.

What type of PR is this?
/kind feature

What this PR does / why we need it:
This PR enables recording prometheus metrics for the snapshot-metadata sidecar.

Which issue(s) this PR fixes:
Fixes #11

Does this PR introduce a user-facing change?:

Enable prometheus metrics for the snapshot-metatadata sidecar

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: Nikhil-Ladha / name: Nikhil Ladha (8331a93)

[k8s-ci-robot]

Welcome @Nikhil-Ladha!

It looks like this is your first PR to kubernetes-csi/external-snapshot-metadata 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-csi/external-snapshot-metadata has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

Hi @Nikhil-Ladha. Thanks for your PR.

I'm waiting for a kubernetes-csi 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-sigs/prow repository.

[Nikhil-Ladha]

/cc @Rakshith-R @iPraveenParihar

[k8s-ci-robot]

@Nikhil-Ladha: GitHub didn't allow me to request PR reviews from the following users: Rakshith-R, iPraveenParihar.

Note that only kubernetes-csi members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @Rakshith-R @iPraveenParihar

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.

[Nikhil-Ladha]

Since, there are no metrics being emitted as of now from the sidecar. This is the only metrics being logged in the server.

$ curl --http0.9 -X GET http://10.244.0.18:8080/metrics --output -
# HELP process_start_time_seconds [ALPHA] Start time of the process since unix epoch in seconds.
# TYPE process_start_time_seconds gauge
process_start_time_seconds 1.73339252136e+09
# HELP snapshot_metadata_controller_operations_in_flight [ALPHA] Total number of operations in flight
# TYPE snapshot_metadata_controller_operations_in_flight gauge
snapshot_metadata_controller_operations_in_flight 0

P.S: I have added unit tests to support working of other metrics utility functions. Hope, that should suffice for now.

[carlbraganza]

Why don't you propose how basic timing of GetMetadataAllocated and GetMetadataDelta would be measured?

[carlbraganza]

@Nikhil-Ladha I have a high level question: What was wrong with the original use of the github.com/kubernetes-csi/csi-lib-utils/metrics package? I'm sorry for being so slow in noticing this!

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Nikhil-Ladha
Once this PR has been reviewed and has the lgtm label, please assign carlbraganza for approval. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

• OWNERS

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

[Nikhil-Ladha]

@Nikhil-Ladha I have a high level question: What was wrong with the original use of the github.com/kubernetes-csi/csi-lib-utils/metrics package? I'm sorry for being so slow in noticing this!

@carlbraganza updated the PR based on our discussion offline, please take a look now.

[carlbraganza]

This will not record the time taken for s.streamGetMetadataAllocatedResponse(ctx, stream, csiStream)! Also, this logic doesn't record earlier failures in the subroutine. The same is true for the Delta case.

I suggest the following changes to handle all cases with just one call to your record function:

1. Change the method signature to make err a named return variable. e.g.

func (s *Server) GetMetadataAllocated(req *api.GetMetadataAllocatedRequest, stream api.SnapshotMetadata_GetMetadataAllocatedServer) (err error) {

2. Remove err from all auto-definitions - you will have to declare some variables explicitly where multiple auto-definitions occur. e.g.

     var (
        csiReq *csi.GetMetadataAllocatedRequest
        csiStream csi.SnapshotMetadata_GetMetadataAllocatedClient
     )

3. Make sure that the last statement assigns to err:

err = s.streamGetMetadataAllocatedResponse(ctx, stream, csiStream)

3. At the top of the function you can define something like this:

        defer func(startTime time.Time) {
	    opLabel := map[string]string{
		runtime.LabelTargetSnapshotName: fmt.Sprintf("%s/%s", req.Namespace, req.SnapshotName),
	     }
             s.config.Runtime.RecordMetricsWithLabels(opLabel, runtime.MetadataAllocatedOperationName, startTime, err)
        }(time.Now())

[Nikhil-Ladha]

I didn't track the failures in other subroutines purposefully, as I thought if the operation hasn't even reached the CSI driver then what's the use of tracking the metrics for it? Also, regarding the stream response, I wasn't sure if we should be recording that.
Anyway, this seems fine as well and I will update the code accordingly.

Though one question, why do I need to do this-

Remove err from all auto-definitions - you will have to declare some variables explicitly where multiple auto-definitions occur. e.g.

The auto-definitions use the same named err error variable so we should be able to track the err value while recording the metrics in the end. Can you please help me understand what difference does that make?

[carlbraganza]

Hmm... you may be right in that it won't get redefined - I don't know the latest Go semantics but some experiments in the playground shows that it is much more restrictive than before. Please check! Also, the UTs should help in ensuring existing behavior continues.

[Nikhil-Ladha]

Updated the code accordingly, please take a look.
Also, add UT for the metrics wherever necessary.

[Rakshith-R]

/ok-to-test