From 302a27f0dbc6de844b27e7505f065c44a4b65301 Mon Sep 17 00:00:00 2001 From: mcilwain Date: Wed, 4 Oct 2017 14:26:20 -0700 Subject: [PATCH] Record a version of EPP metrics with TLD for domain commands Also fixes the issue that dry run EPP commands were incorrectly being reported on. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=171062984 --- java/google/registry/flows/EppController.java | 14 ++-- java/google/registry/flows/EppMetrics.java | 65 ++++++++++++++----- java/google/registry/flows/FlowReporter.java | 39 ++++++----- .../registry/model/eppinput/EppInput.java | 5 ++ .../registry/monitoring/metrics/Counter.java | 5 ++ .../metrics/MetricRegistryImpl.java | 20 ++++++ .../monitoring/whitebox/EppMetric.java | 38 +++++++++++ .../registry/flows/EppControllerTest.java | 63 +++++++++++++++--- .../flows/EppLifecycleContactTest.java | 22 ++++--- .../flows/EppLifecycleDomainTest.java | 6 ++ .../registry/flows/FlowReporterTest.java | 10 +++ .../monitoring/whitebox/EppMetricTest.java | 46 ++++++++++++- .../registry/testing/EppMetricSubject.java | 11 ++++ 13 files changed, 283 insertions(+), 61 deletions(-) diff --git a/java/google/registry/flows/EppController.java b/java/google/registry/flows/EppController.java index 4f62cc0fa..d42a29867 100644 --- a/java/google/registry/flows/EppController.java +++ b/java/google/registry/flows/EppController.java @@ -17,6 +17,7 @@ package google.registry.flows; import static com.google.common.base.Strings.nullToEmpty; import static com.google.common.io.BaseEncoding.base64; import static google.registry.flows.EppXmlTransformer.unmarshal; +import static google.registry.flows.FlowReporter.extractTlds; import static java.nio.charset.StandardCharsets.UTF_8; import com.google.common.annotations.VisibleForTesting; @@ -89,6 +90,9 @@ public final class EppController { } if (!eppInput.getTargetIds().isEmpty()) { eppMetricBuilder.setEppTarget(Joiner.on(',').join(eppInput.getTargetIds())); + if (eppInput.isDomainResourceType()) { + eppMetricBuilder.setTlds(extractTlds(eppInput.getTargetIds())); + } } EppOutput output = runFlowConvertEppErrors(flowComponentBuilder .flowModule(new FlowModule.Builder() @@ -106,10 +110,12 @@ public final class EppController { } return output; } finally { - EppMetric metric = eppMetricBuilder.build(); - bigQueryMetricsEnqueuer.export(metric); - eppMetrics.incrementEppRequests(metric); - eppMetrics.recordProcessingTime(metric); + if (!isDryRun) { + EppMetric metric = eppMetricBuilder.build(); + bigQueryMetricsEnqueuer.export(metric); + eppMetrics.incrementEppRequests(metric); + eppMetrics.recordProcessingTime(metric); + } } } diff --git a/java/google/registry/flows/EppMetrics.java b/java/google/registry/flows/EppMetrics.java index e2f105faa..0be4e6316 100644 --- a/java/google/registry/flows/EppMetrics.java +++ b/java/google/registry/flows/EppMetrics.java @@ -14,6 +14,8 @@ package google.registry.flows; +import static google.registry.monitoring.metrics.EventMetric.DEFAULT_FITTER; + import com.google.common.collect.ImmutableSet; import google.registry.monitoring.metrics.EventMetric; import google.registry.monitoring.metrics.IncrementableMetric; @@ -25,31 +27,57 @@ import javax.inject.Inject; /** EPP Instrumentation. */ public class EppMetrics { - private static final ImmutableSet LABEL_DESCRIPTORS = + private static final ImmutableSet LABEL_DESCRIPTORS_BY_REGISTRAR = ImmutableSet.of( LabelDescriptor.create("command", "The name of the command."), LabelDescriptor.create("client_id", "The name of the client."), LabelDescriptor.create("status", "The return status of the command.")); - private static final IncrementableMetric eppRequests = + private static final ImmutableSet LABEL_DESCRIPTORS_BY_TLD = + ImmutableSet.of( + LabelDescriptor.create("command", "The name of the command."), + LabelDescriptor.create("tld", "The TLD acted on by the command (if applicable)."), + LabelDescriptor.create("status", "The return status of the command.")); + + private static final IncrementableMetric eppRequestsByRegistrar = MetricRegistryImpl.getDefault() .newIncrementableMetric( - "/epp/requests", "Count of EPP Requests", "count", LABEL_DESCRIPTORS); + "/epp/requests", + "Count of EPP Requests By Registrar", + "count", + LABEL_DESCRIPTORS_BY_REGISTRAR); - private static final EventMetric processingTime = + private static final IncrementableMetric eppRequestsByTld = + MetricRegistryImpl.getDefault() + .newIncrementableMetric( + "/epp/requests_by_tld", + "Count of EPP Requests By TLD", + "count", + LABEL_DESCRIPTORS_BY_TLD); + + private static final EventMetric processingTimeByRegistrar = MetricRegistryImpl.getDefault() .newEventMetric( "/epp/processing_time", - "EPP Processing Time", + "EPP Processing Time By Registrar", "milliseconds", - LABEL_DESCRIPTORS, - EventMetric.DEFAULT_FITTER); + LABEL_DESCRIPTORS_BY_REGISTRAR, + DEFAULT_FITTER); + + private static final EventMetric processingTimeByTld = + MetricRegistryImpl.getDefault() + .newEventMetric( + "/epp/processing_time_by_tld", + "EPP Processing Time By TLD", + "milliseconds", + LABEL_DESCRIPTORS_BY_TLD, + DEFAULT_FITTER); @Inject public EppMetrics() {} /** - * Increment a counter which tracks EPP requests. + * Increments the counters which tracks EPP requests. * * @see EppController * @see FlowRunner @@ -57,20 +85,21 @@ public class EppMetrics { public void incrementEppRequests(EppMetric metric) { String eppStatusCode = metric.getStatus().isPresent() ? String.valueOf(metric.getStatus().get().code) : ""; - eppRequests.increment( - metric.getCommandName().or(""), - metric.getClientId().or(""), - eppStatusCode); + eppRequestsByRegistrar.increment( + metric.getCommandName().or(""), metric.getClientId().or(""), eppStatusCode); + eppRequestsByTld.increment( + metric.getCommandName().or(""), metric.getTld().or(""), eppStatusCode); } - /** Record the server-side processing time for an EPP request. */ + /** Records the server-side processing time for an EPP request. */ public void recordProcessingTime(EppMetric metric) { String eppStatusCode = metric.getStatus().isPresent() ? String.valueOf(metric.getStatus().get().code) : ""; - processingTime.record( - metric.getEndTimestamp().getMillis() - metric.getStartTimestamp().getMillis(), - metric.getCommandName().or(""), - metric.getClientId().or(""), - eppStatusCode); + long processingTime = + metric.getEndTimestamp().getMillis() - metric.getStartTimestamp().getMillis(); + processingTimeByRegistrar.record( + processingTime, metric.getCommandName().or(""), metric.getClientId().or(""), eppStatusCode); + processingTimeByTld.record( + processingTime, metric.getCommandName().or(""), metric.getTld().or(""), eppStatusCode); } } diff --git a/java/google/registry/flows/FlowReporter.java b/java/google/registry/flows/FlowReporter.java index 3799d6de1..29abc014f 100644 --- a/java/google/registry/flows/FlowReporter.java +++ b/java/google/registry/flows/FlowReporter.java @@ -65,31 +65,34 @@ public class FlowReporter { logger.infofmt( "%s: %s", EPPINPUT_LOG_SIGNATURE, - JSONValue.toJSONString(ImmutableMap.of( - "xml", prettyPrint(inputXmlBytes), - "xmlBytes", base64().encode(inputXmlBytes)))); + JSONValue.toJSONString( + ImmutableMap.of( + "xml", prettyPrint(inputXmlBytes), + "xmlBytes", base64().encode(inputXmlBytes)))); // Explicitly log flow metadata separately from the EPP XML itself so that it stays compact // enough to be sure to fit in a single log entry (the XML part in rare cases could be long // enough to overflow into multiple log entries, breaking routine parsing of the JSON format). - String resourceType = eppInput.getResourceType().or(""); - boolean isDomain = "domain".equals(resourceType); String singleTargetId = eppInput.getSingleTargetId().or(""); ImmutableList targetIds = eppInput.getTargetIds(); logger.infofmt( "%s: %s", METADATA_LOG_SIGNATURE, - JSONValue.toJSONString(new ImmutableMap.Builder() - .put("serverTrid", trid.getServerTransactionId()) - .put("clientId", clientId) - .put("commandType", eppInput.getCommandType()) - .put("resourceType", resourceType) - .put("flowClassName", flowClass.getSimpleName()) - .put("targetId", singleTargetId) - .put("targetIds", targetIds) - .put("tld", isDomain ? extractTld(singleTargetId).or("") : "") - .put("tlds", isDomain ? extractTlds(targetIds).asList() : EMPTY_LIST) - .put("icannActivityReportField", extractActivityReportField(flowClass)) - .build())); + JSONValue.toJSONString( + new ImmutableMap.Builder() + .put("serverTrid", trid.getServerTransactionId()) + .put("clientId", clientId) + .put("commandType", eppInput.getCommandType()) + .put("resourceType", eppInput.getResourceType().or("")) + .put("flowClassName", flowClass.getSimpleName()) + .put("targetId", singleTargetId) + .put("targetIds", targetIds) + .put( + "tld", eppInput.isDomainResourceType() ? extractTld(singleTargetId).or("") : "") + .put( + "tlds", + eppInput.isDomainResourceType() ? extractTlds(targetIds).asList() : EMPTY_LIST) + .put("icannActivityReportField", extractActivityReportField(flowClass)) + .build())); } /** @@ -113,7 +116,7 @@ public class FlowReporter { * Returns the set of unique results of {@link #extractTld} applied to each given domain name, * excluding any absent results (i.e. cases where no TLD was detected). */ - private static final ImmutableSet extractTlds(Iterable domainNames) { + public static final ImmutableSet extractTlds(Iterable domainNames) { ImmutableSet.Builder set = new ImmutableSet.Builder<>(); for (String domainName : domainNames) { Optional extractedTld = extractTld(domainName); diff --git a/java/google/registry/model/eppinput/EppInput.java b/java/google/registry/model/eppinput/EppInput.java index 8ac861846..7b7330900 100644 --- a/java/google/registry/model/eppinput/EppInput.java +++ b/java/google/registry/model/eppinput/EppInput.java @@ -111,6 +111,11 @@ public class EppInput extends ImmutableObject { return Optional.absent(); } + /** Returns whether this EppInput represents a command that operates on domain resources. */ + public boolean isDomainResourceType() { + return getResourceType().or("").equals("domain"); + } + @Nullable private ResourceCommand getResourceCommand() { InnerCommand innerCommand = commandWrapper.getCommand(); diff --git a/java/google/registry/monitoring/metrics/Counter.java b/java/google/registry/monitoring/metrics/Counter.java index 0969b442b..88bb38f77 100644 --- a/java/google/registry/monitoring/metrics/Counter.java +++ b/java/google/registry/monitoring/metrics/Counter.java @@ -68,6 +68,11 @@ public final class Counter extends AbstractMetric */ private final Striped valueLocks = Striped.lock(DEFAULT_CONCURRENCY_LEVEL); + /** + * Constructs a new Counter. + * + *

