HDDS-10386. Introduce Metrics for deletekey operation in OM Service.

[muskan1012]

What changes were proposed in this pull request?

In this PR few performance metrics are introduced in OM Service for deleteKey operation.

Please describe your PR in detail:
In this PR following metrics are introduced in OM service for delete key operation:

• deleteKeyFailureLatencyNs
• deleteKeySuccessLatencyNs
• deletekeyResolveBucketlinkLatencyNs
• deleteKeyAclCheckLatencyNs
• deleteKeyAclCheckAndResolveBucketlinkLatencyNs

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-10386

How was this patch tested?

Tested manually on the cluster.

[tanvipenumudy]

Thank you @muskan1012 for working on the patch, please find a few comments.

[kerneltime]

@muskan1012 can you take a look at captureLatencyNs used in #3780 for capturing the latency of operation call. This will help avoid the need to capture start and stop time in each piece of the code you are trying to measure.

[tanvipenumudy]

Thank you @muskan1012 for updating the patch, please find a few more comments.

[tanvipenumudy]

While capturing the latencies of checkKeyACLs and resolveBucketLink individual calls, you can directly wrap the calls under the captureLatencyNs block (as mentioned in the previous reviews).

For latency capture spanning across multiple lines of code (such as success/failure operation latencies), you can either use the same lambda function with the lines of code enclosed under the '{}' of the captureLatencyNs block (following code snippet), or it should also be okay to capture the individual timestamps (you can make a judgement from the readability perspective).

captureLatencyNs(metrics.getMetricLatencyNs(), () -> {
// lines of code to be included whose
// latency needs to be taken into account.
}

[muskan1012]

While capturing the latencies of checkKeyACLs and resolveBucketLink individual calls, you can directly wrap the calls under the captureLatencyNs block (as mentioned in the previous reviews).

For latency capture spanning across multiple lines of code (such as success/failure operation latencies), you can either use the same lambda function with the lines of code enclosed under the '{}' of the captureLatencyNs block (following code snippet), or it should also be okay to capture the individual timestamps (you can make a judgement from the readability perspective).

captureLatencyNs(metrics.getMetricLatencyNs(), () -> {
// lines of code to be included whose
// latency needs to be taken into account.
}

Same thing is happening in both resolveBucketLink and checkAcl, in both case we must need to declare volumeName and bucketName as final to use it with captureLatencyNs.

[tanvipenumudy]

Thank you @muskan1012, LGTM.

@kerneltime, @duongkame could you also please take a look, thanks!

[muskan1012]

@tanvipenumudy, @kerneltime please have a look on this PR.

[tanvipenumudy]

Thank you @muskan1012 for updating the patch iteratively. Please find a few suggestions as well as minor nits, please see if you would like to address the same, otherwise which we may keep the patch simple.

Could you also please rebase the patch on top of the latest master ?

Changes requested

[tanvipenumudy]

How was this patch tested?
Tested manually on the cluster.

@muskan1012, it would help if you could also please share the prometheus/graphana metrics (whichever is convenient) being introduced once the changes are incorporated, thanks.

[tanvipenumudy]

@muskan1012 could you please address the patch build compilation error and checkstyle failures?

[tanvipenumudy]

Please find a couple of small comments, thank you for updating the patch!

[tanvipenumudy]

Tested the changes locally on a docker-compose cluster with the monitoring add-ons.
LGTM +1, pending CI/CD report.

[tanvipenumudy]

Thank you @muskan1012 for the patch, thank you @kerneltime for the review.