From 5f23f2a15a0b0b1fa36a7b8b44ceb965c5de5b1f Mon Sep 17 00:00:00 2001 From: Ben McIlwain Date: Thu, 5 Sep 2024 15:41:20 -0400 Subject: [PATCH] Reduce cardinality of reserved list processing time metric (#2542) This single metric currently accounts for 22.2% of our total metrics bill, almost double the size of our EPP requests metric, while also simultaneously being much less useful. This change reduces the cardinality by removing two parameters we don't care that much about, which should significantly reduce the size and thus the cost. If after this change the metric is still too large, I'll also then remove the matchCount parameter from this metric. We could possibly even consider deleting the metric in its entirety, as we hardly ever use it. This PR also removes unused code for premium list metrics that have never actually been written out (and that we won't bother with at this point). --- .../model/tld/label/DomainLabelMetrics.java | 53 ++++--------------- .../model/tld/label/ReservedListTest.java | 16 +++--- docs/operational-procedures.md | 2 +- 3 files changed, 19 insertions(+), 52 deletions(-) diff --git a/core/src/main/java/google/registry/model/tld/label/DomainLabelMetrics.java b/core/src/main/java/google/registry/model/tld/label/DomainLabelMetrics.java index 058f3c40d..a8843c5de 100644 --- a/core/src/main/java/google/registry/model/tld/label/DomainLabelMetrics.java +++ b/core/src/main/java/google/registry/model/tld/label/DomainLabelMetrics.java @@ -50,8 +50,7 @@ class DomainLabelMetrics { } /** - * Labels attached to {@link #reservedListChecks} and {@link #reservedListProcessingTime} - * metrics. + * Labels attached to {@link #reservedListChecks} metrics. * *

A domain name can be matched by multiple reserved lists. To keep the metrics useful by * emitting only one metric result for each check, while avoiding potential combinatorial @@ -67,6 +66,13 @@ class DomainLabelMetrics { LabelDescriptor.create("most_severe_reserved_list", "Reserved list name, if any."), LabelDescriptor.create("most_severe_reservation_type", "Type of reservation found.")); + /** Labels attached to {@link #reservedListProcessingTime} metrics. */ + private static final ImmutableSet + RESERVED_LIST_PROCESSING_TIME_LABEL_DESCRIPTORS = + ImmutableSet.of( + LabelDescriptor.create("tld", "TLD"), + LabelDescriptor.create("reserved_list_count", "Number of matching reserved lists.")); + /** Labels attached to {@link #reservedListHits} metric. */ private static final ImmutableSet RESERVED_LIST_HIT_LABEL_DESCRIPTORS = ImmutableSet.of( @@ -74,15 +80,6 @@ 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 = @@ -101,7 +98,7 @@ class DomainLabelMetrics { "/domain_label/reserved/processing_time", "Reserved list check processing time", "milliseconds", - RESERVED_LIST_LABEL_DESCRIPTORS, + RESERVED_LIST_PROCESSING_TIME_LABEL_DESCRIPTORS, EventMetric.DEFAULT_FITTER); /** @@ -123,28 +120,6 @@ 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) { @@ -163,14 +138,6 @@ class DomainLabelMetrics { (matches.isEmpty() ? "(none)" : mostSevereMatch.reservationType()).toString(); reservedListChecks.increment( tld, matchCount, mostSevereReservedList, mostSevereReservationType); - 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()); + reservedListProcessingTime.record(elapsedMillis, tld, matchCount); } } diff --git a/core/src/test/java/google/registry/model/tld/label/ReservedListTest.java b/core/src/test/java/google/registry/model/tld/label/ReservedListTest.java index 06bd6f26d..7d3f38fa0 100644 --- a/core/src/test/java/google/registry/model/tld/label/ReservedListTest.java +++ b/core/src/test/java/google/registry/model/tld/label/ReservedListTest.java @@ -65,7 +65,7 @@ class ReservedListTest { .and() .hasNoOtherValues(); assertThat(reservedListProcessingTime) - .hasAnyValueForLabels("tld", "0", "(none)", "(none)") + .hasAnyValueForLabels("tld", "0") .and() .hasNoOtherValues(); assertThat(reservedListHits).hasNoOtherValues(); @@ -130,11 +130,11 @@ class ReservedListTest { .and() .hasNoOtherValues(); assertThat(reservedListProcessingTime) - .hasAnyValueForLabels("tld", "0", "(none)", "(none)") + .hasAnyValueForLabels("tld", "0") .and() - .hasAnyValueForLabels("tld", "1", "reserved1", FULLY_BLOCKED.toString()) + .hasAnyValueForLabels("tld", "1") .and() - .hasAnyValueForLabels("tld", "1", "reserved2", FULLY_BLOCKED.toString()) + .hasAnyValueForLabels("tld", "1") .and() .hasNoOtherValues(); assertThat(reservedListHits) @@ -182,9 +182,9 @@ class ReservedListTest { .and() .hasNoOtherValues(); assertThat(reservedListProcessingTime) - .hasAnyValueForLabels("tld", "1", "reserved2", FULLY_BLOCKED.toString()) + .hasAnyValueForLabels("tld", "1") .and() - .hasAnyValueForLabels("tld", "0", "(none)", "(none)") + .hasAnyValueForLabels("tld", "0") .and() .hasNoOtherValues(); assertThat(reservedListHits) @@ -209,9 +209,9 @@ class ReservedListTest { .and() .hasNoOtherValues(); assertThat(reservedListProcessingTime) - .hasAnyValueForLabels("tld", "1", "reserved1", ALLOWED_IN_SUNRISE.toString()) + .hasAnyValueForLabels("tld", "1") .and() - .hasAnyValueForLabels("tld", "2", "reserved2", FULLY_BLOCKED.toString()) + .hasAnyValueForLabels("tld", "2") .and() .hasNoOtherValues(); assertThat(reservedListHits) diff --git a/docs/operational-procedures.md b/docs/operational-procedures.md index 089951b1c..8ebab935a 100644 --- a/docs/operational-procedures.md +++ b/docs/operational-procedures.md @@ -20,7 +20,7 @@ metrics monitored are as follows: name, client id, and return status code. * `/custom/epp/processing_time` -- A [Distribution][distribution] representing the processing time for EPP requests, described by command name, client id, - and retujrn status code. + and return status code. * `/custom/whois/requests` -- A count of WHOIS requests, described by command name, number of returned results, and return status code. * `/custom/whois/processing_time` -- A [Distribution][distribution]