diff --git a/java/google/registry/flows/EppController.java b/java/google/registry/flows/EppController.java index cb5fe2742..722440aa5 100644 --- a/java/google/registry/flows/EppController.java +++ b/java/google/registry/flows/EppController.java @@ -47,7 +47,7 @@ public final class EppController { private static final FormattingLogger logger = FormattingLogger.getLoggerForCallerClass(); @Inject FlowComponent.Builder flowComponentBuilder; - @Inject EppMetric.Builder metricBuilder; + @Inject EppMetric.Builder eppMetricBuilder; @Inject EppMetrics eppMetrics; @Inject BigQueryMetricsEnqueuer bigQueryMetricsEnqueuer; @Inject EppController() {} @@ -60,8 +60,8 @@ public final class EppController { boolean isDryRun, boolean isSuperuser, byte[] inputXmlBytes) { - metricBuilder.setClientId(Optional.fromNullable(sessionMetadata.getClientId())); - metricBuilder.setPrivilegeLevel(isSuperuser ? "SUPERUSER" : "NORMAL"); + eppMetricBuilder.setClientId(Optional.fromNullable(sessionMetadata.getClientId())); + eppMetricBuilder.setPrivilegeLevel(isSuperuser ? "SUPERUSER" : "NORMAL"); try { EppInput eppInput; try { @@ -82,11 +82,11 @@ public final class EppController { new String(inputXmlBytes, UTF_8).trim(), // Charset decoding failures are swallowed. Strings.repeat("=", 40)); // Return early by sending an error message, with no clTRID since we couldn't unmarshal it. - metricBuilder.setStatus(e.getResult().getCode()); + eppMetricBuilder.setStatus(e.getResult().getCode()); return getErrorResponse(e.getResult(), Trid.create(null)); } if (!eppInput.getTargetIds().isEmpty()) { - metricBuilder.setEppTarget(Joiner.on(',').join(eppInput.getTargetIds())); + eppMetricBuilder.setEppTarget(Joiner.on(',').join(eppInput.getTargetIds())); } EppOutput output = runFlowConvertEppErrors(flowComponentBuilder .flowModule(new FlowModule.Builder() @@ -100,11 +100,11 @@ public final class EppController { .build()) .build()); if (output.isResponse()) { - metricBuilder.setStatus(output.getResponse().getResult().getCode()); + eppMetricBuilder.setStatus(output.getResponse().getResult().getCode()); } return output; } finally { - EppMetric metric = metricBuilder.build(); + EppMetric metric = eppMetricBuilder.build(); bigQueryMetricsEnqueuer.export(metric); eppMetrics.incrementEppRequests(metric); eppMetrics.recordProcessingTime(metric); @@ -114,7 +114,7 @@ public final class EppController { /** Runs an EPP flow and converts known exceptions into EPP error responses. */ private EppOutput runFlowConvertEppErrors(FlowComponent flowComponent) { try { - return flowComponent.flowRunner().run(); + return flowComponent.flowRunner().run(eppMetricBuilder); } catch (EppException | EppExceptionInProviderException e) { // The command failed. Send the client an error message. EppException eppEx = (EppException) (e instanceof EppException ? e : e.getCause()); diff --git a/java/google/registry/flows/FlowRunner.java b/java/google/registry/flows/FlowRunner.java index 2c4d73bf4..4733e90a1 100644 --- a/java/google/registry/flows/FlowRunner.java +++ b/java/google/registry/flows/FlowRunner.java @@ -62,12 +62,12 @@ public class FlowRunner { @Inject @DryRun boolean isDryRun; @Inject @Superuser boolean isSuperuser; @Inject @Transactional boolean isTransactional; - @Inject EppMetric.Builder metric; @Inject SessionMetadata sessionMetadata; @Inject Trid trid; @Inject FlowRunner() {} - public EppOutput run() throws EppException { + /** Runs the EPP flow, and records metrics on the given builder. */ + public EppOutput run(final EppMetric.Builder eppMetricBuilder) throws EppException { String prettyXml = prettyPrint(inputXmlBytes); String xmlBase64 = base64().encode(inputXmlBytes); // This log line is very fragile since it's used for ICANN reporting - DO NOT CHANGE. @@ -95,16 +95,16 @@ public class FlowRunner { "xml", prettyXml, "xmlBytes", xmlBase64, "icannActivityReportField", extractActivityReportField(flowClass)))); - metric.setCommandNameFromFlow(flowClass.getSimpleName()); + eppMetricBuilder.setCommandNameFromFlow(flowClass.getSimpleName()); if (!isTransactional) { - metric.incrementAttempts(); + eppMetricBuilder.incrementAttempts(); return EppOutput.create(flowProvider.get().run()); } try { return ofy().transact(new Work() { @Override public EppOutput run() { - metric.incrementAttempts(); + eppMetricBuilder.incrementAttempts(); try { EppOutput output = EppOutput.create(flowProvider.get().run()); if (isDryRun) { diff --git a/javatests/google/registry/flows/EppCommitLogsTest.java b/javatests/google/registry/flows/EppCommitLogsTest.java index 22cac4163..d98974088 100644 --- a/javatests/google/registry/flows/EppCommitLogsTest.java +++ b/javatests/google/registry/flows/EppCommitLogsTest.java @@ -29,6 +29,7 @@ import google.registry.config.RegistryConfig.ConfigModule.TmchCaMode; import google.registry.flows.EppTestComponent.FakesAndMocksModule; import google.registry.model.domain.DomainResource; import google.registry.model.ofy.Ofy; +import google.registry.monitoring.whitebox.EppMetric; import google.registry.testing.AppEngineRule; import google.registry.testing.EppLoader; import google.registry.testing.ExceptionRule; @@ -87,7 +88,7 @@ public class EppCommitLogsTest extends ShardableTestCase { .build()) .build() .flowRunner() - .run(); + .run(EppMetric.builder()); } @Test diff --git a/javatests/google/registry/flows/EppControllerTest.java b/javatests/google/registry/flows/EppControllerTest.java index 30c2a8ddb..c8da36482 100644 --- a/javatests/google/registry/flows/EppControllerTest.java +++ b/javatests/google/registry/flows/EppControllerTest.java @@ -75,14 +75,14 @@ public class EppControllerTest extends ShardableTestCase { .thenReturn(flowComponentBuilder); when(flowComponentBuilder.build()).thenReturn(flowComponent); when(flowComponent.flowRunner()).thenReturn(flowRunner); - when(flowRunner.run()).thenReturn(eppOutput); when(eppOutput.isResponse()).thenReturn(true); when(eppOutput.getResponse()).thenReturn(eppResponse); when(eppResponse.getResult()).thenReturn(result); when(result.getCode()).thenReturn(Code.SUCCESS_WITH_NO_MESSAGES); eppController = new EppController(); - eppController.metricBuilder = EppMetric.builderForRequest("request-id-1", clock); + eppController.eppMetricBuilder = EppMetric.builderForRequest("request-id-1", clock); + when(flowRunner.run(eppController.eppMetricBuilder)).thenReturn(eppOutput); eppController.bigQueryMetricsEnqueuer = metricsEnqueuer; eppController.flowComponentBuilder = flowComponentBuilder; eppController.eppMetrics = eppMetrics; diff --git a/javatests/google/registry/flows/FlowRunnerTest.java b/javatests/google/registry/flows/FlowRunnerTest.java index 29e8fabe2..95f1e1efd 100644 --- a/javatests/google/registry/flows/FlowRunnerTest.java +++ b/javatests/google/registry/flows/FlowRunnerTest.java @@ -59,6 +59,8 @@ public class FlowRunnerTest extends ShardableTestCase { public final AppEngineRule appEngineRule = new AppEngineRule.Builder().build(); private final FlowRunner flowRunner = new FlowRunner(); + private final EppMetric.Builder eppMetricBuilder = + EppMetric.builderForRequest("request-id-1", new FakeClock()); private final TestLogHandler handler = new TestLogHandler(); @@ -89,7 +91,6 @@ public class FlowRunnerTest extends ShardableTestCase { flowRunner.isDryRun = false; flowRunner.isSuperuser = false; flowRunner.isTransactional = false; - flowRunner.metric = EppMetric.builderForRequest("request-id-1", new FakeClock()); flowRunner.sessionMetadata = new StatelessRequestSessionMetadata("TheRegistrar", ImmutableSet.of()); flowRunner.trid = Trid.create("client-123", "server-456"); @@ -97,33 +98,33 @@ public class FlowRunnerTest extends ShardableTestCase { @Test public void testRun_nonTransactionalCommand_incrementsMetricAttempts() throws Exception { - flowRunner.run(); - assertThat(flowRunner.metric.build().getAttempts()).isEqualTo(1); + flowRunner.run(eppMetricBuilder); + assertThat(eppMetricBuilder.build().getAttempts()).isEqualTo(1); } @Test public void testRun_transactionalCommand_incrementsMetricAttempts() throws Exception { flowRunner.isTransactional = true; - flowRunner.run(); - assertThat(flowRunner.metric.build().getAttempts()).isEqualTo(1); + flowRunner.run(eppMetricBuilder); + assertThat(eppMetricBuilder.build().getAttempts()).isEqualTo(1); } @Test public void testRun_nonTransactionalCommand_setsCommandNameOnMetric() throws Exception { flowRunner.isTransactional = true; - flowRunner.run(); - assertThat(flowRunner.metric.build().getCommandName()).hasValue("TestCommand"); + flowRunner.run(eppMetricBuilder); + assertThat(eppMetricBuilder.build().getCommandName()).hasValue("TestCommand"); } @Test public void testRun_transactionalCommand_setsCommandNameOnMetric() throws Exception { - flowRunner.run(); - assertThat(flowRunner.metric.build().getCommandName()).hasValue("TestCommand"); + flowRunner.run(eppMetricBuilder); + assertThat(eppMetricBuilder.build().getCommandName()).hasValue("TestCommand"); } @Test public void testRun_reportingLogStatement_basic() throws Exception { - flowRunner.run(); + flowRunner.run(eppMetricBuilder); assertThat(parseJsonMap(findLogMessageByPrefix(handler, "EPP-REPORTING-LOG-SIGNATURE: "))) .containsExactly( "trid", "server-456", @@ -136,7 +137,7 @@ public class FlowRunnerTest extends ShardableTestCase { @Test public void testRun_reportingLogStatement_withReportingSpec() throws Exception { flowRunner.flowClass = TestReportingSpecCommandFlow.class; - flowRunner.run(); + flowRunner.run(eppMetricBuilder); assertThat(parseJsonMap(findLogMessageByPrefix(handler, "EPP-REPORTING-LOG-SIGNATURE: "))) .containsExactly( "trid", "server-456", @@ -149,7 +150,7 @@ public class FlowRunnerTest extends ShardableTestCase { @Test public void testRun_reportingLogStatement_noClientId() throws Exception { flowRunner.clientId = ""; - flowRunner.run(); + flowRunner.run(eppMetricBuilder); assertThat(parseJsonMap(findLogMessageByPrefix(handler, "EPP-REPORTING-LOG-SIGNATURE: "))) .containsExactly( "trid", "server-456", @@ -164,7 +165,7 @@ public class FlowRunnerTest extends ShardableTestCase { String domainCreateXml = loadFileWithSubstitutions( getClass(), "domain_create_prettyprinted.xml", ImmutableMap.of()); flowRunner.inputXmlBytes = domainCreateXml.getBytes(UTF_8); - flowRunner.run(); + flowRunner.run(eppMetricBuilder); assertThat(parseJsonMap(findLogMessageByPrefix(handler, "EPP-REPORTING-LOG-SIGNATURE: "))) .containsExactly( "trid", "server-456", @@ -176,7 +177,7 @@ public class FlowRunnerTest extends ShardableTestCase { @Test public void testRun_legacyLoggingStatement_basic() throws Exception { - flowRunner.run(); + flowRunner.run(eppMetricBuilder); assertThat(Splitter.on("\n\t").split(findLogMessageByPrefix(handler, "EPP Command\n\t"))) .containsExactly( "server-456", @@ -195,7 +196,7 @@ public class FlowRunnerTest extends ShardableTestCase { public void testRun_legacyLoggingStatement_httpSessionMetadata() throws Exception { flowRunner.sessionMetadata = new HttpSessionMetadata(new FakeHttpSession()); flowRunner.sessionMetadata.setClientId("TheRegistrar"); - flowRunner.run(); + flowRunner.run(eppMetricBuilder); assertThat(Splitter.on("\n\t").split(findLogMessageByPrefix(handler, "EPP Command\n\t"))) .contains( "HttpSessionMetadata" @@ -206,7 +207,7 @@ public class FlowRunnerTest extends ShardableTestCase { public void testRun_legacyLoggingStatement_gaeUserCredentials() throws Exception { flowRunner.credentials = GaeUserCredentials.forTestingUser(new User("user@example.com", "authDomain"), false); - flowRunner.run(); + flowRunner.run(eppMetricBuilder); assertThat(Splitter.on("\n\t").split(findLogMessageByPrefix(handler, "EPP Command\n\t"))) .contains("GaeUserCredentials{gaeUser=user@example.com, isAdmin=false}"); } @@ -214,7 +215,7 @@ public class FlowRunnerTest extends ShardableTestCase { @Test public void testRun_legacyLoggingStatement_tlsCredentials() throws Exception { flowRunner.credentials = new TlsCredentials("abc123def", Optional.of("127.0.0.1"), "sni"); - flowRunner.run(); + flowRunner.run(eppMetricBuilder); assertThat(Splitter.on("\n\t").split(findLogMessageByPrefix(handler, "EPP Command\n\t"))) .contains( "TlsCredentials{clientCertificateHash=abc123def, clientAddress=/127.0.0.1, sni=sni}"); @@ -223,7 +224,7 @@ public class FlowRunnerTest extends ShardableTestCase { @Test public void testRun_legacyLoggingStatement_dryRun() throws Exception { flowRunner.isDryRun = true; - flowRunner.run(); + flowRunner.run(eppMetricBuilder); assertThat(Splitter.on("\n\t").split(findLogMessageByPrefix(handler, "EPP Command\n\t"))) .contains("DRY_RUN"); } @@ -233,7 +234,7 @@ public class FlowRunnerTest extends ShardableTestCase { String domainCreateXml = loadFileWithSubstitutions( getClass(), "domain_create_prettyprinted.xml", ImmutableMap.of()); flowRunner.inputXmlBytes = domainCreateXml.getBytes(UTF_8); - flowRunner.run(); + flowRunner.run(eppMetricBuilder); String logMessage = findLogMessageByPrefix(handler, "EPP Command\n\t"); List lines = Splitter.on("\n\t").splitToList(logMessage); assertThat(lines.size()).named("number of lines in log message").isAtLeast(9); diff --git a/javatests/google/registry/flows/FlowTestCase.java b/javatests/google/registry/flows/FlowTestCase.java index dfedd6629..c3f16dc2b 100644 --- a/javatests/google/registry/flows/FlowTestCase.java +++ b/javatests/google/registry/flows/FlowTestCase.java @@ -15,6 +15,7 @@ package google.registry.flows; import static com.google.common.base.MoreObjects.firstNonNull; +import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.collect.Sets.difference; import static com.google.common.truth.Truth.assertThat; import static google.registry.flows.EppXmlTransformer.marshal; @@ -46,6 +47,7 @@ import google.registry.model.ofy.Ofy; import google.registry.model.poll.PollMessage; import google.registry.model.reporting.HistoryEntry; import google.registry.model.tmch.ClaimsListShard.ClaimsListSingleton; +import google.registry.monitoring.whitebox.EppMetric; import google.registry.testing.AppEngineRule; import google.registry.testing.EppLoader; import google.registry.testing.ExceptionRule; @@ -96,6 +98,8 @@ public abstract class FlowTestCase extends ShardableTestCase { protected TransportCredentials credentials = new PasswordOnlyTransportCredentials(); protected EppRequestSource eppRequestSource = EppRequestSource.UNIT_TEST; + private EppMetric.Builder eppMetricBuilder; + @Before public void init() throws Exception { sessionMetadata = new HttpSessionMetadata(new FakeHttpSession()); @@ -123,6 +127,11 @@ public abstract class FlowTestCase extends ShardableTestCase { return eppLoader.getEpp(); } + protected EppMetric getEppMetric() { + checkNotNull(eppMetricBuilder, "Run the flow first before checking EPP metrics"); + return eppMetricBuilder.build(); + } + protected String readFile(String filename) { return readResourceUtf8(getClass(), "testdata/" + filename); } @@ -273,6 +282,7 @@ public abstract class FlowTestCase extends ShardableTestCase { private EppOutput runFlowInternal(CommitMode commitMode, UserPrivileges userPrivileges) throws Exception { + eppMetricBuilder = EppMetric.builderForRequest("request-id-1", clock); // Assert that the xml triggers the flow we expect. assertThat(FlowPicker.getFlowClass(eppLoader.getEpp())) .isEqualTo(new TypeInstantiator(getClass()){}.getExactType()); @@ -293,7 +303,7 @@ public abstract class FlowTestCase extends ShardableTestCase { .build()) .build() .flowRunner() - .run(); + .run(eppMetricBuilder); } /** Run a flow and marshal the result to EPP, or throw if it doesn't validate. */ diff --git a/javatests/google/registry/flows/domain/DomainCreateFlowTest.java b/javatests/google/registry/flows/domain/DomainCreateFlowTest.java index 1b6dbfda2..d60614556 100644 --- a/javatests/google/registry/flows/domain/DomainCreateFlowTest.java +++ b/javatests/google/registry/flows/domain/DomainCreateFlowTest.java @@ -125,6 +125,7 @@ import google.registry.model.registrar.Registrar; import google.registry.model.registry.Registry; import google.registry.model.registry.Registry.TldState; import google.registry.model.reporting.HistoryEntry; +import google.registry.monitoring.whitebox.EppMetric; import google.registry.testing.DatastoreHelper; import google.registry.testing.TaskQueueHelper.TaskMatcher; import java.util.Map; @@ -1996,4 +1997,13 @@ public class DomainCreateFlowTest extends ResourceFlowTestCase