mirror of
https://github.com/google/nomulus.git
synced 2025-05-13 16:07:15 +02:00
Only inject EPP metric builder in a single place
This fixes recording of number of attempts and command name on EPP flows, which was broken because a separate metric builder was being injected in two places, EppController and FlowRunner, with the one injected into FlowRunner being discarded rather than having changes applied to the same instance as in EppController. This also adds a test that the metric is created successfully inside a flow. Note that tests already exist for EppController to ensure that the metric is recorded correctly. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=152306596
This commit is contained in:
mcilwain
Ben McIlwain
parent
794743c7bc
commit
4606b1d08e
7 changed files with 58 additions and 36 deletions
|
|
@ -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());
|
||||
|
|
|
|||
|
|
@ -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<EppOutput>() {
|
||||
@Override
|
||||
public EppOutput run() {
|
||||
metric.incrementAttempts();
|
||||
eppMetricBuilder.incrementAttempts();
|
||||
try {
|
||||
EppOutput output = EppOutput.create(flowProvider.get().run());
|
||||
if (isDryRun) {
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
|
|
|
|||
|
|
@ -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.<String>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.<String, String>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.<String, String>of());
|
||||
flowRunner.inputXmlBytes = domainCreateXml.getBytes(UTF_8);
|
||||
flowRunner.run();
|
||||
flowRunner.run(eppMetricBuilder);
|
||||
String logMessage = findLogMessageByPrefix(handler, "EPP Command\n\t");
|
||||
List<String> lines = Splitter.on("\n\t").splitToList(logMessage);
|
||||
assertThat(lines.size()).named("number of lines in log message").isAtLeast(9);
|
||||
|
|
|
|||
|
|
@ -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<F extends Flow> 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<F extends Flow> 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<F extends Flow> 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<F>(getClass()){}.getExactType());
|
||||
|
|
@ -293,7 +303,7 @@ public abstract class FlowTestCase<F extends Flow> extends ShardableTestCase {
|
|||
.build())
|
||||
.build()
|
||||
.flowRunner()
|
||||
.run();
|
||||
.run(eppMetricBuilder);
|
||||
}
|
||||
|
||||
/** Run a flow and marshal the result to EPP, or throw if it doesn't validate. */
|
||||
|
|
|
|||
|
|
@ -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<DomainCreateFlow,
|
|||
runFlow();
|
||||
assertIcannReportingActivityFieldLogged("srs-dom-create");
|
||||
}
|
||||
|
||||
@Test
|
||||
public void testEppMetric_isSuccessfullyCreated() throws Exception {
|
||||
persistContactsAndHosts();
|
||||
runFlow();
|
||||
EppMetric eppMetric = getEppMetric();
|
||||
assertThat(eppMetric.getCommandName()).hasValue("DomainCreate");
|
||||
assertThat(eppMetric.getAttempts()).isEqualTo(1);
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue