[Named min timestamp leases] Metrics #7400

ergo14 · 2024-10-24T14:03:17Z

Metrics

ergo14 · 2024-10-24T14:06:32Z

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

+        long wereNotFullyFulfilled = requests.keySet().stream()
+                .filter(timestampName -> {
+                    int requestedTimestamps = requests.get(timestampName);
+                    long returnedTimestamps = responses.get(timestampName).getFreshTimestamps().getCount();
+                    if (returnedTimestamps >= requestedTimestamps) {
+                        return false;
+                    }
+                    telemetryTimestampShortFallConsumer.accept(requestedTimestamps - returnedTimestamps);
+                    return true;
+                })
+                .count();


This code path will be moved and factored as part of the previous PR reviews, so just review the logic here.

I'll move things later as appropriate

ergo14 · 2024-10-24T14:07:21Z

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

+                    if (returnedTimestamps >= requestedTimestamps) {
+                        return false;
+                    }
+                    telemetryTimestampShortFallConsumer.accept(requestedTimestamps - returnedTimestamps);


I only want to report shortfall > 0

An alternative is to report shortfall >= 0, or any possible shortfall (or overflow)

Realistically, we care about the count here rather than the histogram. But, it may be needed eventually

Why do we need a histogram?

I think we will struggle with interpreting these. If you care about magnitude, just log it. But I'd just log a binary based on wasFullyFulfilled > 0

and if that turns out to be false and we need more, we'll do that later, but at this point I'm struggling to think why I'd want to see the exact number.

Why do we need a histogram?

More data? Most appropriate would be a meter then? I don't usually like counters because of how they are reported but can use that as well

yeah, just a counter

ergo14 · 2024-10-24T14:07:58Z

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

@@ -30,4 +30,10 @@ namespaces:
        type: timer
        docs: observed call duration during leader election

+  timestampLease:


This will generate a class named the same as the other metrics in this PR. I think that's fine

ergo14 · 2024-10-24T14:09:45Z

