----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/74396/#review225423 -----------------------------------------------------------
kms/src/main/java/org/apache/ranger/kms/metrics/KMSMetricWrapper.java Lines 57 (patched) <https://reviews.apache.org/r/74396/#comment314042> A good practice is to assign the static member to a local variable and use that variable: KMSMetricWrapper wrapper = KMSMetricWrapper.instance; if (wrapper == null) { synchronized (KMSMetricWrapper.class) { wrapper = KMSMetricWrapper.instance; if (wrapper == null) { wrapper = KMSMetricWrapper.instance = new KMSMetricWrapper(isMetricCollectionThreadSafe); } } return wrapper; kms/src/main/java/org/apache/ranger/kms/metrics/KMSMetrics.java Lines 34 (patched) <https://reviews.apache.org/r/74396/#comment314040> For metrics about API calls, it will help to prefix with "API", like: API_KEY_CREATE_COUNT API_KEY_CREATE_ELAPSED_TIME API_EEK_GENERATE_COUNT API_EEK_GENERATE_ELAPSED_TIME kms/src/main/java/org/apache/ranger/kms/metrics/collector/KMSMetricsCollector.java Lines 68 (patched) <https://reviews.apache.org/r/74396/#comment314041> Consider adding a class to track API usage to make it easier to instrument. public class KMSMetricsCollector { public APIMetric createAPIMetric(KMSMetric counter, KMSMetric elapsedTime) { return new APIMetric(counter, elapsedTime); } public class APIMetric implements AutoCloseable { private final KMSMetric counter; private final KMSMetric elapsedTime; private final Stopwatch sw; private APIMetric(KMSMetric counter, KMSMetric elapsedTime) { this.counter = counter; this.elapsedTime = elapsedTime; this.sw = Stopwatch.createStarted(); } @Override public void close() { incrementCounter(counter); updateMetric(elapsedTime, sw.stop().elapsed(TimeUnit.MILLISECONDS)); } } } With above in place, instrumentation can be easier like: try (APIMetric apiMetric = kmsMetricsCollector.createAPIMetric(KMSMetric.KEY_CREATE_COUNT, KMSMetric.KEY_CREATE_ELAPSED_TIME)) { ... // API implementation } - Madhan Neethiraj On April 27, 2023, 2:11 p.m., Vikas Kumar wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/74396/ > ----------------------------------------------------------- > > (Updated April 27, 2023, 2:11 p.m.) > > > Review request for ranger, Kishore Gopalakrishna, Madhan Neethiraj, Ramesh > Mani, and Sailaja Polavarapu. > > > Bugs: RANGER-4047 > https://issues.apache.org/jira/browse/RANGER-4047 > > > Repository: ranger > > > Description > ------- > > RANGER-4047-Ranger KMS health metrics. > > The solution depends on "ranger-metrics" common module for both Json and > Prometheus sink. > > Kms has added only KMS specific application metrics. Generally, one COUNT > metric and corresponding elapsed time gauge metric for each REST end points. > > By default, metric collection is not thread-safe but the by adding following > property in kms-site.xml it can be made thread-safe: > > Prop name: hadoop.kms.metric.collection.threadsafe > > > Diffs > ----- > > distro/src/main/assembly/kms.xml 4b4a2ac8e > kms/pom.xml e97b993d7 > kms/src/main/java/org/apache/hadoop/crypto/key/kms/server/KMS.java > 12bcdc1aa > > kms/src/main/java/org/apache/hadoop/crypto/key/kms/server/KMSAuthenticationFilter.java > 274bac910 > kms/src/main/java/org/apache/hadoop/crypto/key/kms/server/KMSWebApp.java > a32444cd1 > kms/src/main/java/org/apache/hadoop/crypto/key/kms/server/MetricREST.java > PRE-CREATION > kms/src/main/java/org/apache/ranger/kms/metrics/KMSMetricWrapper.java > PRE-CREATION > kms/src/main/java/org/apache/ranger/kms/metrics/KMSMetrics.java > PRE-CREATION > > kms/src/main/java/org/apache/ranger/kms/metrics/collector/KMSMetricsCollector.java > PRE-CREATION > kms/src/main/java/org/apache/ranger/kms/metrics/source/KMSMetricSource.java > PRE-CREATION > kms/src/main/resources/hadoop-metrics2.properties PRE-CREATION > kms/src/test/java/org/apache/ranger/kms/metrics/TestKMSMetricsWrapper.java > PRE-CREATION > kms/src/test/resources/hadoop-metrics2.properties PRE-CREATION > > > Diff: https://reviews.apache.org/r/74396/diff/2/ > > > Testing > ------- > > Build is successful but end-to-end functional testing is under-progress. > > Reviewers may start code review. > > ===========Update======== > mvn clean compile package install is working > > installed Admin and kms on Ubuntu and I was able to create/list keys . Also I > was able to get response for > > curl -ivk -H "Content-Type: application/json" -H -X GET > http://localhost:9292/kms/metrics/json?user.name=vikas > > sample response: > > { > "KMS" : { > "GET_CURRENT_KEY_COUNT" : 0, > "DELETE_KEY_ELAPSED_TIME" : 0, > "EEK_DECRYPT_ELAPSED_TIME" : 0, > "GET_KEYS_METADATA_ELAPSED_TIME" : 0, > "EEK_GENERATE_ELAPSED_TIME" : 0, > "GET_CURRENT_KEY_ELAPSED_TIME" : 0, > "EEK_REENCRYPT_ELAPSED_TIME" : 0, > "KEY_CREATE_COUNT" : 1, > "UNAUTHORIZED_CALLS_COUNT" : 0, > "KEY_CREATE_ELAPSED_TIME" : 81, > "GET_KEY_VERSION_COUNT" : 0, > "ROLL_NEW_VERSION_ELAPSED_TIME" : 0, > "REENCRYPT_EEK_BATCH_COUNT" : 0, > "REENCRYPT_EEK_BATCH_ELAPSED_TIME" : 0, > "GET_KEYS_METADATA_COUNT" : 0, > "GET_KEY_VERSIONS_COUNT" : 0, > "GET_KEY_VERSIONS_ELAPSED_TIME" : 0, > "GET_KEYS_COUNT" : 2, > "EEK_GENERATE_COUNT" : 0, > "INVALIDATE_CACHE_COUNT" : 0, > "GET_METADATA_COUNT" : 3, > "REENCRYPT_EEK_BATCH_KEYS_COUNT" : 0, > "EEK_REENCRYPT_COUNT" : 0, > "UNAUTHENTICATED_CALLS_COUNT" : 0, > "GET_KEY_VERSION_ELAPSED_TIME" : 0, > "INVALIDATE_CACHE_ELAPSED_TIME" : 0, > "ROLL_NEW_VERSION_COUNT" : 0, > "EEK_DECRYPT_COUNT" : 0, > "GET_KEYS_METADATA_KEYNAMES_COUNT" : 0, > "DELETE_KEY_COUNT" : 0, > "GET_KEYS_ELAPSED_TIME" : 72, > "GET_METADATA_ELAPSED_TIME" : 14, > "TOTAL_CALL_COUNT" : 7 > }, > "RangerJvm" : { > "GcTimeTotal" : 339, > "SystemLoadAvg" : 1.47, > "ThreadsBusy" : 5, > "GcCountTotal" : 9, > "MemoryMax" : 1005584384, > "MemoryCurrent" : 221646760, > "ThreadsWaiting" : 20, > "ProcessorsAvailable" : 2, > "GcTimeMax" : 339, > "ThreadsBlocked" : 0, > "ThreadsRemaining" : 9 > } > > > Thanks, > > Vikas Kumar > >
