Avoid metrics on reentry of lock

3 files changed, 54 insertions, 29 deletions

diff --git a/zkfacade/src/main/java/com/yahoo/vespa/curator/stats/LockAttempt.java b/zkfacade/src/main/java/com/yahoo/vespa/curator/stats/LockAttempt.java
index 1478b36d331..9cf490bf8c6 100644
--- a/zkfacade/src/main/java/com/yahoo/vespa/curator/stats/LockAttempt.java
+++ b/zkfacade/src/main/java/com/yahoo/vespa/curator/stats/LockAttempt.java
@@ -21,19 +21,21 @@ public class LockAttempt {
     private final String lockPath;
     private final Instant callAcquireInstant;
     private final Duration timeout;
+    private final boolean reentry;
     private final LockMetrics lockMetrics;
     private final List<LockAttempt> nestedLockAttempts = new ArrayList<>();
     private final LatencyStats.ActiveInterval activeAcquireInterval;
     // Only accessed by mutating thread:
-    private LatencyStats.ActiveInterval activeLockedInterval = null;
+    private Optional<LatencyStats.ActiveInterval> activeLockedInterval = Optional.empty();
 
     private volatile Optional<Instant> lockAcquiredInstant = Optional.empty();
     private volatile Optional<Instant> terminalStateInstant = Optional.empty();
     private volatile Optional<String> stackTrace = Optional.empty();
 
     public static LockAttempt invokingAcquire(ThreadLockStats threadLockStats, String lockPath,
-                                              Duration timeout, LockMetrics lockMetrics) {
-        return new LockAttempt(threadLockStats, lockPath, timeout, Instant.now(), lockMetrics);
+                                              Duration timeout, LockMetrics lockMetrics,
+                                              boolean reentry) {
+        return new LockAttempt(threadLockStats, lockPath, timeout, Instant.now(), lockMetrics, reentry);
     }
 
     public enum LockState {
@@ -50,13 +52,14 @@ public class LockAttempt {
     private volatile LockState lockState = LockState.ACQUIRING;
 
     private LockAttempt(ThreadLockStats threadLockStats, String lockPath, Duration timeout,
-                        Instant callAcquireInstant, LockMetrics lockMetrics) {
+                        Instant callAcquireInstant, LockMetrics lockMetrics, boolean reentry) {
         this.threadLockStats = threadLockStats;
         this.lockPath = lockPath;
         this.callAcquireInstant = callAcquireInstant;
         this.timeout = timeout;
         this.lockMetrics = lockMetrics;
-        this.activeAcquireInterval = lockMetrics.acquireInvoked();
+        this.reentry = reentry;
+        this.activeAcquireInterval = lockMetrics.acquireInvoked(reentry);
     }
 
     public String getThreadName() { return threadLockStats.getThreadName(); }
@@ -101,22 +104,22 @@ public class LockAttempt {
 
     void acquireFailed() {
         setTerminalState(LockState.ACQUIRE_FAILED);
-        lockMetrics.acquireFailed(activeAcquireInterval);
+        lockMetrics.acquireFailed(reentry, activeAcquireInterval);
     }
 
     void timedOut() {
         setTerminalState(LockState.TIMED_OUT);
-        lockMetrics.acquireTimedOut(activeAcquireInterval);
+        lockMetrics.acquireTimedOut(reentry, activeAcquireInterval);
     }
 
     void lockAcquired() {
         lockState = LockState.ACQUIRED;
         lockAcquiredInstant = Optional.of(Instant.now());
-        activeLockedInterval = lockMetrics.lockAcquired(activeAcquireInterval);
+        activeLockedInterval = Optional.of(lockMetrics.lockAcquired(reentry, activeAcquireInterval));
     }
 
     void preRelease() {
-        lockMetrics.preRelease(activeLockedInterval);
+        lockMetrics.preRelease(reentry, activeLockedInterval.orElseThrow());
     }
 
     void postRelease() {
@@ -125,7 +128,7 @@ public class LockAttempt {
 
     void releaseFailed() {
         setTerminalState(LockState.RELEASED_WITH_ERROR);
-        lockMetrics.releaseFailed();
+        lockMetrics.releaseFailed(reentry);
     }
 
     void setTerminalState(LockState terminalState) { setTerminalState(terminalState, Instant.now()); }
diff --git a/zkfacade/src/main/java/com/yahoo/vespa/curator/stats/LockMetrics.java b/zkfacade/src/main/java/com/yahoo/vespa/curator/stats/LockMetrics.java
index 18ab70d42da..36758354171 100644
--- a/zkfacade/src/main/java/com/yahoo/vespa/curator/stats/LockMetrics.java
+++ b/zkfacade/src/main/java/com/yahoo/vespa/curator/stats/LockMetrics.java
@@ -17,6 +17,7 @@ public class LockMetrics {
     private final AtomicInteger acquireSucceededCount = new AtomicInteger(0);
     private final AtomicInteger releaseCount = new AtomicInteger(0);
     private final AtomicInteger releaseFailedCount = new AtomicInteger(0);
+    private final AtomicInteger reentryCount = new AtomicInteger(0);
 
     private final AtomicInteger cumulativeAcquireCount = new AtomicInteger(0);
     private final AtomicInteger cumulativeAcquireFailedCount = new AtomicInteger(0);
@@ -24,43 +25,55 @@ public class LockMetrics {
     private final AtomicInteger cumulativeAcquireSucceededCount = new AtomicInteger(0);
     private final AtomicInteger cumulativeReleaseCount = new AtomicInteger(0);
     private final AtomicInteger cumulativeReleaseFailedCount = new AtomicInteger(0);
+    private final AtomicInteger cumulativeReentryCount = new AtomicInteger(0);
 
     private final LatencyStats acquireStats = new LatencyStats();
     private final LatencyStats lockedStats = new LatencyStats();
 
     /** Returns a Runnable that must be invoked when the acquire() finishes. */
-    ActiveInterval acquireInvoked() {
+    ActiveInterval acquireInvoked(boolean reentry) {
+        if (reentry) {
+            reentryCount.incrementAndGet();
+            cumulativeReentryCount.incrementAndGet();
+            return () -> { };
+        }
+
         acquireCount.incrementAndGet();
         cumulativeAcquireCount.incrementAndGet();
         return acquireStats.startNewInterval();
     }
 
-    void acquireFailed(ActiveInterval acquireInterval) {
+    void acquireFailed(boolean reentry, ActiveInterval acquireInterval) {
         acquireInterval.close();
+        if (reentry) return;
         acquireFailedCount.incrementAndGet();
         cumulativeAcquireFailedCount.incrementAndGet();
     }
 
-    void acquireTimedOut(ActiveInterval acquireInterval) {
+    void acquireTimedOut(boolean reentry, ActiveInterval acquireInterval) {
         acquireInterval.close();
+        if (reentry) return;
         acquireTimedOutCount.incrementAndGet();
         cumulativeAcquireTimedOutCount.incrementAndGet();
     }
 
-    ActiveInterval lockAcquired(ActiveInterval acquireInterval) {
+    ActiveInterval lockAcquired(boolean reentry, ActiveInterval acquireInterval) {
         acquireInterval.close();
+        if (reentry) return () -> {};
         acquireSucceededCount.incrementAndGet();
         cumulativeAcquireSucceededCount.incrementAndGet();
         return lockedStats.startNewInterval();
     }
 
-    void preRelease(ActiveInterval lockedInterval) {
+    void preRelease(boolean reentry, ActiveInterval lockedInterval) {
         lockedInterval.close();
+        if (reentry) return;
         releaseCount.incrementAndGet();
         cumulativeReleaseCount.incrementAndGet();
     }
 
-    void releaseFailed() {
+    void releaseFailed(boolean reentry) {
+        if (reentry) return;
         releaseFailedCount.incrementAndGet();
         cumulativeReleaseFailedCount.incrementAndGet();
     }
@@ -71,6 +84,7 @@ public class LockMetrics {
     public int getAndResetAcquireSucceededCount() { return acquireSucceededCount.getAndSet(0); }
     public int getAndResetReleaseCount() { return releaseCount.getAndSet(0); }
     public int getAndResetReleaseFailedCount() { return releaseFailedCount.getAndSet(0); }
+    public int getAndResetReentryCount() { return reentryCount.getAndSet(0); }
 
     public int getCumulativeAcquireCount() { return cumulativeAcquireCount.get(); }
     public int getCumulativeAcquireFailedCount() { return cumulativeAcquireFailedCount.get(); }
@@ -78,6 +92,7 @@ public class LockMetrics {
     public int getCumulativeAcquireSucceededCount() { return cumulativeAcquireSucceededCount.get(); }
     public int getCumulativeReleaseCount() { return cumulativeReleaseCount.get(); }
     public int getCumulativeReleaseFailedCount() { return cumulativeReleaseFailedCount.get(); }
+    public int getCumulativeReentryCount() { return cumulativeReentryCount.get(); }
 
     public LatencyMetrics getAcquireLatencyMetrics() { return acquireStats.getLatencyMetrics(); }
     public LatencyMetrics getLockedLatencyMetrics() { return lockedStats.getLatencyMetrics(); }
@@ -86,20 +101,22 @@ public class LockMetrics {
     public LatencyMetrics getAndResetLockedLatencyMetrics() { return lockedStats.getLatencyMetricsAndStartNewPeriod(); }
 
     //  For tests
-    void setAcquireCount(int count) { acquireCount.set(count); }
-    void setAcquireFailedCount(int count) { acquireFailedCount.set(count); }
-    void setAcquireTimedOutCount(int count) { acquireTimedOutCount.set(count); }
-    void setAcquireSucceededCount(int count) { acquireSucceededCount.set(count); }
-    void setReleaseCount(int count) { releaseCount.set(count); }
-    void setReleaseFailedCount(int count) { releaseFailedCount.set(count); }
+    LockMetrics setAcquireCount(int count) { acquireCount.set(count); return this; }
+    LockMetrics setAcquireFailedCount(int count) { acquireFailedCount.set(count); return this; }
+    LockMetrics setAcquireTimedOutCount(int count) { acquireTimedOutCount.set(count); return this; }
+    LockMetrics setAcquireSucceededCount(int count) { acquireSucceededCount.set(count); return this; }
+    LockMetrics setReleaseCount(int count) { releaseCount.set(count); return this; }
+    LockMetrics setReleaseFailedCount(int count) { releaseFailedCount.set(count); return this; }
+    LockMetrics setReentryCount(int count) { reentryCount.set(count); return this; }
 
     //  For tests
-    void setCumulativeAcquireCount(int count) { cumulativeAcquireCount.set(count); }
-    void setCumulativeAcquireFailedCount(int count) { cumulativeAcquireFailedCount.set(count); }
-    void setCumulativeAcquireTimedOutCount(int count) { cumulativeAcquireTimedOutCount.set(count); }
-    void setCumulativeAcquireSucceededCount(int count) { cumulativeAcquireSucceededCount.set(count); }
-    void setCumulativeReleaseCount(int count) { cumulativeReleaseCount.set(count); }
-    void setCumulativeReleaseFailedCount(int count) { cumulativeReleaseFailedCount.set(count); }
+    LockMetrics setCumulativeAcquireCount(int count) { cumulativeAcquireCount.set(count); return this; }
+    LockMetrics setCumulativeAcquireFailedCount(int count) { cumulativeAcquireFailedCount.set(count); return this; }
+    LockMetrics setCumulativeAcquireTimedOutCount(int count) { cumulativeAcquireTimedOutCount.set(count); return this; }
+    LockMetrics setCumulativeAcquireSucceededCount(int count) { cumulativeAcquireSucceededCount.set(count); return this; }
+    LockMetrics setCumulativeReleaseCount(int count) { cumulativeReleaseCount.set(count); return this; }
+    LockMetrics setCumulativeReleaseFailedCount(int count) { cumulativeReleaseFailedCount.set(count); return this; }
+    LockMetrics setCumulativeReentryCount(int count) { cumulativeReentryCount.set(count); return this; }
 
     @Override
     public String toString() {
@@ -110,12 +127,14 @@ public class LockMetrics {
                 ", acquireSucceededCount=" + acquireSucceededCount +
                 ", releaseCount=" + releaseCount +
                 ", releaseFailedCount=" + releaseFailedCount +
+                ", reentryCount=" + reentryCount +
                 ", cumulativeAcquireCount=" + cumulativeAcquireCount +
                 ", cumulativeAcquireFailedCount=" + cumulativeAcquireFailedCount +
                 ", cumulativeAcquireTimedOutCount=" + cumulativeAcquireTimedOutCount +
                 ", cumulativeAcquireSucceededCount=" + cumulativeAcquireSucceededCount +
                 ", cumulativeReleaseCount=" + cumulativeReleaseCount +
                 ", cumulativeReleaseFailedCount=" + cumulativeReleaseFailedCount +
+                ", cumulativeReentryCount=" + cumulativeReentryCount +
                 ", acquireStats=" + acquireStats +
                 ", lockedStats=" + lockedStats +
                 '}';
diff --git a/zkfacade/src/main/java/com/yahoo/vespa/curator/stats/ThreadLockStats.java b/zkfacade/src/main/java/com/yahoo/vespa/curator/stats/ThreadLockStats.java
index 964aa83c52f..393fac5e3db 100644
--- a/zkfacade/src/main/java/com/yahoo/vespa/curator/stats/ThreadLockStats.java
+++ b/zkfacade/src/main/java/com/yahoo/vespa/curator/stats/ThreadLockStats.java
@@ -64,7 +64,10 @@ public class ThreadLockStats {
 
     /** Mutable method (see class doc) */
     public void invokingAcquire(String lockPath, Duration timeout) {
-        LockAttempt lockAttempt = LockAttempt.invokingAcquire(this, lockPath, timeout, getGlobalLockMetrics(lockPath));
+        boolean reentry = lockAttemptsStack.stream().anyMatch(lockAttempt -> lockAttempt.getLockPath().equals(lockPath));
+
+        LockAttempt lockAttempt = LockAttempt.invokingAcquire(this, lockPath, timeout,
+                getGlobalLockMetrics(lockPath), reentry);
 
         LockAttempt lastLockAttempt = lockAttemptsStack.peekLast();
         if (lastLockAttempt == null) {