From 1617ffaab07978a2960a3ea96cc5ab9ff8af8afc Mon Sep 17 00:00:00 2001
From: Jean-Paul Calderone <exarkun@twistedmatrix.com>
Date: Mon, 6 Dec 2021 16:13:50 -0500
Subject: [PATCH] rejected zkap metric
---
src/_zkapauthorizer/_storage_server.py | 40 +++++++++++++-
.../tests/test_storage_server.py | 53 +++++++++++++++++--
2 files changed, 88 insertions(+), 5 deletions(-)
diff --git a/src/_zkapauthorizer/_storage_server.py b/src/_zkapauthorizer/_storage_server.py
index 1b54113..58039f4 100644
--- a/src/_zkapauthorizer/_storage_server.py
+++ b/src/_zkapauthorizer/_storage_server.py
@@ -41,7 +41,7 @@ from attr.validators import instance_of, provides
from challenge_bypass_ristretto import SigningKey, TokenPreimage, VerificationSignature
from eliot import log_call, start_action
from foolscap.api import Referenceable
-from prometheus_client import CollectorRegistry, Histogram
+from prometheus_client import CollectorRegistry, Histogram, Counter
from twisted.internet.defer import Deferred
from twisted.internet.interfaces import IReactorTime
from twisted.python.filepath import FilePath
@@ -149,6 +149,11 @@ class _ValidationResult(object):
signature_check_failed=signature_check_failed,
)
+ def observe_error_metrics(self, error_metric):
+ num_signature_errors = len(self.signature_check_failed)
+ if num_signature_errors > 0:
+ error_metric.labels("signature").inc(1)
+
def raise_for(self, required_pass_count):
"""
:raise MorePassesRequired: Always raised with fields populated from this
@@ -199,8 +204,18 @@ class ZKAPAuthorizerStorageServer(Referenceable):
validator=provides(IReactorTime),
default=attr.Factory(partial(namedAny, "twisted.internet.reactor")),
)
+ # This histogram holds observations about the number of ZKAPs spent
+ # together on one operation. Only ZKAPs for operations that succeed are
+ # accounted for. For example, if two immutable shares are uploaded
+ # together at a cost of 5 ZKAPs then the "5 ZKAPs" bucket observes one
+ # sample.
_metric_spending_successes = attr.ib(init=False)
+ # This counter holds observations about spending attempts that included
+ # ZKAPs without an acceptable signature. For each spending attempt that
+ # includes any such ZKAPs, this counter is incremented.
+ _metric_spending_errors = attr.ib(init=False)
+
def _get_spending_histogram_buckets(self):
"""
Create the upper bounds for the ZKAP spending histogram.
@@ -222,7 +237,7 @@ class ZKAPAuthorizerStorageServer(Referenceable):
return list(2 ** n for n in range(11)) + [float("inf")]
@_metric_spending_successes.default
- def _make_histogram(self):
+ def _make_success_histogram(self):
return Histogram(
"zkapauthorizer_server_spending_successes",
"ZKAP Spending Successes histogram",
@@ -230,6 +245,15 @@ class ZKAPAuthorizerStorageServer(Referenceable):
buckets=self._get_spending_histogram_buckets(),
)
+ @_metric_spending_errors.default
+ def _make_error_metric(self):
+ return Counter(
+ "zkapauthorizer_server_spending_errors",
+ "ZKAP Spending Errors",
+ labelnames=["signature"],
+ registry=self._registry,
+ )
+
def _clear_metrics(self):
"""
Forget all recorded metrics.
@@ -238,6 +262,9 @@ class ZKAPAuthorizerStorageServer(Referenceable):
# https://github.com/prometheus/client_python/issues/707
self._metric_spending_successes._metric_init()
+ # It works on this one though.
+ self._metric_spending_errors.clear()
+
def remote_get_version(self):
"""
Pass-through without pass check to allow clients to learn about our
@@ -265,6 +292,9 @@ class ZKAPAuthorizerStorageServer(Referenceable):
self._signing_key,
)
+ # Observe error metrics before blowing up the operation.
+ validation.observe_error_metrics(self._metric_spending_errors)
+
# Note: The *allocate_buckets* protocol allows for some shares to
# already exist on the server. When this is the case, the cost of the
# operation is based only on the shares which are really allocated
@@ -344,6 +374,9 @@ class ZKAPAuthorizerStorageServer(Referenceable):
passes,
self._signing_key,
)
+ # Observe error metrics before blowing up the operation.
+ validation.observe_error_metrics(self._metric_spending_errors)
+
check_pass_quantity_for_lease(
self._pass_value,
storage_index,
@@ -442,6 +475,9 @@ class ZKAPAuthorizerStorageServer(Referenceable):
self._signing_key,
)
+ # Observe error metrics before blowing up the operation.
+ validation.observe_error_metrics(self._metric_spending_errors)
+
# Inspect the operation to determine its price based on any
# allocations.
required_new_passes = get_writev_price(
diff --git a/src/_zkapauthorizer/tests/test_storage_server.py b/src/_zkapauthorizer/tests/test_storage_server.py
index ad570cb..65ffd2b 100644
--- a/src/_zkapauthorizer/tests/test_storage_server.py
+++ b/src/_zkapauthorizer/tests/test_storage_server.py
@@ -152,6 +152,9 @@ def read_spending_success_histogram_total(storage_server):
return sum(b.get() for b in buckets)
+def read_invalid_count(storage_server, label):
+ return storage_server._metric_spending_errors.labels(label)._value.get()
+
def read_spending_success_histogram_bucket(storage_server, num_passes):
# type: (ZKAPAuthorizerStorageServer, int) -> int
"""
@@ -195,6 +198,7 @@ class PassValidationTests(TestCase):
AnonymousStorageServer(self.clock),
).storage_server
self.signing_key = random_signing_key()
+ self.unrelated_signing_key = random_signing_key()
self.storage_server = ZKAPAuthorizerStorageServer(
self.anonymous_storage_server,
self.pass_value,
@@ -580,12 +584,14 @@ class PassValidationTests(TestCase):
lease_cancel_secrets(),
),
test_and_write_vectors_for_shares=slot_test_and_write_vectors_for_shares(),
+ num_invalid_passes=integers(min_value=0, max_value=10),
)
def test_mutable_spending_metrics(
self,
storage_index,
secrets,
test_and_write_vectors_for_shares,
+ num_invalid_passes,
):
tw_vectors = {
k: v.for_call() for (k, v) in test_and_write_vectors_for_shares.items()
@@ -600,12 +606,17 @@ class PassValidationTests(TestCase):
slot_testv_and_readv_and_writev_message(storage_index),
list(RandomToken.create() for i in range(num_passes)),
)
+ invalid_passes = make_passes(
+ self.unrelated_signing_key,
+ slot_testv_and_readv_and_writev_message(storage_index),
+ list(RandomToken.create() for i in range(num_invalid_passes)),
+ )
test, read = self.storage_server.doRemoteCall(
"slot_testv_and_readv_and_writev",
(),
dict(
- passes=valid_passes,
+ passes=valid_passes + invalid_passes,
storage_index=storage_index,
secrets=secrets,
tw_vectors=tw_vectors,
@@ -617,12 +628,18 @@ class PassValidationTests(TestCase):
after_bucket = read_spending_success_histogram_bucket(
self.storage_server, num_passes
)
+ after_invalid_count = read_invalid_count(self.storage_server, "signature")
self.expectThat(
after_count,
Equals(1),
"Unexpected histogram sum value",
)
+ self.expectThat(
+ after_invalid_count,
+ Equals(1 if invalid_passes else 0),
+ "Unexpected invalid passes counter value",
+ )
self.assertThat(
after_bucket,
Equals(1),
@@ -703,6 +720,7 @@ class PassValidationTests(TestCase):
existing_sharenums=sharenum_sets(),
new_sharenums=sharenum_sets(),
size=sizes(),
+ num_invalid_passes=integers(min_value=0, max_value=10),
)
def test_immutable_spending_metrics(
self,
@@ -712,10 +730,15 @@ class PassValidationTests(TestCase):
existing_sharenums,
new_sharenums,
size,
+ num_invalid_passes,
):
"""
When ZKAPs are spent to call *allocate_buckets* the number of passes spent
is recorded as a metric.
+
+ :param num_invalid_passes: A number of additional passes to supply
+ with the operation. These passes will not be considered valid by
+ the server and should be recorded as such.
"""
# maybe create some existing shares that won't need to be paid for by
# the subsequent `allocate_buckets` operation - but of which the
@@ -742,12 +765,17 @@ class PassValidationTests(TestCase):
allocate_buckets_message(storage_index),
list(RandomToken.create() for i in range(num_passes)),
)
+ invalid_passes = make_passes(
+ self.unrelated_signing_key,
+ allocate_buckets_message(storage_index),
+ list(RandomToken.create() for i in range(num_invalid_passes)),
+ )
alreadygot, allocated = self.storage_server.doRemoteCall(
"allocate_buckets",
(),
dict(
- passes=valid_passes,
+ passes=valid_passes + invalid_passes,
storage_index=storage_index,
renew_secret=renew_secret,
cancel_secret=cancel_secret,
@@ -761,6 +789,7 @@ class PassValidationTests(TestCase):
after_bucket = read_spending_success_histogram_bucket(
self.storage_server, num_spent_passes
)
+ after_invalid_count = read_invalid_count(self.storage_server, "signature")
self.expectThat(
after_count,
@@ -769,6 +798,11 @@ class PassValidationTests(TestCase):
)
# If this bucket is 1 then all the other buckets must be 0, otherwise
# the sum above will be greater than 1.
+ self.expectThat(
+ after_invalid_count,
+ Equals(1 if invalid_passes else 0),
+ "Unexpected invalid passes counter value",
+ )
self.assertThat(
after_bucket,
Equals(1),
@@ -781,6 +815,7 @@ class PassValidationTests(TestCase):
cancel_secret=lease_cancel_secrets(),
sharenums=sharenum_sets(),
allocated_size=sizes(),
+ num_invalid_passes=integers(min_value=0, max_value=10),
)
def test_add_lease_metrics(
self,
@@ -789,6 +824,7 @@ class PassValidationTests(TestCase):
cancel_secret,
sharenums,
allocated_size,
+ num_invalid_passes,
):
# Create some shares at a slot which will require lease renewal.
write_toy_shares(
@@ -809,12 +845,17 @@ class PassValidationTests(TestCase):
add_lease_message(storage_index),
list(RandomToken.create() for i in range(num_passes)),
)
+ invalid_passes = make_passes(
+ self.unrelated_signing_key,
+ add_lease_message(storage_index),
+ list(RandomToken.create() for i in range(num_invalid_passes)),
+ )
self.storage_server.doRemoteCall(
"add_lease",
(),
dict(
- passes=valid_passes,
+ passes=valid_passes + invalid_passes,
storage_index=storage_index,
renew_secret=renew_secret,
cancel_secret=cancel_secret,
@@ -825,12 +866,18 @@ class PassValidationTests(TestCase):
after_bucket = read_spending_success_histogram_bucket(
self.storage_server, num_passes
)
+ after_invalid_count = read_invalid_count(self.storage_server, "signature")
self.expectThat(
after_count,
Equals(1),
"Unexpected histogram sum value",
)
+ self.expectThat(
+ after_invalid_count,
+ Equals(1 if invalid_passes else 0),
+ "Unexpected invalid passes counter value",
+ )
self.assertThat(
after_bucket,
Equals(1),
--
GitLab