From 4260fb573fc207c6161a04ef957ae8ac1ea8e6a9 Mon Sep 17 00:00:00 2001 From: mcilwain Date: Fri, 24 Mar 2017 09:03:01 -0700 Subject: [PATCH] Use the actual EPP command flow name for EppMetrics It was previously only using the name of the inner command XML element, e.g. "Create", "Delete", "Update", etc. This wasn't very useful because there was no way to discriminate between operations on different types of EPP resources. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=151131491 --- java/google/registry/flows/EppController.java | 1 - java/google/registry/flows/FlowRunner.java | 2 + .../monitoring/whitebox/EppMetric.java | 10 ++++- .../registry/flows/EppControllerTest.java | 1 - .../google/registry/flows/FlowRunnerTest.java | 37 +++++++++++++------ 5 files changed, 36 insertions(+), 15 deletions(-) diff --git a/java/google/registry/flows/EppController.java b/java/google/registry/flows/EppController.java index e9faca719..cb5fe2742 100644 --- a/java/google/registry/flows/EppController.java +++ b/java/google/registry/flows/EppController.java @@ -85,7 +85,6 @@ public final class EppController { metricBuilder.setStatus(e.getResult().getCode()); return getErrorResponse(e.getResult(), Trid.create(null)); } - metricBuilder.setCommandName(eppInput.getCommandName()); if (!eppInput.getTargetIds().isEmpty()) { metricBuilder.setEppTarget(Joiner.on(',').join(eppInput.getTargetIds())); } diff --git a/java/google/registry/flows/FlowRunner.java b/java/google/registry/flows/FlowRunner.java index 5162fe426..46c5ca711 100644 --- a/java/google/registry/flows/FlowRunner.java +++ b/java/google/registry/flows/FlowRunner.java @@ -56,6 +56,7 @@ public class FlowRunner { @Inject TransportCredentials credentials; @Inject EppRequestSource eppRequestSource; @Inject Provider flowProvider; + @Inject Class flowClass; @Inject @InputXml byte[] inputXmlBytes; @Inject @DryRun boolean isDryRun; @Inject @Superuser boolean isSuperuser; @@ -92,6 +93,7 @@ public class FlowRunner { "clientId", clientId, "xml", prettyXml, "xmlBytes", xmlBase64))); + metric.setCommandNameFromFlow(flowClass.getSimpleName()); if (!isTransactional) { metric.incrementAttempts(); return EppOutput.create(flowProvider.get().run()); diff --git a/java/google/registry/monitoring/whitebox/EppMetric.java b/java/google/registry/monitoring/whitebox/EppMetric.java index f2955d7e9..a43e45364 100644 --- a/java/google/registry/monitoring/whitebox/EppMetric.java +++ b/java/google/registry/monitoring/whitebox/EppMetric.java @@ -14,6 +14,7 @@ package google.registry.monitoring.whitebox; +import static com.google.common.base.Preconditions.checkArgument; import static google.registry.bigquery.BigqueryUtils.toBigqueryTimestamp; import com.google.api.services.bigquery.model.TableFieldSchema; @@ -141,7 +142,14 @@ public abstract class EppMetric implements BigQueryMetric { abstract Builder setEndTimestamp(DateTime endTimestamp); - public abstract Builder setCommandName(String commandName); + abstract Builder setCommandName(String commandName); + + public Builder setCommandNameFromFlow(String flowSimpleClassName) { + checkArgument( + flowSimpleClassName.endsWith("Flow"), + "Must pass in the simple class name of a flow class"); + return setCommandName(flowSimpleClassName.replaceFirst("Flow$", "")); + } public abstract Builder setClientId(String clientId); diff --git a/javatests/google/registry/flows/EppControllerTest.java b/javatests/google/registry/flows/EppControllerTest.java index ca9130909..30c2a8ddb 100644 --- a/javatests/google/registry/flows/EppControllerTest.java +++ b/javatests/google/registry/flows/EppControllerTest.java @@ -138,7 +138,6 @@ public class EppControllerTest extends ShardableTestCase { assertThat(metric.getClientId()).hasValue("some-client"); assertThat(metric.getPrivilegeLevel()).hasValue("SUPERUSER"); assertThat(metric.getStatus()).hasValue(Code.SUCCESS_WITH_NO_MESSAGES); - assertThat(metric.getCommandName()).hasValue("Create"); assertThat(metric.getEppTarget()).hasValue("example.tld"); } } diff --git a/javatests/google/registry/flows/FlowRunnerTest.java b/javatests/google/registry/flows/FlowRunnerTest.java index 1267f64a2..afa214070 100644 --- a/javatests/google/registry/flows/FlowRunnerTest.java +++ b/javatests/google/registry/flows/FlowRunnerTest.java @@ -30,6 +30,7 @@ import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; import com.google.common.testing.TestLogHandler; import google.registry.model.eppcommon.Trid; +import google.registry.model.eppoutput.EppOutput.ResponseOrGreeting; import google.registry.model.eppoutput.EppResponse; import google.registry.monitoring.whitebox.EppMetric; import google.registry.testing.AppEngineRule; @@ -59,22 +60,21 @@ public class FlowRunnerTest extends ShardableTestCase { private final TestLogHandler handler = new TestLogHandler(); + static class TestCommandFlow implements Flow { + @Override + public ResponseOrGreeting run() throws EppException { + return mock(EppResponse.class); + } + } + @Before public void before() { Logger.getLogger(FlowRunner.class.getCanonicalName()).addHandler(handler); - - final EppResponse eppResponse = mock(EppResponse.class); - flowRunner.clientId = "TheRegistrar"; flowRunner.credentials = new PasswordOnlyTransportCredentials(); flowRunner.eppRequestSource = EppRequestSource.UNIT_TEST; - flowRunner.flowProvider = - Providers.of( - new Flow() { - @Override - public EppResponse run() { - return eppResponse; - }}); + flowRunner.flowProvider = Providers.of(new TestCommandFlow()); + flowRunner.flowClass = TestCommandFlow.class; flowRunner.inputXmlBytes = "".getBytes(UTF_8); flowRunner.isDryRun = false; flowRunner.isSuperuser = false; @@ -98,18 +98,31 @@ public class FlowRunnerTest extends ShardableTestCase { } @Test - public void testRun_notIsTransactional_incrementsMetricAttempts() throws Exception { + public void testRun_nonTransactionalCommand_incrementsMetricAttempts() throws Exception { flowRunner.run(); assertThat(flowRunner.metric.build().getAttempts()).isEqualTo(1); } @Test - public void testRun_isTransactional_incrementsMetricAttempts() throws Exception { + public void testRun_transactionalCommand_incrementsMetricAttempts() throws Exception { flowRunner.isTransactional = true; flowRunner.run(); assertThat(flowRunner.metric.build().getAttempts()).isEqualTo(1); } + @Test + public void testRun_nonTransactionalCommand_setsCommandNameOnMetric() throws Exception { + flowRunner.isTransactional = true; + flowRunner.run(); + assertThat(flowRunner.metric.build().getCommandName()).hasValue("TestCommand"); + } + + @Test + public void testRun_transactionalCommand_setsCommandNameOnMetric() throws Exception { + flowRunner.run(); + assertThat(flowRunner.metric.build().getCommandName()).hasValue("TestCommand"); + } + @Test public void testRun_reportingLogStatement_noClientId() throws Exception { flowRunner.clientId = "";