From 8cfb7483165bf765ad17bf4d94f2e7efb04b3e11 Mon Sep 17 00:00:00 2001 From: guyben Date: Wed, 16 Jan 2019 06:57:01 -0800 Subject: [PATCH] Remove TLD @Parameter from inside the DnsMetrics Currently, the TLD is a "hidden" part of the metric - we record it, but by looking at the call site you can't see that we record it. Also, it's injected from the query / POST parameter, so we might not even be aware of what the value is and it might not be the value we wanted. Instead, making it explicit in the Metric call. That way it's also more similar to the "logging" statements that record the same data but have to explicitly output the TLD. It also makes the tests better, as we test that we indeed record the correct TLD ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=229550115 --- java/google/registry/dns/DnsMetrics.java | 16 ++--- .../registry/dns/PublishDnsUpdatesAction.java | 15 ++--- .../dns/PublishDnsUpdatesActionTest.java | 64 +++++++++++-------- 3 files changed, 52 insertions(+), 43 deletions(-) diff --git a/java/google/registry/dns/DnsMetrics.java b/java/google/registry/dns/DnsMetrics.java index 64b501955..200b6aff9 100644 --- a/java/google/registry/dns/DnsMetrics.java +++ b/java/google/registry/dns/DnsMetrics.java @@ -14,8 +14,6 @@ package google.registry.dns; -import static google.registry.request.RequestParameters.PARAM_TLD; - import com.google.common.collect.ImmutableSet; import com.google.monitoring.metrics.DistributionFitter; import com.google.monitoring.metrics.EventMetric; @@ -25,7 +23,6 @@ import com.google.monitoring.metrics.IncrementableMetric; import com.google.monitoring.metrics.LabelDescriptor; import com.google.monitoring.metrics.MetricRegistryImpl; import google.registry.config.RegistryEnvironment; -import google.registry.request.Parameter; import javax.inject.Inject; import org.joda.time.Duration; @@ -184,7 +181,6 @@ public class DnsMetrics { EXPONENTIAL_FITTER); @Inject RegistryEnvironment registryEnvironment; - @Inject @Parameter(PARAM_TLD) String tld; @Inject DnsMetrics() {} @@ -193,7 +189,7 @@ public class DnsMetrics { * Increment a monotonic counter that tracks calls to {@link * google.registry.dns.writer.DnsWriter#publishDomain(String)}, per TLD. */ - public void incrementPublishDomainRequests(long numRequests, PublishStatus status) { + public void incrementPublishDomainRequests(String tld, long numRequests, PublishStatus status) { if (numRequests > 0) { publishDomainRequests.incrementBy(numRequests, tld, status.name()); } @@ -203,7 +199,7 @@ public class DnsMetrics { * Increment a monotonic counter that tracks calls to {@link * google.registry.dns.writer.DnsWriter#publishHost(String)}, per TLD. */ - public void incrementPublishHostRequests(long numRequests, PublishStatus status) { + public void incrementPublishHostRequests(String tld, long numRequests, PublishStatus status) { if (numRequests > 0) { publishHostRequests.incrementBy(numRequests, tld, status.name()); } @@ -212,12 +208,13 @@ public class DnsMetrics { /** * Measures information about the entire batched commit, per TLD. * - * The information includes running times (per item and per commit), and batch sizes (per item and - * per commit) + *

The information includes running times (per item and per commit), and batch sizes (per item + * and per commit) * - * This is to be used for load testing the system, and will not measure anything in prod. + *

