diff --git a/src/_zkapauthorizer/_storage_server.py b/src/_zkapauthorizer/_storage_server.py index 7d72edc7f21c655ce30671b97c8a20c5d0ba4ce5..c5ce9a5c50280b73ae383abe0b0dcbea2a6754c2 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 d80201e0314c39d1c467e34a0398095b7d1c68ef..c4db72a340c1dc80fe78304925f0271281bbcf52 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 3f637b1e9201317d64e314b275a42820aef9c193..1abecc5c7c7aac004e33980591b1983e07de4db0 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