From e0e8bfb91dcfad1f229d2cc1a3f2c09ce65f01ae Mon Sep 17 00:00:00 2001
From: Jean-Paul Calderone <exarkun@twistedmatrix.com>
Date: Mon, 6 Dec 2021 15:08:23 -0500
Subject: [PATCH] simplify successful spending metrics
Let's see how far we get with just this simpler version. We now have a
histogram for number of zkaps spent, bucketed by the size of individual spend
operations.
Also implement it for mutables
---
src/_zkapauthorizer/_storage_server.py | 136 ++++--------------
src/_zkapauthorizer/tests/test_plugin.py | 2 +-
.../tests/test_storage_server.py | 106 +++-----------
3 files changed, 46 insertions(+), 198 deletions(-)
diff --git a/src/_zkapauthorizer/_storage_server.py b/src/_zkapauthorizer/_storage_server.py
index 7d72edc..c5ce9a5 100644
--- a/src/_zkapauthorizer/_storage_server.py
+++ b/src/_zkapauthorizer/_storage_server.py
@@ -61,7 +61,7 @@ from .storage_common import (
)
try:
- from typing import Dict, Generator, Iterable, List, Optional, Tuple
+ from typing import Dict, List, Optional
except ImportError:
pass
@@ -156,63 +156,6 @@ class LeaseRenewalRequired(Exception):
"""
-def observe_spending_successes(metric, observations):
- # type: (Histogram, Iterable[Tuple[int, int]]) -> None
- """
- Put some spending observations into a Histogram.
-
- :param observations: The first element of each tuple is the size of a
- share for which spending ocurred. The second element of each tuple is
- the number of passes spent on that share.
- """
- # After https://github.com/prometheus/client_python/pull/734 we can get
- # rid of the inner loop.
- for (size, count) in observations:
- for _ in range(count):
- metric.observe(size)
-
-
-def compute_spending_metrics(bytes_per_pass, sizes):
- # type: (int, List[int]) -> Generator[Tuple[int, int]]
- """
- Attribute portions of a payment for one or more shares to the individual
- shares. This supports maintaining a histogram of spending where
- information is placed in buckets by the size of the data is relates to.
-
- This is somewhat less straightforward than one might hope because payment
- for more than one share combines all of the share sizes for the purposes
- of pricing. We have to reverse engineer that combination to attribute
- portions of the spending to each share. We do this by noting that price
- is proportional to size and by allowing for some imprecision when a share
- size does not fall exactly on a multiple of pass value.
-
- :param bytes_per_pass: The number of bytes one pass pays for for one
- storage period.
-
- :param sizes: The sizes of the shares that were paid for.
-
- :return: A generator of tuples that describe a share size in bytes and a
- number of passes spent to store the share of that size. Each element
- from ``sizes`` will be represented in this result.
- """
- if len(sizes) == 0:
- return
- overrun = 0
- # Make a copy so we can pop one off. Also sort the sizes so we have a
- # consistent result for a given collection of sizes independent of the
- # order they're considered.
- values = sorted(list(sizes))
- last_allocated_size = values.pop()
- for allocated_size in values:
- share_result, overrun = divmod(allocated_size + overrun, bytes_per_pass)
- yield (allocated_size, share_result)
-
- share_result, overrun = divmod(last_allocated_size + overrun, bytes_per_pass)
- if overrun > 0:
- share_result += 1
- yield (last_allocated_size, share_result)
-
-
@implementer(RIPrivacyPassAuthorizedStorageServer)
# It would be great to use `frozen=True` (value-based hashing) instead of
# `cmp=False` (identity based hashing) but Referenceable wants to set some
@@ -247,35 +190,23 @@ class ZKAPAuthorizerStorageServer(Referenceable):
def _get_buckets(self):
"""
- Create the upper bounds for the ZKAP spending histogram. The bounds are
- set as a function of the pass value.
-
- For example, if the value of a pass is 1 MB then the upper bounds for
- the buckets will be:
-
- 32 KB
- 64 KB
- 128 KB
- 256 KB
- 512 KB
- 1 MB
- 10 MB
- 100 MB
- 1 GB
- INF
+ Create the upper bounds for the ZKAP spending histogram.
"""
- return (
- self._pass_value / 32,
- self._pass_value / 16,
- self._pass_value / 8,
- self._pass_value / 4,
- self._pass_value / 2,
- self._pass_value,
- self._pass_value * 10,
- self._pass_value * 100,
- self._pass_value * 1000,
- float("inf"),
- )
+ # We want a lot of small buckets to be able to get an idea of how much
+ # spending is for tiny files where our billing system doesn't work
+ # extremely well. We also want some large buckets so we have a point
+ # of comparison - is there a lot more or less spending on big files
+ # than small files? Prometheus recommends a metric have a maximum
+ # cardinality below 10
+ # (<https://prometheus.io/docs/practices/instrumentation/#do-not-overuse-labels>).
+ # Histograms are implemented with labels so the cardinality is equal
+ # to the number of buckets. We will push this a little bit so we can
+ # span a better range. The good news is that this is a static
+ # cardinality (it does not change based on the data observed) so we
+ # are not at risk of blowing up the metrics overhead unboundedly. 11
+ # finite buckets + 1 infinite bucket covers 1 to 1024 ZKAPs (plus
+ # infinity) and only needs 12 buckets.
+ return list(2 ** n for n in range(11)) + [float("inf")]
def __attrs_post_init__(self):
self._metric_spending_successes = Histogram(
@@ -311,16 +242,10 @@ class ZKAPAuthorizerStorageServer(Referenceable):
passes,
self._signing_key,
)
- observe_spending_successes(
- self._metric_spending_successes,
- # All immutable shares are the same size so the metrics system can
- # observe a single event for that size and the total number of
- # spent passes.
- #
- # XXX Some shares may exist already so those passes aren't
- # necessarily spent...
- [(allocated_size, len(validation.valid))],
- )
+
+ # XXX Some shares may exist already so those passes aren't necessarily
+ # spent...
+ self._metric_spending_successes.observe(len(validation.valid))
# Note: The *allocate_buckets* protocol allows for some shares to
# already exist on the server. When this is the case, the cost of the
@@ -384,26 +309,13 @@ class ZKAPAuthorizerStorageServer(Referenceable):
passes,
self._signing_key,
)
- allocated_sizes = check_pass_quantity_for_lease(
+ check_pass_quantity_for_lease(
self._pass_value,
storage_index,
validation,
self._original,
)
- # siiiigh. Tahoe doesn't guarantee that mutable shares in a single
- # slot are all the same size. They *probably* are. The official
- # (only) Tahoe client always makes them the same size. The storage
- # protocol allows a client to make them different sizes though.
- #
- # So ... deal with that here. Attribute the ZKAPs being spent
- # proportionally to the size of each share.
- observe_spending_successes(
- self._metric_spending_successes,
- compute_spending_metrics(
- self._pass_value,
- allocated_sizes.values(),
- ),
- )
+ self._metric_spending_successes.observe(len(validation.valid))
return self._original.remote_add_lease(storage_index, *a, **kw)
@@ -521,6 +433,8 @@ class ZKAPAuthorizerStorageServer(Referenceable):
renew_leases=False,
)
+ self._metric_spending_successes.observe(required_new_passes)
+
# Add the leases that we charged the client for. This includes:
#
# - leases on newly created shares
diff --git a/src/_zkapauthorizer/tests/test_plugin.py b/src/_zkapauthorizer/tests/test_plugin.py
index d80201e..c4db72a 100644
--- a/src/_zkapauthorizer/tests/test_plugin.py
+++ b/src/_zkapauthorizer/tests/test_plugin.py
@@ -78,8 +78,8 @@ from .matchers import Provides, raises
from .strategies import (
announcements,
client_dummyredeemer_configurations,
- clocks,
client_lease_maintenance_configurations,
+ clocks,
dummy_ristretto_keys,
lease_cancel_secrets,
lease_maintenance_configurations,
diff --git a/src/_zkapauthorizer/tests/test_storage_server.py b/src/_zkapauthorizer/tests/test_storage_server.py
index 3f637b1..1abecc5 100644
--- a/src/_zkapauthorizer/tests/test_storage_server.py
+++ b/src/_zkapauthorizer/tests/test_storage_server.py
@@ -25,17 +25,12 @@ from challenge_bypass_ristretto import RandomToken, random_signing_key
from foolscap.referenceable import LocalReferenceable
from hypothesis import given, note
from hypothesis.strategies import integers, just, lists, one_of, tuples
-from prometheus_client import CollectorRegistry, Histogram
-from testtools import TestCase, skip
+from testtools import TestCase
from testtools.matchers import AfterPreprocessing, Equals, MatchesAll
from twisted.internet.task import Clock
from twisted.python.runtime import platform
-from .._storage_server import (
- _ValidationResult,
- compute_spending_metrics,
- observe_spending_successes,
-)
+from .._storage_server import _ValidationResult
from ..api import MorePassesRequired, ZKAPAuthorizerStorageServer
from ..storage_common import (
add_lease_message,
@@ -549,7 +544,6 @@ class PassValidationTests(TestCase):
Equals(expected_sizes),
)
- @skip("do it after #169")
@given(
storage_index=storage_indexes(),
secrets=tuples(
@@ -568,7 +562,7 @@ class PassValidationTests(TestCase):
tw_vectors = {
k: v.for_call() for (k, v) in test_and_write_vectors_for_shares.items()
}
- expected = get_required_new_passes_for_mutable_write(
+ num_passes = get_required_new_passes_for_mutable_write(
self.pass_value,
dict.fromkeys(tw_vectors.keys(), 0),
tw_vectors,
@@ -576,11 +570,11 @@ class PassValidationTests(TestCase):
valid_passes = make_passes(
self.signing_key,
slot_testv_and_readv_and_writev_message(storage_index),
- list(RandomToken.create() for i in range(expected)),
+ list(RandomToken.create() for i in range(num_passes)),
)
before_count = read_count(self.storage_server)
- before_bucket = read_bucket(self.storage_server, 0)
+ before_bucket = read_bucket(self.storage_server, num_passes)
# Create an initial share to toy with.
test, read = self.storage_server.doRemoteCall(
@@ -596,16 +590,16 @@ class PassValidationTests(TestCase):
)
after_count = read_count(self.storage_server)
- after_bucket = read_bucket(self.storage_server, 0)
+ after_bucket = read_bucket(self.storage_server, num_passes)
self.expectThat(
after_count - before_count,
- Equals(expected),
+ Equals(1),
"Unexpected histogram sum value",
)
self.assertThat(
after_bucket - before_bucket,
- Equals(expected),
+ Equals(1),
"Unexpected histogram bucket value",
)
@@ -623,17 +617,17 @@ class PassValidationTests(TestCase):
When ZKAPs are spent to call *allocate_buckets* the number of passes spent
is recorded as a metric.
"""
- expected = required_passes(
+ num_passes = required_passes(
self.storage_server._pass_value, [size] * len(sharenums)
)
valid_passes = make_passes(
self.signing_key,
allocate_buckets_message(storage_index),
- list(RandomToken.create() for i in range(expected)),
+ list(RandomToken.create() for i in range(num_passes)),
)
before_count = read_count(self.storage_server)
- before_bucket = read_bucket(self.storage_server, size)
+ before_bucket = read_bucket(self.storage_server, num_passes)
alreadygot, allocated = self.storage_server.doRemoteCall(
"allocate_buckets",
@@ -650,16 +644,16 @@ class PassValidationTests(TestCase):
)
after_count = read_count(self.storage_server)
- after_bucket = read_bucket(self.storage_server, size)
+ after_bucket = read_bucket(self.storage_server, num_passes)
self.expectThat(
after_count - before_count,
- Equals(expected),
+ Equals(1),
"Unexpected histogram sum value",
)
self.assertThat(
after_bucket - before_bucket,
- Equals(expected),
+ Equals(1),
"Unexpected histogram bucket value",
)
@@ -689,17 +683,17 @@ class PassValidationTests(TestCase):
LocalReferenceable(None),
)
- expected = required_passes(
+ num_passes = required_passes(
self.storage_server._pass_value, [allocated_size] * len(sharenums)
)
valid_passes = make_passes(
self.signing_key,
add_lease_message(storage_index),
- list(RandomToken.create() for i in range(expected)),
+ list(RandomToken.create() for i in range(num_passes)),
)
before_count = read_count(self.storage_server)
- before_bucket = read_bucket(self.storage_server, allocated_size)
+ before_bucket = read_bucket(self.storage_server, num_passes)
self.storage_server.doRemoteCall(
"add_lease",
@@ -713,78 +707,18 @@ class PassValidationTests(TestCase):
)
after_count = read_count(self.storage_server)
- after_bucket = read_bucket(self.storage_server, allocated_size)
+ after_bucket = read_bucket(self.storage_server, num_passes)
self.expectThat(
after_count - before_count,
- Equals(expected),
+ Equals(1),
"Unexpected histogram sum value",
)
self.assertThat(
after_bucket - before_bucket,
- Equals(expected),
+ Equals(1),
"Unexpected histogram bucket value",
)
-class SpendingMetricTests(TestCase):
- @given(
- integers(min_value=1),
- lists(sizes()),
- )
- def test_total_passes_observed(self, bytes_per_pass, sizes):
- """
- The total number of spent passes reported by ``compute_spending_metrics``
- equals the total number of passes it is called with.
- """
- num_passes = required_passes(bytes_per_pass, sizes)
- observations = compute_spending_metrics(bytes_per_pass, sizes)
- total_recorded = sum(count for (size, count) in observations)
- self.assertThat(
- total_recorded,
- Equals(num_passes),
- "expected {} passes but metrics only accounted for {}".format(
- num_passes,
- total_recorded,
- ),
- )
-
- @given(
- integers(min_value=1),
- lists(sizes()),
- )
- def test_share_sizes_observed(self, bytes_per_pass, sizes):
- """
- Every size passed to ``compute_spending_metrics`` has a corresponding
- element in the result.
- """
- observations = compute_spending_metrics(bytes_per_pass, sizes)
- sizes_seen = list(size for (size, count) in observations)
- self.assertThat(
- sorted(sizes),
- Equals(sorted(sizes_seen)),
- )
-
- def test_observe_spending(self):
- """
- ``observe_spending_successes`` adds observations to a Prometheus histogram
- metric.
- """
- registry = CollectorRegistry()
- metric = Histogram(
- "zkapauthorizer_tests",
- "",
- registry=registry,
- buckets=[1, 2, 3, float("inf")],
- )
- observe_spending_successes(
- metric,
- iter([(1, 3), (2, 5), (3, 7), (4, 11), (3, 13), (5, 17)]),
- )
- self.assertThat(
- list(b.get() for b in metric._buckets),
- Equals([3, 5, 20, 28]),
- )
-
-
# Counter of invalid ZKAP spend attempts
--
GitLab