Metric tracking fresh ts shortfall

atlasdb-config/src/main/java/com/palantir/atlasdb/factory/DefaultLockAndTimestampServiceFactory.java

@@ -61,6 +61,7 @@
 import com.palantir.lock.client.RequestBatchersFactory;
 import com.palantir.lock.client.TimeLockClient;
 import com.palantir.lock.client.TimestampCorroboratingTimelockService;
+import com.palantir.lock.client.TimestampLeaseMetrics;
 import com.palantir.lock.client.metrics.TimeLockFeedbackBackgroundTask;
 import com.palantir.lock.client.timestampleases.MinLeasedTimestampGetter;
 import com.palantir.lock.client.timestampleases.MinLeasedTimestampGetterImpl;
@@ -369,7 +370,8 @@ private static LockAndTimestampServices getLockAndTimestampServices(
                 namespacedConjureTimelockService,
                 multiClientTimelockServiceSupplier,
                 namespacedTimelockRpcClient,
-                timeLockHelperServices.requestBatchersFactory());
+                timeLockHelperServices.requestBatchersFactory(),
+                metricsManager);
 
         TimestampManagementService timestampManagementService = new RemoteTimestampManagementAdapter(
                 serviceProvider.getTimestampManagementRpcClient(), timelockNamespace);