This is to be used for load testing the system, and will not measure anything in prod. */ void recordCommit( + String tld, String dnsWriter, CommitStatus status, Duration processingDuration, @@ -254,6 +251,7 @@ public class DnsMetrics { } void recordActionResult( + String tld, String dnsWriter, ActionStatus status, int numberOfItems, diff --git a/java/google/registry/dns/PublishDnsUpdatesAction.java b/java/google/registry/dns/PublishDnsUpdatesAction.java index 76b7cfe21..f118cd187 100644 --- a/java/google/registry/dns/PublishDnsUpdatesAction.java +++ b/java/google/registry/dns/PublishDnsUpdatesAction.java @@ -96,6 +96,7 @@ public final class PublishDnsUpdatesAction implements Runnable, Callable { DateTime now = clock.nowUtc(); dnsMetrics.recordActionResult( + tld, dnsWriter, status, nullToEmpty(domains).size() + nullToEmpty(hosts).size(), @@ -197,8 +198,8 @@ public final class PublishDnsUpdatesAction implements Runnable, Callable { domainsPublished += 1; } } - dnsMetrics.incrementPublishDomainRequests(domainsPublished, PublishStatus.ACCEPTED); - dnsMetrics.incrementPublishDomainRequests(domainsRejected, PublishStatus.REJECTED); + dnsMetrics.incrementPublishDomainRequests(tld, domainsPublished, PublishStatus.ACCEPTED); + dnsMetrics.incrementPublishDomainRequests(tld, domainsRejected, PublishStatus.REJECTED); int hostsPublished = 0; int hostsRejected = 0; @@ -213,8 +214,8 @@ public final class PublishDnsUpdatesAction implements Runnable, Callable { hostsPublished += 1; } } - dnsMetrics.incrementPublishHostRequests(hostsPublished, PublishStatus.ACCEPTED); - dnsMetrics.incrementPublishHostRequests(hostsRejected, PublishStatus.REJECTED); + dnsMetrics.incrementPublishHostRequests(tld, hostsPublished, PublishStatus.ACCEPTED); + dnsMetrics.incrementPublishHostRequests(tld, hostsRejected, PublishStatus.REJECTED); // If we got here it means we managed to stage the entire batch without any errors. // Next we will commit the batch. @@ -229,11 +230,7 @@ public final class PublishDnsUpdatesAction implements Runnable, Callable { recordActionResult(actionStatus); Duration duration = new Duration(timeAtStart, clock.nowUtc()); dnsMetrics.recordCommit( - dnsWriter, - commitStatus, - duration, - domainsPublished, - hostsPublished); + tld, dnsWriter, commitStatus, duration, domainsPublished, hostsPublished); logger.atInfo().log( "writer.commit() statistics: TLD: %s, dnsWriter: %s, commitStatus: %s, duration: %s, " + "domainsPublished: %d, domainsRejected: %d, hostsPublished: %d, hostsRejected: %d", diff --git a/javatests/google/registry/dns/PublishDnsUpdatesActionTest.java b/javatests/google/registry/dns/PublishDnsUpdatesActionTest.java index 00ef25b35..0de352653 100644 --- a/javatests/google/registry/dns/PublishDnsUpdatesActionTest.java +++ b/javatests/google/registry/dns/PublishDnsUpdatesActionTest.java @@ -115,13 +115,15 @@ public class PublishDnsUpdatesActionTest { verify(dnsWriter).publishHost("ns1.example.xn--q9jyb4c"); verify(dnsWriter).commit(); verifyNoMoreInteractions(dnsWriter); - verify(dnsMetrics).incrementPublishDomainRequests(0, PublishStatus.ACCEPTED); - verify(dnsMetrics).incrementPublishDomainRequests(0, PublishStatus.REJECTED); - verify(dnsMetrics).incrementPublishHostRequests(1, PublishStatus.ACCEPTED); - verify(dnsMetrics).incrementPublishHostRequests(0, PublishStatus.REJECTED); - verify(dnsMetrics).recordCommit("correctWriter", CommitStatus.SUCCESS, Duration.ZERO, 0, 1); + verify(dnsMetrics).incrementPublishDomainRequests("xn--q9jyb4c", 0, PublishStatus.ACCEPTED); + verify(dnsMetrics).incrementPublishDomainRequests("xn--q9jyb4c", 0, PublishStatus.REJECTED); + verify(dnsMetrics).incrementPublishHostRequests("xn--q9jyb4c", 1, PublishStatus.ACCEPTED); + verify(dnsMetrics).incrementPublishHostRequests("xn--q9jyb4c", 0, PublishStatus.REJECTED); + verify(dnsMetrics) + .recordCommit("xn--q9jyb4c", "correctWriter", CommitStatus.SUCCESS, Duration.ZERO, 0, 1); verify(dnsMetrics) .recordActionResult( + "xn--q9jyb4c", "correctWriter", ActionStatus.SUCCESS, 1, @@ -141,13 +143,15 @@ public class PublishDnsUpdatesActionTest { verify(dnsWriter).publishDomain("example.xn--q9jyb4c"); verify(dnsWriter).commit(); verifyNoMoreInteractions(dnsWriter); - verify(dnsMetrics).incrementPublishDomainRequests(1, PublishStatus.ACCEPTED); - verify(dnsMetrics).incrementPublishDomainRequests(0, PublishStatus.REJECTED); - verify(dnsMetrics).incrementPublishHostRequests(0, PublishStatus.ACCEPTED); - verify(dnsMetrics).incrementPublishHostRequests(0, PublishStatus.REJECTED); - verify(dnsMetrics).recordCommit("correctWriter", CommitStatus.SUCCESS, Duration.ZERO, 1, 0); + verify(dnsMetrics).incrementPublishDomainRequests("xn--q9jyb4c", 1, PublishStatus.ACCEPTED); + verify(dnsMetrics).incrementPublishDomainRequests("xn--q9jyb4c", 0, PublishStatus.REJECTED); + verify(dnsMetrics).incrementPublishHostRequests("xn--q9jyb4c", 0, PublishStatus.ACCEPTED); + verify(dnsMetrics).incrementPublishHostRequests("xn--q9jyb4c", 0, PublishStatus.REJECTED); + verify(dnsMetrics) + .recordCommit("xn--q9jyb4c", "correctWriter", CommitStatus.SUCCESS, Duration.ZERO, 1, 0); verify(dnsMetrics) .recordActionResult( + "xn--q9jyb4c", "correctWriter", ActionStatus.SUCCESS, 1, @@ -186,13 +190,15 @@ public class PublishDnsUpdatesActionTest { assertThrows(RuntimeException.class, action::run); - verify(dnsMetrics).incrementPublishDomainRequests(2, PublishStatus.ACCEPTED); - verify(dnsMetrics).incrementPublishDomainRequests(0, PublishStatus.REJECTED); - verify(dnsMetrics).incrementPublishHostRequests(3, PublishStatus.ACCEPTED); - verify(dnsMetrics).incrementPublishHostRequests(0, PublishStatus.REJECTED); - verify(dnsMetrics).recordCommit("correctWriter", CommitStatus.FAILURE, Duration.ZERO, 2, 3); + verify(dnsMetrics).incrementPublishDomainRequests("xn--q9jyb4c", 2, PublishStatus.ACCEPTED); + verify(dnsMetrics).incrementPublishDomainRequests("xn--q9jyb4c", 0, PublishStatus.REJECTED); + verify(dnsMetrics).incrementPublishHostRequests("xn--q9jyb4c", 3, PublishStatus.ACCEPTED); + verify(dnsMetrics).incrementPublishHostRequests("xn--q9jyb4c", 0, PublishStatus.REJECTED); + verify(dnsMetrics) + .recordCommit("xn--q9jyb4c", "correctWriter", CommitStatus.FAILURE, Duration.ZERO, 2, 3); verify(dnsMetrics) .recordActionResult( + "xn--q9jyb4c", "correctWriter", ActionStatus.COMMIT_FAILURE, 5, @@ -218,13 +224,15 @@ public class PublishDnsUpdatesActionTest { verify(dnsWriter).publishHost("ns1.example2.xn--q9jyb4c"); verify(dnsWriter).commit(); verifyNoMoreInteractions(dnsWriter); - verify(dnsMetrics).incrementPublishDomainRequests(2, PublishStatus.ACCEPTED); - verify(dnsMetrics).incrementPublishDomainRequests(0, PublishStatus.REJECTED); - verify(dnsMetrics).incrementPublishHostRequests(3, PublishStatus.ACCEPTED); - verify(dnsMetrics).incrementPublishHostRequests(0, PublishStatus.REJECTED); - verify(dnsMetrics).recordCommit("correctWriter", CommitStatus.SUCCESS, Duration.ZERO, 2, 3); + verify(dnsMetrics).incrementPublishDomainRequests("xn--q9jyb4c", 2, PublishStatus.ACCEPTED); + verify(dnsMetrics).incrementPublishDomainRequests("xn--q9jyb4c", 0, PublishStatus.REJECTED); + verify(dnsMetrics).incrementPublishHostRequests("xn--q9jyb4c", 3, PublishStatus.ACCEPTED); + verify(dnsMetrics).incrementPublishHostRequests("xn--q9jyb4c", 0, PublishStatus.REJECTED); + verify(dnsMetrics) + .recordCommit("xn--q9jyb4c", "correctWriter", CommitStatus.SUCCESS, Duration.ZERO, 2, 3); verify(dnsMetrics) .recordActionResult( + "xn--q9jyb4c", "correctWriter", ActionStatus.SUCCESS, 5, @@ -244,13 +252,15 @@ public class PublishDnsUpdatesActionTest { verify(dnsWriter).commit(); verifyNoMoreInteractions(dnsWriter); - verify(dnsMetrics).incrementPublishDomainRequests(0, PublishStatus.ACCEPTED); - verify(dnsMetrics).incrementPublishDomainRequests(2, PublishStatus.REJECTED); - verify(dnsMetrics).incrementPublishHostRequests(0, PublishStatus.ACCEPTED); - verify(dnsMetrics).incrementPublishHostRequests(3, PublishStatus.REJECTED); - verify(dnsMetrics).recordCommit("correctWriter", CommitStatus.SUCCESS, Duration.ZERO, 0, 0); + verify(dnsMetrics).incrementPublishDomainRequests("xn--q9jyb4c", 0, PublishStatus.ACCEPTED); + verify(dnsMetrics).incrementPublishDomainRequests("xn--q9jyb4c", 2, PublishStatus.REJECTED); + verify(dnsMetrics).incrementPublishHostRequests("xn--q9jyb4c", 0, PublishStatus.ACCEPTED); + verify(dnsMetrics).incrementPublishHostRequests("xn--q9jyb4c", 3, PublishStatus.REJECTED); + verify(dnsMetrics) + .recordCommit("xn--q9jyb4c", "correctWriter", CommitStatus.SUCCESS, Duration.ZERO, 0, 0); verify(dnsMetrics) .recordActionResult( + "xn--q9jyb4c", "correctWriter", ActionStatus.SUCCESS, 5, @@ -274,6 +284,7 @@ public class PublishDnsUpdatesActionTest { verifyNoMoreInteractions(dnsWriter); verify(dnsMetrics) .recordActionResult( + "xn--q9jyb4c", "correctWriter", ActionStatus.LOCK_FAILURE, 5, @@ -297,6 +308,7 @@ public class PublishDnsUpdatesActionTest { verifyNoMoreInteractions(dnsWriter); verify(dnsMetrics) .recordActionResult( + "xn--q9jyb4c", "correctWriter", ActionStatus.BAD_LOCK_INDEX, 2, @@ -322,6 +334,7 @@ public class PublishDnsUpdatesActionTest { verifyNoMoreInteractions(dnsWriter); verify(dnsMetrics) .recordActionResult( + "xn--q9jyb4c", "correctWriter", ActionStatus.BAD_LOCK_INDEX, 2, @@ -345,6 +358,7 @@ public class PublishDnsUpdatesActionTest { verifyNoMoreInteractions(dnsWriter); verify(dnsMetrics) .recordActionResult( + "xn--q9jyb4c", "wrongWriter", ActionStatus.BAD_WRITER, 5,