...lock-impl/src/main/java/com/palantir/atlasdb/timelock/lock/NamedMinTimestampTrackerImpl.java

 import javax.annotation.concurrent.GuardedBy;

 final class NamedMinTimestampTrackerImpl implements NamedMinTimestampTracker {
    private final String timestampName;
+    private final AtomicInteger numLocksHeld;


Write path: this is modified in lock and unlock calls which are already synchronized

Read path: this will be ran in some telemetry wc thread, and the read seems to be a volatile read only - I can't imagine a less perf-impacting read (other than a final field) if we are to have memory visibility. I guess we could have a variable with no synchronization and hope for the best, the metrics don't need to be 100% sure

ergo14 · 2024-10-24T14:10:34Z

...lock-impl/src/main/java/com/palantir/atlasdb/timelock/lock/NamedMinTimestampTrackerImpl.java

 import javax.annotation.concurrent.GuardedBy;

 final class NamedMinTimestampTrackerImpl implements NamedMinTimestampTracker {
    private final String timestampName;
+    private final AtomicInteger numLocksHeld;
+    private final AtomicLong approximateMinLocked;


This one I only update when there are calls for getMinimumTimestamp as calling getFirstKey is O(tree height) in the tree map iirc

ergo14 · 2024-10-24T14:11:24Z

...lock-impl/src/main/java/com/palantir/atlasdb/timelock/lock/NamedMinTimestampTrackerImpl.java


    @GuardedBy("this")
    private final SortedMap<Long, UUID> holdersByTimestamp = new TreeMap<>();

-    NamedMinTimestampTrackerImpl(String timestampName) {
+    private NamedMinTimestampTrackerImpl(


I also considered adding meters for lock/unlock as generally those give more signal/granularity than a size metric

But, looking at Meter, updating it involves more CASs etc

ergo14 · 2024-10-24T14:12:20Z

...lock-impl/src/main/java/com/palantir/atlasdb/timelock/lock/NamedMinTimestampTrackerImpl.java

@@ -46,6 +64,7 @@ public synchronized void lock(long timestamp, UUID requestId) {
                    SafeArg.of("requestId", requestId),


One thing to note is: the code path is now different than before as this impacts the immutable timestamp. I can special case.

Of course, with all concurrency comments on this PR, I may be too paranoid here

ergo14 · 2024-10-24T14:12:45Z

timelock-impl/src/main/metrics/timestampleases.yml

+        type: gauge
+        tags:
+          - name
+      approximateSmallestLeased:


name to make sure any operator/dev knows this is best-effort and does not over-index

jkozlowski · 2024-10-24T14:41:57Z

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

@@ -61,24 +63,38 @@ public final class TimestampLeaseAcquirerImpl implements TimestampLeaseAcquirer
    private final NamespacedTimestampLeaseService delegate;
    private final Unlocker unlocker;
    private final Supplier<UUID> uuidSupplier;
+    private final Consumer<Long> telemetryTimestampShortFallConsumer;

    private final Retryer<RequestAndResponse> retryer = RetryerBuilder.<RequestAndResponse>newBuilder()
            .retryIfResult(requestAndResponse -> !wasRequestFullyFulfilled(requestAndResponse))
            .retryIfException()
            .withStopStrategy(StopStrategies.stopAfterAttempt(3))


Should we add some backoff?

Discussed offline that we won't be adding this now

jkozlowski · 2024-10-24T14:51:06Z

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

-        });
+        long wereNotFullyFulfilled = requests.keySet().stream()
+                .filter(timestampName -> {
+                    int requestedTimestamps = requests.get(timestampName);


Do we validate somewhere that these are non-zero?

No I can add a preconditions before I start the whole retry block?

jkozlowski · 2024-10-25T14:53:04Z

...lock-impl/src/main/java/com/palantir/atlasdb/timelock/lock/NamedMinTimestampTrackerImpl.java

    }

    @Override
    public synchronized Optional<Long> getMinimumTimestamp() {
        if (holdersByTimestamp.isEmpty()) {
            return Optional.empty();
        }
-        return Optional.of(holdersByTimestamp.firstKey());
+        Long minimum = holdersByTimestamp.firstKey();


Can we just report it in the same way we do right now in the immutable timestamp case? Just have it be done by Atlas and in the consumer application. Doing this in timelock breaks the usecase here of being able to write dashboard for a particular consumer (or at least maybe makes it more snowflakey/different than how we do all other metrics?)

jkozlowski · 2024-10-25T14:53:54Z

...lock-impl/src/main/java/com/palantir/atlasdb/timelock/lock/NamedMinTimestampTrackerImpl.java

    }

    @Override
    public synchronized Optional<Long> getMinimumTimestamp() {
        if (holdersByTimestamp.isEmpty()) {
            return Optional.empty();
        }
-        return Optional.of(holdersByTimestamp.firstKey());
+        Long minimum = holdersByTimestamp.firstKey();


And in general, I'm less bought in to the idea of this metric right now / care about it the least for the initial milestone, so at least split this out into a separate PR, so we can ship the counter first.

I will spin out a PR only for the shortfall thing

ergo14 · 2024-10-25T15:41:06Z

Setting to draft - need to:

Spin to 2 PRs
One for the size of the lock collection on TimeLock
One where I move the metric for the value of the named timestamp to the client side

[skip ci]

ergo14 force-pushed the aa-nmtl-metrics branch from 9a2ae70 to e3a0686 Compare October 24, 2024 14:05

ergo14 commented Oct 24, 2024

View reviewed changes

ergo14 changed the title ~~[Named min timestamp leases] Timestamp partial fill metric~~ [Named min timestamp leases] Metrics Oct 24, 2024

jkozlowski reviewed Oct 24, 2024

View reviewed changes

ergo14 force-pushed the aa-nmtl-wiring branch from 59546dd to c5620cc Compare October 24, 2024 15:15

ergo14 force-pushed the aa-nmtl-metrics branch from e3a0686 to cd3afb5 Compare October 24, 2024 15:25

ergo14 force-pushed the aa-nmtl-wiring branch from c5620cc to bdc5c3c Compare October 24, 2024 21:56

ergo14 force-pushed the aa-nmtl-metrics branch from cd3afb5 to a4e1d8e Compare October 24, 2024 22:15

ergo14 force-pushed the aa-nmtl-wiring branch from bdc5c3c to 80fe31c Compare October 25, 2024 12:37

ergo14 force-pushed the aa-nmtl-metrics branch from a4e1d8e to 9bdd9e3 Compare October 25, 2024 12:38

ergo14 force-pushed the aa-nmtl-wiring branch from 80fe31c to db8f15d Compare October 25, 2024 13:48

ergo14 force-pushed the aa-nmtl-metrics branch from 9bdd9e3 to 44ce9a8 Compare October 25, 2024 13:58

jkozlowski reviewed Oct 25, 2024

View reviewed changes

ergo14 force-pushed the aa-nmtl-wiring branch from db8f15d to 218cad2 Compare October 25, 2024 15:04

ergo14 force-pushed the aa-nmtl-metrics branch from 44ce9a8 to 2257446 Compare October 25, 2024 15:04

ergo14 mentioned this pull request Oct 25, 2024

[Named min timestamp leases] Metric tracking fresh ts shortfall #7402

Open

ergo14 force-pushed the aa-nmtl-metrics branch from 2257446 to 19d1124 Compare October 25, 2024 15:39

ergo14 marked this pull request as draft October 25, 2024 15:41

ergo14 force-pushed the aa-nmtl-wiring branch 2 times, most recently from 68962f6 to 69ff52c Compare October 25, 2024 15:54

ergo14 force-pushed the aa-nmtl-metrics branch from 19d1124 to d5df92a Compare October 25, 2024 15:59

svc-autorelease and others added 5 commits October 25, 2024 16:18

Release 0.1176.0

90862c4

[skip ci]

[Named min timestamp leases] Wiring it all except legacy (#7398)

ccfe5ae

Release 0.1177.0

5d62e44

[skip ci]

Metric tracking fresh ts shortfall

80ae615

Add all metrics

8005f25

ergo14 force-pushed the aa-nmtl-metrics branch from d5df92a to 8005f25 Compare October 25, 2024 16:20

ergo14 mentioned this pull request Oct 28, 2024

[Named min timestamp leases] Add metric for number of locked timestamps #7406

Open

ergo14 closed this Oct 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Named min timestamp leases] Metrics #7400

[Named min timestamp leases] Metrics #7400

ergo14 commented Oct 24, 2024

ergo14 Oct 24, 2024

ergo14 Oct 24, 2024

ergo14 Oct 24, 2024

jkozlowski Oct 24, 2024

jkozlowski Oct 24, 2024 •

edited

Loading

jkozlowski Oct 24, 2024

ergo14 Oct 24, 2024 •

edited

Loading

jkozlowski Oct 24, 2024

ergo14 Oct 24, 2024

ergo14 Oct 24, 2024

ergo14 Oct 24, 2024

ergo14 Oct 24, 2024

ergo14 Oct 24, 2024

ergo14 Oct 24, 2024

jkozlowski Oct 24, 2024

ergo14 Oct 25, 2024

jkozlowski Oct 24, 2024

ergo14 Oct 25, 2024

jkozlowski Oct 25, 2024

jkozlowski Oct 25, 2024

ergo14 Oct 25, 2024

ergo14 Oct 25, 2024

ergo14 commented Oct 25, 2024

		@@ -46,6 +64,7 @@ public synchronized void lock(long timestamp, UUID requestId) {
		SafeArg.of("requestId", requestId),

[Named min timestamp leases] Metrics #7400

[Named min timestamp leases] Metrics #7400

Conversation

ergo14 commented Oct 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkozlowski Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ergo14 Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ergo14 commented Oct 25, 2024

jkozlowski Oct 24, 2024 •

edited

Loading

ergo14 Oct 24, 2024 •

edited

Loading