@@ -391,7 +393,8 @@ private static RemoteTimelockServiceAdapter createRemoteTimelockServiceAdapter(
             NamespacedConjureTimelockService namespacedConjureTimelockService,
             Supplier<InternalMultiClientConjureTimelockService> multiClientTimelockServiceSupplier,
             NamespacedTimelockRpcClient namespacedTimelockRpcClient,
-            RequestBatchersFactory batchersFactory) {
+            RequestBatchersFactory batchersFactory,
+            MetricsManager metricsManager) {
         LockTokenUnlocker unlocker = getTimeLockUnlocker(
                 timelockNamespace,
                 timelockRequestBatcherProviders,
@@ -401,8 +404,8 @@ private static RemoteTimelockServiceAdapter createRemoteTimelockServiceAdapter(
         NamespacedTimestampLeaseService timestampLeaseService = new NamespacedTimestampLeaseServiceImpl(
                 Namespace.of(timelockNamespace), multiClientTimelockServiceSupplier.get());
 
-        TimestampLeaseAcquirer timestampLeaseAcquirer =
-                TimestampLeaseAcquirerImpl.create(timestampLeaseService, unlocker);
+        TimestampLeaseAcquirer timestampLeaseAcquirer = TimestampLeaseAcquirerImpl.create(
+                timestampLeaseService, unlocker, TimestampLeaseMetrics.of(metricsManager.getTaggedRegistry()));
 
         MinLeasedTimestampGetter minLeasedTimestampGetter = new MinLeasedTimestampGetterImpl(timestampLeaseService);
 

atlasdb-tests-shared/src/main/java/com/palantir/timelock/paxos/AbstractInMemoryTimelockExtension.java

@@ -43,6 +43,7 @@
 import com.palantir.lock.client.NamespacedConjureTimelockServiceImpl;
 import com.palantir.lock.client.RemoteTimelockServiceAdapter;
 import com.palantir.lock.client.RequestBatchersFactory;
+import com.palantir.lock.client.TimestampLeaseMetrics;
 import com.palantir.lock.client.TransactionStarter;
 import com.palantir.lock.client.timestampleases.MinLeasedTimestampGetterImpl;
 import com.palantir.lock.client.timestampleases.NamespacedTimestampLeaseService;
@@ -90,6 +91,7 @@ public abstract class AbstractInMemoryTimelockExtension implements TimeLockServi
     private NamespacedConjureTimelockServiceImpl namespacedConjureTimelockService;
     private NamespacedTimestampLeaseService timestampLeaseService;
     private LockTokenUnlocker unlocker;
+    private final MetricsManager metricsManager = MetricsManagers.createForTests();
 
     public AbstractInMemoryTimelockExtension() {
         this("client");
@@ -129,8 +131,6 @@ public void setup() {
                 .clusterSnapshot(clusterConfig)
                 .build();
 
-        MetricsManager metricsManager = MetricsManagers.createForTests();
-
         timeLockAgent = TimeLockAgent.create(
                 metricsManager,
                 install,
@@ -146,7 +146,7 @@ public void setup() {
                 () -> System.exit(0));
 
         delegate = timeLockAgent.createInvalidatingTimeLockServices(client);
-        createHelperServices(metricsManager);
+        createHelperServices();
 
         // Wait for leadership
         Awaitility.await()
@@ -156,7 +156,7 @@ public void setup() {
                 .until(() -> delegate.getTimestampService().getFreshTimestamp() > 0);
     }
 
-    private void createHelperServices(MetricsManager metricsManager) {
+    private void createHelperServices() {
         helperServices = TimeLockHelperServices.create(
                 client,
                 metricsManager,
@@ -248,7 +248,8 @@ public TimelockService getLegacyTimelockService() {
                 lockLeaseService,
                 transactionStarter,
                 commitTimestampGetter,
-                TimestampLeaseAcquirerImpl.create(timestampLeaseService, unlocker),
+                TimestampLeaseAcquirerImpl.create(
+                        timestampLeaseService, unlocker, TimestampLeaseMetrics.of(metricsManager.getTaggedRegistry())),
                 new MinLeasedTimestampGetterImpl(timestampLeaseService));
     }
 

lock-api/build.gradle

@@ -5,6 +5,7 @@ apply plugin: 'com.palantir.metric-schema'
 license {
     exclude '**/TimestampCorrectnessMetrics.java'
     exclude '**/TopologyMetrics.java'
+    exclude '**/TimestampLeaseMetrics.java'
 }
 
 libsDirName = file('build/artifacts')

lock-api/src/main/java/com/palantir/lock/client/timestampleases/TimestampLeaseAcquirerImpl.java

@@ -16,6 +16,7 @@
 
 package com.palantir.lock.client.timestampleases;
 
+import com.codahale.metrics.Counter;
 import com.github.rholder.retry.Attempt;
 import com.github.rholder.retry.RetryException;
 import com.github.rholder.retry.Retryer;
@@ -39,6 +40,7 @@
 import com.palantir.lock.LimitingLongSupplier;
 import com.palantir.lock.client.LeasedLockToken;
 import com.palantir.lock.client.LockTokenUnlocker;
+import com.palantir.lock.client.TimestampLeaseMetrics;
 import com.palantir.lock.v2.TimestampLeaseResult;
 import com.palantir.lock.v2.TimestampLeaseResults;
 import com.palantir.logsafe.Preconditions;
@@ -62,28 +64,38 @@ public final class TimestampLeaseAcquirerImpl implements TimestampLeaseAcquirer
     private final NamespacedTimestampLeaseService delegate;
     private final Unlocker unlocker;
     private final Supplier<UUID> uuidSupplier;
+    private final Counter notEnoughFreshTimestampsCounter;
 
     private final Retryer<Optional<TimestampLeaseResponses>> retryer =
             RetryerBuilder.<Optional<TimestampLeaseResponses>>newBuilder()
                     .retryIfResult(Optional::isEmpty)
                     .withStopStrategy(StopStrategies.stopAfterAttempt(3))
                     .build();
 
-    @VisibleForTesting
-    TimestampLeaseAcquirerImpl(
-            NamespacedTimestampLeaseService delegate, Unlocker unlocker, Supplier<UUID> uuidSupplier) {
+    private TimestampLeaseAcquirerImpl(
+            NamespacedTimestampLeaseService delegate,
+            Unlocker unlocker,
+            Supplier<UUID> uuidSupplier,
+            Counter notEnoughFreshTimestampsCounter) {
         this.delegate = delegate;
         this.unlocker = unlocker;
         this.uuidSupplier = uuidSupplier;
+        this.notEnoughFreshTimestampsCounter = notEnoughFreshTimestampsCounter;
     }
 
-    private TimestampLeaseAcquirerImpl(NamespacedTimestampLeaseService delegate, Unlocker unlocker) {
-        this(delegate, unlocker, UniqueIds::pseudoRandomUuidV4);
+    @VisibleForTesting
+    TimestampLeaseAcquirerImpl(
+            NamespacedTimestampLeaseService delegate, Unlocker unlocker, Supplier<UUID> uuidSupplier) {
+        this(delegate, unlocker, uuidSupplier, new Counter());
     }
 
     public static TimestampLeaseAcquirer create(
-            NamespacedTimestampLeaseService timestampLeaseService, LockTokenUnlocker unlocker) {
-        return new TimestampLeaseAcquirerImpl(timestampLeaseService, identifier -> unlock(unlocker, identifier));
+            NamespacedTimestampLeaseService delegate, LockTokenUnlocker unlocker, TimestampLeaseMetrics metrics) {
+        return new TimestampLeaseAcquirerImpl(
+                delegate,
+                identifier -> unlock(unlocker, identifier),
+                UniqueIds::pseudoRandomUuidV4,
+                metrics.notEnoughFreshTimestamps());
     }
 
     @Override
@@ -146,23 +158,27 @@ private Optional<TimestampLeaseResponses> acquireLeases(Map<TimestampLeaseName,
                 requestedFreshTimestamps.keySet().equals(responseMap.keySet()),
                 "Response lease timestamps need to match request timestamp names exactly");
 
-        boolean wasFullyFulfilled = requestedFreshTimestamps.keySet().stream().allMatch(timestampName -> {
-            int requestedTimestamps = requestedFreshTimestamps.get(timestampName);
-            long returnedTimestamps =
-                    responseMap.get(timestampName).getFreshTimestamps().getCount();
-            return returnedTimestamps >= requestedTimestamps;
-        });
+        long wereNotFullyFulfilled = requestedFreshTimestamps.keySet().stream()
+                .filter(timestampName -> {
+                    int requestedTimestamps = requestedFreshTimestamps.get(timestampName);
+                    long returnedTimestamps =
+                            responseMap.get(timestampName).getFreshTimestamps().getCount();
+                    return returnedTimestamps < requestedTimestamps;
+                })
+                .count();
+
+        if (wereNotFullyFulfilled > 0) {
+            notEnoughFreshTimestampsCounter.inc(wereNotFullyFulfilled);
 
-        if (!wasFullyFulfilled) {
             unlock(response);
             log.info(
                     "Timestamp lease request was not fully fulfilled. This should happen infrequently.",
-                    SafeArg.of("requests", requests),
+                    SafeArg.of("requests", request),
                     SafeArg.of("responses", response));
             return Optional.empty();
+        } else {
+            return Optional.of(response);
         }
-
-        return Optional.of(response);
     }
 
     private void unlock(TimestampLeaseResponses responses) {

lock-api/src/main/metrics/metric-schema.yml

@@ -30,4 +30,10 @@ namespaces:
         type: timer
         docs: observed call duration during leader election
 
+  timestampLease:
+    docs: Timestamp lease client side telemetry
+    metrics:
+      notEnoughFreshTimestamps:
+        type: counter
+        docs: Counts instances where TimeLock returns less than the requested number of timestamps in acquire timestamp lease calls
 

timelock-server/src/testCommon/java/com/palantir/atlasdb/timelock/NamespacedClients.java

@@ -34,6 +34,7 @@
 import com.palantir.lock.client.NamespacedConjureTimelockServiceImpl;
 import com.palantir.lock.client.RemoteLockServiceAdapter;
 import com.palantir.lock.client.RemoteTimelockServiceAdapter;
+import com.palantir.lock.client.TimestampLeaseMetrics;
 import com.palantir.lock.client.timestampleases.MinLeasedTimestampGetter;
 import com.palantir.lock.client.timestampleases.MinLeasedTimestampGetterImpl;
 import com.palantir.lock.client.timestampleases.NamespacedTimestampLeaseService;
@@ -55,6 +56,7 @@
 import com.palantir.timestamp.TimestampManagementRpcClient;
 import com.palantir.timestamp.TimestampManagementService;
 import com.palantir.timestamp.TimestampRange;
+import com.palantir.tritium.metrics.registry.DefaultTaggedMetricRegistry;
 import java.util.Set;
 import org.immutables.value.Value;
 
@@ -94,8 +96,8 @@ default TimelockService timelockService() {
         NamespacedTimestampLeaseService timestampLeaseService = new NamespacedTimestampLeaseServiceImpl(
                 Namespace.of(namespace()), internalMultiClientConjureTimelockService());
 
-        TimestampLeaseAcquirer timestampLeaseAcquirer =
-                TimestampLeaseAcquirerImpl.create(timestampLeaseService, unlocker);
+        TimestampLeaseAcquirer timestampLeaseAcquirer = TimestampLeaseAcquirerImpl.create(
+                timestampLeaseService, unlocker, TimestampLeaseMetrics.of(new DefaultTaggedMetricRegistry()));
 
         MinLeasedTimestampGetter minLeasedTimestampGetter = new MinLeasedTimestampGetterImpl(timestampLeaseService);