Note that the order of the labels is significant. + */ Counter( String name, String description, diff --git a/java/google/registry/monitoring/metrics/MetricRegistryImpl.java b/java/google/registry/monitoring/metrics/MetricRegistryImpl.java index 32a7d6c5f..f341b6df9 100644 --- a/java/google/registry/monitoring/metrics/MetricRegistryImpl.java +++ b/java/google/registry/monitoring/metrics/MetricRegistryImpl.java @@ -42,6 +42,11 @@ public final class MetricRegistryImpl implements MetricRegistry { return INSTANCE; } + /** + * Creates a new event metric. + * + *

Note that the order of the labels is significant. + */ @Override public EventMetric newEventMetric( String name, @@ -57,6 +62,11 @@ public final class MetricRegistryImpl implements MetricRegistry { return metric; } + /** + * Creates a new gauge metric. + * + *

Note that the order of the labels is significant. + */ @Override @CanIgnoreReturnValue public Metric newGauge( @@ -75,6 +85,11 @@ public final class MetricRegistryImpl implements MetricRegistry { return metric; } + /** + * Creates a new settable metric. + * + *

Note that the order of the labels is significant. + */ @Override public SettableMetric newSettableMetric( String name, @@ -90,6 +105,11 @@ public final class MetricRegistryImpl implements MetricRegistry { return metric; } + /** + * Creates a new incrementable metric. + * + *

Note that the order of the labels is significant. + */ @Override public IncrementableMetric newIncrementableMetric( String name, diff --git a/java/google/registry/monitoring/whitebox/EppMetric.java b/java/google/registry/monitoring/whitebox/EppMetric.java index a43e45364..d3415268e 100644 --- a/java/google/registry/monitoring/whitebox/EppMetric.java +++ b/java/google/registry/monitoring/whitebox/EppMetric.java @@ -22,8 +22,11 @@ import com.google.auto.value.AutoValue; import com.google.common.base.Optional; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Iterables; import google.registry.bigquery.BigqueryUtils.FieldType; import google.registry.model.eppoutput.Result.Code; +import google.registry.model.registry.Registries; import google.registry.util.Clock; import org.joda.time.DateTime; @@ -43,6 +46,7 @@ public abstract class EppMetric implements BigQueryMetric { new TableFieldSchema().setName("endTime").setType(FieldType.TIMESTAMP.name()), new TableFieldSchema().setName("commandName").setType(FieldType.STRING.name()), new TableFieldSchema().setName("clientId").setType(FieldType.STRING.name()), + new TableFieldSchema().setName("tld").setType(FieldType.STRING.name()), new TableFieldSchema().setName("privilegeLevel").setType(FieldType.STRING.name()), new TableFieldSchema().setName("eppTarget").setType(FieldType.STRING.name()), new TableFieldSchema().setName("eppStatus").setType(FieldType.INTEGER.name()), @@ -58,6 +62,8 @@ public abstract class EppMetric implements BigQueryMetric { public abstract Optional getClientId(); + public abstract Optional getTld(); + public abstract Optional getPrivilegeLevel(); public abstract Optional getEppTarget(); @@ -88,6 +94,7 @@ public abstract class EppMetric implements BigQueryMetric { // Populate optional values, if present addOptional("commandName", getCommandName(), map); addOptional("clientId", getClientId(), map); + addOptional("tld", getTld(), map); addOptional("privilegeLevel", getPrivilegeLevel(), map); addOptional("eppTarget", getEppTarget(), map); if (getStatus().isPresent()) { @@ -155,6 +162,37 @@ public abstract class EppMetric implements BigQueryMetric { public abstract Builder setClientId(Optional clientId); + public abstract Builder setTld(String tld); + + public abstract Builder setTld(Optional tld); + + /** + * Sets the single TLD field from a list of TLDs associated with a command. + * + *

Due to cardinality reasons we cannot record combinations of different TLDs as might be + * seen in a domain check command, so if this happens we record "_various" instead. We also + * record "_invalid" for a TLD that does not exist in our system, as again that could blow up + * cardinality. Underscore prefixes are used for these sentinel values so that they cannot be + * confused with actual TLDs, which cannot start with underscores. + */ + public Builder setTlds(ImmutableSet tlds) { + switch (tlds.size()) { + case 0: + setTld(Optional.absent()); + break; + case 1: + String tld = Iterables.getOnlyElement(tlds); + // Only record TLDs that actually exist, otherwise we can blow up cardinality by recording + // an arbitrarily large number of strings. + setTld(Optional.fromNullable(Registries.getTlds().contains(tld) ? tld : "_invalid")); + break; + default: + setTld("_various"); + break; + } + return this; + } + public abstract Builder setPrivilegeLevel(String privilegeLevel); public abstract Builder setEppTarget(String eppTarget); diff --git a/javatests/google/registry/flows/EppControllerTest.java b/javatests/google/registry/flows/EppControllerTest.java index 368532d47..435652959 100644 --- a/javatests/google/registry/flows/EppControllerTest.java +++ b/javatests/google/registry/flows/EppControllerTest.java @@ -17,13 +17,16 @@ package google.registry.flows; import static com.google.common.io.BaseEncoding.base64; import static com.google.common.truth.Truth.assertThat; import static google.registry.flows.EppXmlTransformer.marshal; +import static google.registry.testing.DatastoreHelper.createTld; import static google.registry.testing.LogsSubject.assertAboutLogs; import static google.registry.testing.TestDataHelper.loadFileWithSubstitutions; import static google.registry.testing.TestLogHandlerUtils.findFirstLogRecordWithMessagePrefix; import static java.nio.charset.StandardCharsets.UTF_8; import static java.util.logging.Level.INFO; import static java.util.logging.Level.SEVERE; +import static org.mockito.Matchers.eq; import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyZeroInteractions; import static org.mockito.Mockito.when; import com.google.common.base.Splitter; @@ -64,12 +67,12 @@ import org.mockito.runners.MockitoJUnitRunner; public class EppControllerTest extends ShardableTestCase { @Rule - public AppEngineRule appEngineRule = new AppEngineRule.Builder().build(); + public AppEngineRule appEngineRule = new AppEngineRule.Builder().withDatastore().build(); @Mock SessionMetadata sessionMetadata; @Mock TransportCredentials transportCredentials; @Mock EppMetrics eppMetrics; - @Mock BigQueryMetricsEnqueuer metricsEnqueuer; + @Mock BigQueryMetricsEnqueuer bigQueryMetricsEnqueuer; @Mock FlowComponent.Builder flowComponentBuilder; @Mock FlowComponent flowComponent; @Mock FlowRunner flowRunner; @@ -77,9 +80,9 @@ public class EppControllerTest extends ShardableTestCase { @Mock EppResponse eppResponse; @Mock Result result; - private static final DateTime startTime = DateTime.parse("2016-09-01T00:00:00Z"); + private static final DateTime START_TIME = DateTime.parse("2016-09-01T00:00:00Z"); - private final Clock clock = new FakeClock(startTime); + private final Clock clock = new FakeClock(START_TIME); private final TestLogHandler logHandler = new TestLogHandler(); private final String domainCreateXml = @@ -105,7 +108,7 @@ public class EppControllerTest extends ShardableTestCase { eppController = new EppController(); eppController.eppMetricBuilder = EppMetric.builderForRequest("request-id-1", clock); when(flowRunner.run(eppController.eppMetricBuilder)).thenReturn(eppOutput); - eppController.bigQueryMetricsEnqueuer = metricsEnqueuer; + eppController.bigQueryMetricsEnqueuer = bigQueryMetricsEnqueuer; eppController.flowComponentBuilder = flowComponentBuilder; eppController.eppMetrics = eppMetrics; eppController.serverTridProvider = new FakeServerTridProvider(); @@ -130,10 +133,10 @@ public class EppControllerTest extends ShardableTestCase { new byte[0]); ArgumentCaptor metricCaptor = ArgumentCaptor.forClass(EppMetric.class); - verify(metricsEnqueuer).export(metricCaptor.capture()); + verify(bigQueryMetricsEnqueuer).export(metricCaptor.capture()); EppMetric metric = metricCaptor.getValue(); assertThat(metric.getRequestId()).isEqualTo("request-id-1"); - assertThat(metric.getStartTimestamp()).isEqualTo(startTime); + assertThat(metric.getStartTimestamp()).isEqualTo(START_TIME); assertThat(metric.getEndTimestamp()).isEqualTo(clock.nowUtc()); assertThat(metric.getClientId()).hasValue("some-client"); assertThat(metric.getPrivilegeLevel()).hasValue("NORMAL"); @@ -141,7 +144,7 @@ public class EppControllerTest extends ShardableTestCase { } @Test - public void testHandleEppCommand_regularEppCommand_exportsMetric() throws Exception { + public void testHandleEppCommand_regularEppCommand_exportsBigQueryMetric() throws Exception { eppController.handleEppCommand( sessionMetadata, transportCredentials, @@ -151,10 +154,10 @@ public class EppControllerTest extends ShardableTestCase { domainCreateXml.getBytes(UTF_8)); ArgumentCaptor metricCaptor = ArgumentCaptor.forClass(EppMetric.class); - verify(metricsEnqueuer).export(metricCaptor.capture()); + verify(bigQueryMetricsEnqueuer).export(metricCaptor.capture()); EppMetric metric = metricCaptor.getValue(); assertThat(metric.getRequestId()).isEqualTo("request-id-1"); - assertThat(metric.getStartTimestamp()).isEqualTo(startTime); + assertThat(metric.getStartTimestamp()).isEqualTo(START_TIME); assertThat(metric.getEndTimestamp()).isEqualTo(clock.nowUtc()); assertThat(metric.getClientId()).hasValue("some-client"); assertThat(metric.getPrivilegeLevel()).hasValue("SUPERUSER"); @@ -162,6 +165,46 @@ public class EppControllerTest extends ShardableTestCase { assertThat(metric.getEppTarget()).hasValue("example.tld"); } + @Test + public void testHandleEppCommand_regularEppCommand_exportsEppMetrics() throws Exception { + createTld("tld"); + // Note that some of the EPP metric fields, like # of attempts and command name, are set in + // FlowRunner, not EppController, and since FlowRunner is mocked out for these tests they won't + // actually get values. + EppMetric.Builder metricBuilder = + EppMetric.builderForRequest("request-id-1", clock) + .setClientId("some-client") + .setEppTarget("example.tld") + .setStatus(Code.SUCCESS_WITH_NO_MESSAGES) + .setTld("tld") + .setPrivilegeLevel("SUPERUSER"); + eppController.handleEppCommand( + sessionMetadata, + transportCredentials, + EppRequestSource.UNIT_TEST, + false, + true, + domainCreateXml.getBytes(UTF_8)); + + EppMetric expectedMetric = metricBuilder.build(); + verify(eppMetrics).incrementEppRequests(eq(expectedMetric)); + verify(eppMetrics).recordProcessingTime(eq(expectedMetric)); + } + + @Test + public void testHandleEppCommand_dryRunEppCommand_doesNotExportMetric() throws Exception { + eppController.handleEppCommand( + sessionMetadata, + transportCredentials, + EppRequestSource.UNIT_TEST, + true, + true, + domainCreateXml.getBytes(UTF_8)); + + verifyZeroInteractions(bigQueryMetricsEnqueuer); + verifyZeroInteractions(eppMetrics); + } + @Test public void testHandleEppCommand_unmarshallableData_loggedAtInfo_withJsonData() throws Exception { eppController.handleEppCommand( diff --git a/javatests/google/registry/flows/EppLifecycleContactTest.java b/javatests/google/registry/flows/EppLifecycleContactTest.java index f23e94a5d..eda75cf00 100644 --- a/javatests/google/registry/flows/EppLifecycleContactTest.java +++ b/javatests/google/registry/flows/EppLifecycleContactTest.java @@ -44,17 +44,19 @@ public class EppLifecycleContactTest extends EppTestCase { assertCommandAndResponse( "contact_create_sh8013.xml", null, - "contact_create_response_sh8013.xml", - ImmutableMap.of("CRDATE", "2000-06-01T00:00:00Z"), - DateTime.parse("2000-06-01T00:00:00Z")); + "contact_create_response_sh8013.xml", + ImmutableMap.of("CRDATE", "2000-06-01T00:00:00Z"), + DateTime.parse("2000-06-01T00:00:00Z")); assertThat(getRecordedEppMetric()) - .hasClientId("NewRegistrar") - .and() - .hasCommandName("ContactCreate") - .and() - .hasEppTarget("sh8013") - .and() - .hasStatus(SUCCESS); + .hasClientId("NewRegistrar") + .and() + .hasNoTld() + .and() + .hasCommandName("ContactCreate") + .and() + .hasEppTarget("sh8013") + .and() + .hasStatus(SUCCESS); assertCommandAndResponse( "contact_info.xml", "contact_info_from_create_response.xml", diff --git a/javatests/google/registry/flows/EppLifecycleDomainTest.java b/javatests/google/registry/flows/EppLifecycleDomainTest.java index 166957599..8d0ce7904 100644 --- a/javatests/google/registry/flows/EppLifecycleDomainTest.java +++ b/javatests/google/registry/flows/EppLifecycleDomainTest.java @@ -206,6 +206,8 @@ public class EppLifecycleDomainTest extends EppTestCase { assertThat(getRecordedEppMetric()) .hasClientId("NewRegistrar") .and() + .hasNoTld() + .and() .hasCommandName("Login") .and() .hasStatus(SUCCESS); @@ -233,6 +235,8 @@ public class EppLifecycleDomainTest extends EppTestCase { assertThat(getRecordedEppMetric()) .hasClientId("NewRegistrar") .and() + .hasTld("example") + .and() .hasCommandName("DomainDelete") .and() .hasEppTarget("fakesite.example") @@ -353,6 +357,8 @@ public class EppLifecycleDomainTest extends EppTestCase { .and() .hasEppTarget("rich.example") .and() + .hasTld("example") + .and() .hasStatus(SUCCESS); assertCommandAndResponse("logout.xml", "logout_response.xml"); diff --git a/javatests/google/registry/flows/FlowReporterTest.java b/javatests/google/registry/flows/FlowReporterTest.java index fa80ec2c1..8591b5c6d 100644 --- a/javatests/google/registry/flows/FlowReporterTest.java +++ b/javatests/google/registry/flows/FlowReporterTest.java @@ -100,6 +100,8 @@ public class FlowReporterTest extends ShardableTestCase { @Test public void testRecordToLogs_metadata_basic() throws Exception { + when(flowReporter.eppInput.isDomainResourceType()).thenReturn(true); + when(flowReporter.eppInput.getResourceType()).thenReturn(Optional.of("domain")); flowReporter.recordToLogs(); assertThat(parseJsonMap(findFirstLogMessageByPrefix(handler, "FLOW-LOG-SIGNATURE-METADATA: "))) .containsExactly( @@ -136,6 +138,7 @@ public class FlowReporterTest extends ShardableTestCase { @Test public void testRecordToLogs_metadata_notResourceFlow_noResourceTypeOrTld() throws Exception { + when(flowReporter.eppInput.isDomainResourceType()).thenReturn(false); when(flowReporter.eppInput.getResourceType()).thenReturn(Optional.absent()); flowReporter.recordToLogs(); Map json = @@ -148,6 +151,7 @@ public class FlowReporterTest extends ShardableTestCase { @Test public void testRecordToLogs_metadata_notDomainFlow_noTld() throws Exception { + when(flowReporter.eppInput.isDomainResourceType()).thenReturn(false); when(flowReporter.eppInput.getResourceType()).thenReturn(Optional.of("contact")); flowReporter.recordToLogs(); Map json = @@ -159,6 +163,8 @@ public class FlowReporterTest extends ShardableTestCase { @Test public void testRecordToLogs_metadata_multipartDomainName_multipartTld() throws Exception { + when(flowReporter.eppInput.isDomainResourceType()).thenReturn(true); + when(flowReporter.eppInput.getResourceType()).thenReturn(Optional.of("domain")); when(flowReporter.eppInput.getSingleTargetId()).thenReturn(Optional.of("target.co.uk")); when(flowReporter.eppInput.getTargetIds()).thenReturn(ImmutableList.of("target.co.uk")); flowReporter.recordToLogs(); @@ -172,6 +178,7 @@ public class FlowReporterTest extends ShardableTestCase { @Test public void testRecordToLogs_metadata_multipleTargetIds_uniqueTldSet() throws Exception { + when(flowReporter.eppInput.isDomainResourceType()).thenReturn(true); when(flowReporter.eppInput.getSingleTargetId()).thenReturn(Optional.absent()); when(flowReporter.eppInput.getTargetIds()) .thenReturn(ImmutableList.of("target.co.uk", "foo.uk", "bar.uk", "baz.com")); @@ -187,6 +194,7 @@ public class FlowReporterTest extends ShardableTestCase { @Test public void testRecordToLogs_metadata_uppercaseDomainName_lowercaseTld() throws Exception { + when(flowReporter.eppInput.isDomainResourceType()).thenReturn(true); when(flowReporter.eppInput.getSingleTargetId()).thenReturn(Optional.of("TARGET.FOO")); when(flowReporter.eppInput.getTargetIds()).thenReturn(ImmutableList.of("TARGET.FOO")); flowReporter.recordToLogs(); @@ -200,6 +208,7 @@ public class FlowReporterTest extends ShardableTestCase { @Test public void testRecordToLogs_metadata_invalidDomainName_stillGuessesTld() throws Exception { + when(flowReporter.eppInput.isDomainResourceType()).thenReturn(true); when(flowReporter.eppInput.getSingleTargetId()).thenReturn(Optional.of("")); when(flowReporter.eppInput.getTargetIds()).thenReturn(ImmutableList.of("")); flowReporter.recordToLogs(); @@ -213,6 +222,7 @@ public class FlowReporterTest extends ShardableTestCase { @Test public void testRecordToLogs_metadata_domainWithoutPeriod_noTld() throws Exception { + when(flowReporter.eppInput.isDomainResourceType()).thenReturn(true); when(flowReporter.eppInput.getSingleTargetId()).thenReturn(Optional.of("target,foo")); when(flowReporter.eppInput.getTargetIds()).thenReturn(ImmutableList.of("target,foo")); flowReporter.recordToLogs(); diff --git a/javatests/google/registry/monitoring/whitebox/EppMetricTest.java b/javatests/google/registry/monitoring/whitebox/EppMetricTest.java index 5358234e3..0d74b60e0 100644 --- a/javatests/google/registry/monitoring/whitebox/EppMetricTest.java +++ b/javatests/google/registry/monitoring/whitebox/EppMetricTest.java @@ -15,12 +15,15 @@ package google.registry.monitoring.whitebox; import static com.google.common.truth.Truth.assertThat; +import static google.registry.testing.DatastoreHelper.createTld; +import static google.registry.testing.DatastoreHelper.createTlds; import com.google.api.services.bigquery.model.TableFieldSchema; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import google.registry.model.eppoutput.Result.Code; import google.registry.testing.AppEngineRule; +import google.registry.testing.FakeClock; import org.joda.time.DateTime; import org.junit.Rule; import org.junit.Test; @@ -31,7 +34,45 @@ import org.junit.runners.JUnit4; @RunWith(JUnit4.class) public class EppMetricTest { - @Rule public final AppEngineRule appEngine = AppEngineRule.builder().build(); + @Rule public final AppEngineRule appEngine = AppEngineRule.builder().withDatastore().build(); + + @Test + public void test_invalidTld_isRecordedAsInvalid() throws Exception { + EppMetric metric = + EppMetric.builderForRequest("request-id-1", new FakeClock()) + .setTlds(ImmutableSet.of("notarealtld")) + .build(); + assertThat(metric.getTld()).hasValue("_invalid"); + } + + @Test + public void test_validTld_isRecorded() throws Exception { + createTld("example"); + EppMetric metric = + EppMetric.builderForRequest("request-id-1", new FakeClock()) + .setTlds(ImmutableSet.of("example")) + .build(); + assertThat(metric.getTld()).hasValue("example"); + } + + @Test + public void test_multipleTlds_areRecordedAsVarious() throws Exception { + createTlds("foo", "bar"); + EppMetric metric = + EppMetric.builderForRequest("request-id-1", new FakeClock()) + .setTlds(ImmutableSet.of("foo", "bar", "baz")) + .build(); + assertThat(metric.getTld()).hasValue("_various"); + } + + @Test + public void test_zeroTlds_areRecordedAsAbsent() throws Exception { + EppMetric metric = + EppMetric.builderForRequest("request-id-1", new FakeClock()) + .setTlds(ImmutableSet.of()) + .build(); + assertThat(metric.getTld()).isAbsent(); + } @Test public void testGetBigQueryRowEncoding_encodesCorrectly() throws Exception { @@ -42,6 +83,7 @@ public class EppMetricTest { .setEndTimestamp(new DateTime(1338)) .setCommandName("command") .setClientId("client") + .setTld("example") .setPrivilegeLevel("level") .setEppTarget("target") .setStatus(Code.COMMAND_USE_ERROR) @@ -56,6 +98,7 @@ public class EppMetricTest { .put("endTime", "1.338000") .put("commandName", "command") .put("clientId", "client") + .put("tld", "example") .put("privilegeLevel", "level") .put("eppTarget", "target") .put("eppStatus", "2002") @@ -72,6 +115,7 @@ public class EppMetricTest { .setEndTimestamp(new DateTime(1338)) .setCommandName("command") .setClientId("client") + .setTld("example") .setPrivilegeLevel("level") .setEppTarget("target") .setStatus(Code.COMMAND_USE_ERROR) diff --git a/javatests/google/registry/testing/EppMetricSubject.java b/javatests/google/registry/testing/EppMetricSubject.java index 36ea95ebd..d6bbbb141 100644 --- a/javatests/google/registry/testing/EppMetricSubject.java +++ b/javatests/google/registry/testing/EppMetricSubject.java @@ -61,6 +61,17 @@ public class EppMetricSubject extends Subject { return new And<>(this); } + public And hasTld(String tld) { + return hasValue(tld, actual().getTld(), "has tld"); + } + + public And hasNoTld() { + if (actual().getTld().isPresent()) { + fail("has no tld"); + } + return new And<>(this); + } + private And hasValue(E expected, Optional actual, String verb) { checkArgumentNotNull(expected, "Expected value cannot be null"); if (actual == null) {