diff --git a/java/google/registry/model/registry/label/DomainLabelMetrics.java b/java/google/registry/model/registry/label/DomainLabelMetrics.java index b2fdf056c..7b15bd9a9 100644 --- a/java/google/registry/model/registry/label/DomainLabelMetrics.java +++ b/java/google/registry/model/registry/label/DomainLabelMetrics.java @@ -25,6 +25,24 @@ import google.registry.monitoring.metrics.MetricRegistryImpl; /** Instrumentation for reserved lists. */ class DomainLabelMetrics { + /** Possible premium list check outcomes. */ + enum PremiumListCheckOutcome { + /** Bloom filter knows it is not premium */ + BLOOM_FILTER_NEGATIVE, + + /** Bloom filter thinks it might be premium, but it was already negatively cached */ + CACHED_NEGATIVE, + + /** Bloom filter thinks it might be premium, and it is, and was already in the cache */ + CACHED_POSITIVE, + + /** Bloom filter thinks it might be premium, but it is not (though wasn't in the cache) */ + UNCACHED_NEGATIVE, + + /** Bloom filter thinks it might be premium, and it is, but wasn't in the cache */ + UNCACHED_POSITIVE + } + @AutoValue abstract static class MetricsReservedListMatch { static MetricsReservedListMatch create( @@ -62,6 +80,15 @@ class DomainLabelMetrics { LabelDescriptor.create("reserved_list", "Reserved list name."), LabelDescriptor.create("reservation_type", "Type of reservation found.")); + /** + * Labels attached to {@link #premiumListChecks} and {@link #premiumListProcessingTime} metrics. + */ + private static final ImmutableSet PREMIUM_LIST_LABEL_DESCRIPTORS = + ImmutableSet.of( + LabelDescriptor.create("tld", "TLD"), + LabelDescriptor.create("premium_list", "Premium list name."), + LabelDescriptor.create("outcome", "Outcome of the premium list check.")); + /** Metric counting the number of times a label was checked against all reserved lists. */ @VisibleForTesting static final IncrementableMetric reservedListChecks = @@ -102,6 +129,28 @@ class DomainLabelMetrics { "count", RESERVED_LIST_HIT_LABEL_DESCRIPTORS); + + /** Metric recording the result of each premium list check. */ + @VisibleForTesting + static final IncrementableMetric premiumListChecks = + MetricRegistryImpl.getDefault() + .newIncrementableMetric( + "/domain_label/premium/checks", + "Count of premium list checks", + "count", + PREMIUM_LIST_LABEL_DESCRIPTORS); + + /** Metric recording the time required to process each premium list check. */ + @VisibleForTesting + static final EventMetric premiumListProcessingTime = + MetricRegistryImpl.getDefault() + .newEventMetric( + "/domain_label/premium/processing_time", + "Premium list check processing time", + "milliseconds", + PREMIUM_LIST_LABEL_DESCRIPTORS, + EventMetric.DEFAULT_FITTER); + /** Update all three reserved list metrics. */ static void recordReservedListCheckOutcome( String tld, ImmutableSet matches, double elapsedMillis) { @@ -124,4 +173,11 @@ class DomainLabelMetrics { reservedListProcessingTime.record( elapsedMillis, tld, matchCount, mostSevereReservedList, mostSevereReservationType); } + + /** Update both premium list metrics. */ + static void recordPremiumListCheckOutcome( + String tld, String premiumList, PremiumListCheckOutcome outcome, double elapsedMillis) { + premiumListChecks.increment(tld, premiumList, outcome.name()); + premiumListProcessingTime.record(elapsedMillis, tld, premiumList, outcome.name()); + } } diff --git a/java/google/registry/model/registry/label/PremiumList.java b/java/google/registry/model/registry/label/PremiumList.java index d156756fd..720efc2c2 100644 --- a/java/google/registry/model/registry/label/PremiumList.java +++ b/java/google/registry/model/registry/label/PremiumList.java @@ -43,6 +43,7 @@ import google.registry.model.Buildable; import google.registry.model.ImmutableObject; import google.registry.model.annotations.ReportedOn; import google.registry.model.registry.Registry; +import google.registry.util.NonFinalForTesting; import java.io.ByteArrayOutputStream; import java.io.IOException; import java.util.List; @@ -83,7 +84,17 @@ public final class PremiumList extends BaseDomainLabelList probablePremiumLabels; + private BloomFilter probablePremiumLabels; + + /** + * Get the bloom filter. + * + *

Note that this is not a copy, but the mutable object itself, because copying would be + * expensive. You probably should not modify the filter unless you know what you're doing. + */ + public BloomFilter getProbablePremiumLabels() { + return probablePremiumLabels; + } /** * The maximum size of the bloom filter. @@ -182,24 +193,32 @@ public final class PremiumList extends BaseDomainLabelList, Optional> - cachePremiumListEntries = - CacheBuilder.newBuilder() - .expireAfterWrite(getSingletonCachePersistDuration().getMillis(), MILLISECONDS) - .maximumSize(getStaticPremiumListMaxCachedEntries()) - .build( - new CacheLoader, Optional>() { - @Override - public Optional load(final Key entryKey) { - return ofy() - .doTransactionless( - new Work>() { - @Override - public Optional run() { - return Optional.fromNullable(ofy().load().key(entryKey).now()); - }}); - }}); + static LoadingCache, Optional> cachePremiumListEntries = + createCachePremiumListEntries(getSingletonCachePersistDuration().getMillis()); + + @VisibleForTesting + static LoadingCache, Optional> + createCachePremiumListEntries(long cachePersistDurationMillis) { + return CacheBuilder.newBuilder() + .expireAfterWrite(cachePersistDurationMillis, MILLISECONDS) + .maximumSize(getStaticPremiumListMaxCachedEntries()) + .build( + new CacheLoader, Optional>() { + @Override + public Optional load(final Key entryKey) { + return ofy() + .doTransactionless( + new Work>() { + @Override + public Optional run() { + return Optional.fromNullable(ofy().load().key(entryKey).now()); + } + }); + } + }); + } @VisibleForTesting public Key getRevisionKey() { diff --git a/java/google/registry/model/registry/label/PremiumListUtils.java b/java/google/registry/model/registry/label/PremiumListUtils.java index 0cfd0fc9f..76c792f5d 100644 --- a/java/google/registry/model/registry/label/PremiumListUtils.java +++ b/java/google/registry/model/registry/label/PremiumListUtils.java @@ -18,10 +18,17 @@ import static com.google.common.base.Preconditions.checkState; import static com.google.common.collect.Iterables.partition; import static google.registry.model.common.EntityGroupRoot.getCrossTldKey; import static google.registry.model.ofy.ObjectifyService.ofy; +import static google.registry.model.registry.label.DomainLabelMetrics.PremiumListCheckOutcome.BLOOM_FILTER_NEGATIVE; +import static google.registry.model.registry.label.DomainLabelMetrics.PremiumListCheckOutcome.CACHED_NEGATIVE; +import static google.registry.model.registry.label.DomainLabelMetrics.PremiumListCheckOutcome.CACHED_POSITIVE; +import static google.registry.model.registry.label.DomainLabelMetrics.PremiumListCheckOutcome.UNCACHED_NEGATIVE; +import static google.registry.model.registry.label.DomainLabelMetrics.PremiumListCheckOutcome.UNCACHED_POSITIVE; import static google.registry.model.registry.label.PremiumList.cachePremiumListEntries; import static google.registry.model.registry.label.PremiumList.cachePremiumListRevisions; import static google.registry.model.registry.label.PremiumList.cachePremiumLists; +import static org.joda.time.DateTimeZone.UTC; +import com.google.auto.value.AutoValue; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Function; import com.google.common.base.Optional; @@ -33,6 +40,7 @@ import com.googlecode.objectify.Key; import com.googlecode.objectify.VoidWork; import com.googlecode.objectify.Work; import google.registry.model.registry.Registry; +import google.registry.model.registry.label.DomainLabelMetrics.PremiumListCheckOutcome; import google.registry.model.registry.label.PremiumList.PremiumListEntry; import google.registry.model.registry.label.PremiumList.PremiumListRevision; import java.util.List; @@ -47,6 +55,17 @@ public final class PremiumListUtils { /** The number of premium list entry entities that are created and deleted per batch. */ static final int TRANSACTION_BATCH_SIZE = 200; + /** Value type class used by {@link #checkStatus} to return the results of a premiumness check. */ + @AutoValue + abstract static class CheckResults { + static CheckResults create(PremiumListCheckOutcome checkOutcome, Optional premiumPrice) { + return new AutoValue_PremiumListUtils_CheckResults(checkOutcome, premiumPrice); + } + + abstract PremiumListCheckOutcome checkOutcome(); + abstract Optional premiumPrice(); + } + /** * Returns the premium price for the specified label and registry, or absent if the label is not * premium. @@ -54,8 +73,9 @@ public final class PremiumListUtils { public static Optional getPremiumPrice(String label, Registry registry) { // If the registry has no configured premium list, then no labels are premium. if (registry.getPremiumList() == null) { - return Optional. absent(); + return Optional.absent(); } + DateTime startTime = DateTime.now(UTC); String listName = registry.getPremiumList().getName(); Optional optionalPremiumList = PremiumList.get(listName); checkState(optionalPremiumList.isPresent(), "Could not load premium list '%s'", listName); @@ -68,21 +88,45 @@ public final class PremiumListUtils { "Could not load premium list revision " + premiumList.getRevisionKey(), e); } checkState( - revision.probablePremiumLabels != null, + revision.getProbablePremiumLabels() != null, "Probable premium labels bloom filter is null on revision '%s'", premiumList.getRevisionKey()); - if (revision.probablePremiumLabels.mightContain(label)) { - Key entryKey = - Key.create(premiumList.getRevisionKey(), PremiumListEntry.class, label); - try { - Optional entry = cachePremiumListEntries.get(entryKey); - return (entry.isPresent()) ? Optional.of(entry.get().getValue()) : Optional.absent(); - } catch (InvalidCacheLoadException | ExecutionException e) { - throw new RuntimeException("Could not load premium list entry " + entryKey, e); + CheckResults checkResults = checkStatus(revision, label); + DomainLabelMetrics.recordPremiumListCheckOutcome( + registry.getTldStr(), + listName, + checkResults.checkOutcome(), + DateTime.now(UTC).getMillis() - startTime.getMillis()); + return checkResults.premiumPrice(); + } + + private static CheckResults checkStatus(PremiumListRevision premiumListRevision, String label) { + if (!premiumListRevision.getProbablePremiumLabels().mightContain(label)) { + return CheckResults.create(BLOOM_FILTER_NEGATIVE, Optional.absent()); + } + + Key entryKey = + Key.create(Key.create(premiumListRevision), PremiumListEntry.class, label); + try { + // getIfPresent() returns null if the key is not in the cache + Optional entry = cachePremiumListEntries.getIfPresent(entryKey); + if (entry != null) { + if (entry.isPresent()) { + return CheckResults.create(CACHED_POSITIVE, Optional.of(entry.get().getValue())); + } else { + return CheckResults.create(CACHED_NEGATIVE, Optional.absent()); + } } - } else { - return Optional.absent(); + + entry = cachePremiumListEntries.get(entryKey); + if (entry.isPresent()) { + return CheckResults.create(UNCACHED_POSITIVE, Optional.of(entry.get().getValue())); + } else { + return CheckResults.create(UNCACHED_NEGATIVE, Optional.absent()); + } + } catch (InvalidCacheLoadException | ExecutionException e) { + throw new RuntimeException("Could not load premium list entry " + entryKey, e); } } diff --git a/javatests/google/registry/model/registry/label/PremiumListTest.java b/javatests/google/registry/model/registry/label/PremiumListTest.java index 63eb5bf04..eb0aac57e 100644 --- a/javatests/google/registry/model/registry/label/PremiumListTest.java +++ b/javatests/google/registry/model/registry/label/PremiumListTest.java @@ -73,10 +73,10 @@ public class PremiumListTest { public void testProbablePremiumLabels() throws Exception { PremiumList pl = PremiumList.get("tld").get(); PremiumListRevision revision = ofy().load().key(pl.getRevisionKey()).now(); - assertThat(revision.probablePremiumLabels.mightContain("notpremium")).isFalse(); + assertThat(revision.getProbablePremiumLabels().mightContain("notpremium")).isFalse(); for (String label : ImmutableList.of("rich", "lol", "johnny-be-goode", "icann")) { assertWithMessage(label + " should be a probable premium") - .that(revision.probablePremiumLabels.mightContain(label)) + .that(revision.getProbablePremiumLabels().mightContain(label)) .isTrue(); } } diff --git a/javatests/google/registry/model/registry/label/PremiumListUtilsTest.java b/javatests/google/registry/model/registry/label/PremiumListUtilsTest.java index 962254f3c..a066d9974 100644 --- a/javatests/google/registry/model/registry/label/PremiumListUtilsTest.java +++ b/javatests/google/registry/model/registry/label/PremiumListUtilsTest.java @@ -16,11 +16,19 @@ package google.registry.model.registry.label; import static com.google.common.truth.Truth.assertThat; import static google.registry.model.ofy.ObjectifyService.ofy; -import static google.registry.model.registry.label.PremiumList.cachePremiumListEntries; +import static google.registry.model.registry.label.DomainLabelMetrics.PremiumListCheckOutcome.BLOOM_FILTER_NEGATIVE; +import static google.registry.model.registry.label.DomainLabelMetrics.PremiumListCheckOutcome.CACHED_NEGATIVE; +import static google.registry.model.registry.label.DomainLabelMetrics.PremiumListCheckOutcome.CACHED_POSITIVE; +import static google.registry.model.registry.label.DomainLabelMetrics.PremiumListCheckOutcome.UNCACHED_NEGATIVE; +import static google.registry.model.registry.label.DomainLabelMetrics.PremiumListCheckOutcome.UNCACHED_POSITIVE; +import static google.registry.model.registry.label.DomainLabelMetrics.premiumListChecks; +import static google.registry.model.registry.label.DomainLabelMetrics.premiumListProcessingTime; import static google.registry.model.registry.label.PremiumListUtils.deletePremiumList; import static google.registry.model.registry.label.PremiumListUtils.doesPremiumListExist; import static google.registry.model.registry.label.PremiumListUtils.getPremiumPrice; import static google.registry.model.registry.label.PremiumListUtils.savePremiumListAndEntries; +import static google.registry.monitoring.metrics.contrib.EventMetricSubject.assertThat; +import static google.registry.monitoring.metrics.contrib.IncrementableMetricSubject.assertThat; import static google.registry.testing.DatastoreHelper.createTld; import static google.registry.testing.DatastoreHelper.loadPremiumListEntries; import static google.registry.testing.DatastoreHelper.persistPremiumList; @@ -29,6 +37,7 @@ import static google.registry.testing.DatastoreHelper.persistResource; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.googlecode.objectify.Key; +import com.googlecode.objectify.VoidWork; import google.registry.model.pricing.StaticPremiumListPricingEngine; import google.registry.model.registry.Registry; import google.registry.model.registry.label.PremiumList.PremiumListEntry; @@ -53,6 +62,7 @@ public class PremiumListUtilsTest { @Before public void before() throws Exception { // createTld() overwrites the premium list, so call it first. + PremiumList.cachePremiumListEntries = PremiumList.createCachePremiumListEntries(60); createTld("tld"); PremiumList pl = persistPremiumList( @@ -62,6 +72,20 @@ public class PremiumListUtilsTest { "icann,JPY 100", "johnny-be-goode,USD 20.50"); persistResource(Registry.get("tld").asBuilder().setPremiumList(pl).build()); + premiumListChecks.reset(); + premiumListProcessingTime.reset(); + } + + void assertMetricOutcomeCount(int checkCount, DomainLabelMetrics.PremiumListCheckOutcome outcome) + throws Exception { + assertThat(premiumListChecks) + .hasValueForLabels(checkCount, "tld", "tld", outcome.toString()) + .and() + .hasNoOtherValues(); + assertThat(premiumListProcessingTime) + .hasAnyValueForLabels("tld", "tld", outcome.toString()) + .and() + .hasNoOtherValues(); } @Test @@ -74,6 +98,8 @@ public class PremiumListUtilsTest { .build()); assertThat(Registry.get("ghost").getPremiumList()).isNull(); assertThat(getPremiumPrice("blah", Registry.get("ghost"))).isAbsent(); + assertThat(premiumListChecks).hasNoOtherValues(); + assertThat(premiumListProcessingTime).hasNoOtherValues(); } @Test @@ -89,6 +115,7 @@ public class PremiumListUtilsTest { PremiumList premiumList = persistHumongousPremiumList("tld", 2500); assertThat(loadPremiumListEntries(premiumList)).hasSize(2500); assertThat(getPremiumPrice("7", Registry.get("tld"))).hasValue(Money.parse("USD 100")); + assertMetricOutcomeCount(1, UNCACHED_POSITIVE); } @Test @@ -131,11 +158,57 @@ public class PremiumListUtilsTest { assertThat(getPremiumPrice("missingno", Registry.get("tld"))).isAbsent(); // However, if we manually query the cache to force an entity load, it should be found. assertThat( - cachePremiumListEntries.get( + PremiumList.cachePremiumListEntries.get( Key.create(pl.getRevisionKey(), PremiumListEntry.class, "missingno"))) .hasValue(entry); + assertMetricOutcomeCount(1, BLOOM_FILTER_NEGATIVE); } + @Test + public void testGetPremiumPrice_cachedSecondTime() throws Exception { + assertThat(getPremiumPrice("rich", Registry.get("tld"))).hasValue(Money.parse("USD 1999")); + assertThat(getPremiumPrice("rich", Registry.get("tld"))).hasValue(Money.parse("USD 1999")); + assertThat(premiumListChecks) + .hasValueForLabels(1, "tld", "tld", UNCACHED_POSITIVE.toString()) + .and() + .hasValueForLabels(1, "tld", "tld", CACHED_POSITIVE.toString()) + .and() + .hasNoOtherValues(); + assertThat(premiumListProcessingTime) + .hasAnyValueForLabels("tld", "tld", UNCACHED_POSITIVE.toString()) + .and() + .hasAnyValueForLabels("tld", "tld", CACHED_POSITIVE.toString()) + .and() + .hasNoOtherValues(); + } + + @Test + public void testGetPremiumPrice_bloomFilterFalsePositive() throws Exception { + // Remove one of the premium list entries from behind the Bloom filter's back. + PremiumList pl = PremiumList.get("tld").get(); + ofy().transactNew(new VoidWork() { + @Override + public void vrun() { + ofy().delete().keys(Key.create(pl.getRevisionKey(), PremiumListEntry.class, "rich")); + }}); + ofy().clearSessionCache(); + + assertThat(getPremiumPrice("rich", Registry.get("tld"))).isAbsent(); + assertThat(getPremiumPrice("rich", Registry.get("tld"))).isAbsent(); + + assertThat(premiumListChecks) + .hasValueForLabels(1, "tld", "tld", UNCACHED_NEGATIVE.toString()) + .and() + .hasValueForLabels(1, "tld", "tld", CACHED_NEGATIVE.toString()) + .and() + .hasNoOtherValues(); + assertThat(premiumListProcessingTime) + .hasAnyValueForLabels("tld", "tld", UNCACHED_NEGATIVE.toString()) + .and() + .hasAnyValueForLabels("tld", "tld", CACHED_NEGATIVE.toString()) + .and() + .hasNoOtherValues(); + } @Test public void testSave_removedPremiumListEntries_areNoLongerInDatastore() throws Exception { @@ -170,12 +243,25 @@ public class PremiumListUtilsTest { .id("dolt") .now()) .isNull(); + assertThat(premiumListChecks) + .hasValueForLabels(4, "tld", "tld", UNCACHED_POSITIVE.toString()) + .and() + .hasValueForLabels(1, "tld", "tld", BLOOM_FILTER_NEGATIVE.toString()) + .and() + .hasNoOtherValues(); + assertThat(premiumListProcessingTime) + .hasAnyValueForLabels("tld", "tld", UNCACHED_POSITIVE.toString()) + .and() + .hasAnyValueForLabels("tld", "tld", BLOOM_FILTER_NEGATIVE.toString()) + .and() + .hasNoOtherValues(); } @Test public void testGetPremiumPrice_allLabelsAreNonPremium_whenNotInList() throws Exception { assertThat(getPremiumPrice("blah", Registry.get("tld"))).isAbsent(); assertThat(getPremiumPrice("slinge", Registry.get("tld"))).isAbsent(); + assertMetricOutcomeCount(2, BLOOM_FILTER_NEGATIVE); } @Test @@ -196,6 +282,18 @@ public class PremiumListUtilsTest { assertThat(entry.comment).isEqualTo("yupper rooni"); assertThat(entry.price).isEqualTo(Money.parse("USD 999")); assertThat(entry.label).isEqualTo("lol"); + assertThat(premiumListChecks) + .hasValueForLabels(1, "tld", "tld2", UNCACHED_POSITIVE.toString()) + .and() + .hasValueForLabels(1, "tld", "tld2", BLOOM_FILTER_NEGATIVE.toString()) + .and() + .hasNoOtherValues(); + assertThat(premiumListProcessingTime) + .hasAnyValueForLabels("tld", "tld2", UNCACHED_POSITIVE.toString()) + .and() + .hasAnyValueForLabels("tld", "tld2", BLOOM_FILTER_NEGATIVE.toString()) + .and() + .hasNoOtherValues(); } @Test