From b358204e9ff84393fe876bbfc7bb82ab969402e1 Mon Sep 17 00:00:00 2001 From: Ahmed Alouane Date: Fri, 25 Oct 2024 16:28:31 +0100 Subject: [PATCH] Metric tracking fresh ts shortfall --- ...DefaultLockAndTimestampServiceFactory.java | 11 ++-- .../AbstractInMemoryTimelockExtension.java | 11 ++-- lock-api/build.gradle | 1 + .../TimestampLeaseAcquirerImpl.java | 50 ++++++++++++------- lock-api/src/main/metrics/metric-schema.yml | 6 +++ .../atlasdb/timelock/NamespacedClients.java | 6 ++- 6 files changed, 57 insertions(+), 28 deletions(-) diff --git a/atlasdb-config/src/main/java/com/palantir/atlasdb/factory/DefaultLockAndTimestampServiceFactory.java b/atlasdb-config/src/main/java/com/palantir/atlasdb/factory/DefaultLockAndTimestampServiceFactory.java index e9039c6b5c8..de1da0f460e 100644 --- a/atlasdb-config/src/main/java/com/palantir/atlasdb/factory/DefaultLockAndTimestampServiceFactory.java +++ b/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 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); diff --git a/atlasdb-tests-shared/src/main/java/com/palantir/timelock/paxos/AbstractInMemoryTimelockExtension.java b/atlasdb-tests-shared/src/main/java/com/palantir/timelock/paxos/AbstractInMemoryTimelockExtension.java index bf9ec797c59..f1db1deb740 100644 --- a/atlasdb-tests-shared/src/main/java/com/palantir/timelock/paxos/AbstractInMemoryTimelockExtension.java +++ b/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)); } diff --git a/lock-api/build.gradle b/lock-api/build.gradle index c486f57caab..b066ca23040 100644 --- a/lock-api/build.gradle +++ b/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') diff --git a/lock-api/src/main/java/com/palantir/lock/client/timestampleases/TimestampLeaseAcquirerImpl.java b/lock-api/src/main/java/com/palantir/lock/client/timestampleases/TimestampLeaseAcquirerImpl.java index 6ad336b7f79..2930793c30f 100644 --- a/lock-api/src/main/java/com/palantir/lock/client/timestampleases/TimestampLeaseAcquirerImpl.java +++ b/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,6 +64,7 @@ public final class TimestampLeaseAcquirerImpl implements TimestampLeaseAcquirer private final NamespacedTimestampLeaseService delegate; private final Unlocker unlocker; private final Supplier uuidSupplier; + private final Counter notEnoughFreshTimestampsCounter; private final Retryer> retryer = RetryerBuilder.>newBuilder() @@ -69,21 +72,30 @@ public final class TimestampLeaseAcquirerImpl implements TimestampLeaseAcquirer .withStopStrategy(StopStrategies.stopAfterAttempt(3)) .build(); - @VisibleForTesting - TimestampLeaseAcquirerImpl( - NamespacedTimestampLeaseService delegate, Unlocker unlocker, Supplier uuidSupplier) { + private TimestampLeaseAcquirerImpl( + NamespacedTimestampLeaseService delegate, + Unlocker unlocker, + Supplier 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 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 acquireLeases(Map { - 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) { diff --git a/lock-api/src/main/metrics/metric-schema.yml b/lock-api/src/main/metrics/metric-schema.yml index 6e06eca0dac..783a157ad05 100644 --- a/lock-api/src/main/metrics/metric-schema.yml +++ b/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 diff --git a/timelock-server/src/testCommon/java/com/palantir/atlasdb/timelock/NamespacedClients.java b/timelock-server/src/testCommon/java/com/palantir/atlasdb/timelock/NamespacedClients.java index 06786f0cbcb..263bad871ac 100644 --- a/timelock-server/src/testCommon/java/com/palantir/atlasdb/timelock/NamespacedClients.java +++ b/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);