Fix BigQuery data set name handling in activity reporting (#1361)

* Fix BigQuery data set name handling in activity reporting

This is not a constant (as it depends on runtime state), so it can't be named
using UPPER_SNAKE_CASE. Additionally, it's not good practice to use field
initialization when there's logic depending on runtime state involved. So this
PR changes the class to use constructor injection and moves the logic into the
constructor.

* Add fix for ICANN reporting provide

* Extract out ICANN reporting data set

* Inject TransactionManager

* Make TransactionInfo static (per Mike)

* Use ofyTm() in BackupTestStore

* Revert extraneous formatting

* Use auditedOfy in CommitLogMutationTest
This commit is contained in:
Ben McIlwain 2021-10-05 15:11:03 -04:00 committed by GitHub
parent 57e58ce8b7
commit fff63e3b2a
11 changed files with 103 additions and 56 deletions

View file

@ -21,6 +21,7 @@ import static google.registry.config.RegistryConfig.getHibernateHikariIdleTimeou
import static google.registry.config.RegistryConfig.getHibernateHikariMaximumPoolSize;
import static google.registry.config.RegistryConfig.getHibernateHikariMinimumIdle;
import static google.registry.config.RegistryConfig.getHibernateLogSqlQueries;
import static google.registry.persistence.transaction.TransactionManagerFactory.tm;
import com.google.api.client.auth.oauth2.Credential;
import com.google.common.annotations.VisibleForTesting;
@ -34,6 +35,7 @@ import google.registry.config.RegistryConfig.Config;
import google.registry.persistence.transaction.CloudSqlCredentialSupplier;
import google.registry.persistence.transaction.JpaTransactionManager;
import google.registry.persistence.transaction.JpaTransactionManagerImpl;
import google.registry.persistence.transaction.TransactionManager;
import google.registry.privileges.secretmanager.SqlCredential;
import google.registry.privileges.secretmanager.SqlCredentialStore;
import google.registry.privileges.secretmanager.SqlUser;
@ -206,6 +208,12 @@ public abstract class PersistenceModule {
}
}
@Provides
@Singleton
static TransactionManager provideTransactionManager() {
return tm();
}
@Provides
@Singleton
@AppEngineJpaTm

View file

@ -98,14 +98,16 @@ public class JpaTransactionManagerImpl implements JpaTransactionManager {
// EntityManagerFactory is thread safe.
private final EntityManagerFactory emf;
private final Clock clock;
// TODO(b/177588434): Investigate alternatives for managing transaction information. ThreadLocal
// adds an unnecessary restriction that each request has to be processed by one thread
// synchronously.
private final ThreadLocal<TransactionInfo> transactionInfo =
private static final ThreadLocal<TransactionInfo> transactionInfo =
ThreadLocal.withInitial(TransactionInfo::new);
// If this value is present, use it to determine whether or not to replay SQL transactions to
// Datastore, rather than using the schedule stored in Datastore.
private static ThreadLocal<Optional<Boolean>> replaySqlToDatastoreOverrideForTest =
private static final ThreadLocal<Optional<Boolean>> replaySqlToDatastoreOverrideForTest =
ThreadLocal.withInitial(Optional::empty);
public JpaTransactionManagerImpl(EntityManagerFactory emf, Clock clock) {

View file

@ -16,6 +16,7 @@ package google.registry.reporting.icann;
import static google.registry.persistence.transaction.TransactionManagerFactory.tm;
import static google.registry.reporting.icann.IcannReportingModule.DATASTORE_EXPORT_DATA_SET;
import static google.registry.reporting.icann.IcannReportingModule.ICANN_REPORTING_DATA_SET;
import static google.registry.reporting.icann.QueryBuilderUtils.getQueryFromFile;
import static google.registry.reporting.icann.QueryBuilderUtils.getTableName;
@ -23,6 +24,7 @@ import com.google.common.collect.ImmutableMap;
import google.registry.config.RegistryConfig.Config;
import google.registry.util.SqlTemplate;
import javax.inject.Inject;
import javax.inject.Named;
import org.joda.time.LocalDate;
import org.joda.time.YearMonth;
import org.joda.time.format.DateTimeFormat;
@ -38,29 +40,32 @@ public final class ActivityReportingQueryBuilder implements QueryBuilder {
static final String EPP_METRICS = "epp_metrics";
static final String WHOIS_COUNTS = "whois_counts";
static final String ACTIVITY_REPORT_AGGREGATION = "activity_report_aggregation";
final String BIGQUERY_DATA_SET = tm().isOfy() ? "icann_reporting" : "cloud_sql_icann_reporting";
private final String projectId;
private final DnsCountQueryCoordinator dnsCountQueryCoordinator;
private final String icannReportingDataSet;
@Inject
@Config("projectId")
String projectId;
@Inject DnsCountQueryCoordinator dnsCountQueryCoordinator;
@Inject
ActivityReportingQueryBuilder() {}
ActivityReportingQueryBuilder(
@Config("projectId") String projectId,
@Named(ICANN_REPORTING_DATA_SET) String icannReportingDataSet,
DnsCountQueryCoordinator dnsCountQueryCoordinator) {
this.projectId = projectId;
this.dnsCountQueryCoordinator = dnsCountQueryCoordinator;
this.icannReportingDataSet = icannReportingDataSet;
}
/** Returns the aggregate query which generates the activity report from the saved view. */
@Override
public String getReportQuery(YearMonth yearMonth) {
return String.format(
"#standardSQL\nSELECT * FROM `%s.%s.%s`",
projectId, BIGQUERY_DATA_SET, getTableName(ACTIVITY_REPORT_AGGREGATION, yearMonth));
projectId, icannReportingDataSet, getTableName(ACTIVITY_REPORT_AGGREGATION, yearMonth));
}
/** Sets the month we're doing activity reporting for, and returns the view query map. */
@Override
public ImmutableMap<String, String> getViewQueryMap(YearMonth yearMonth) {
LocalDate firstDayOfMonth = yearMonth.toLocalDate(1);
// The pattern-matching is inclusive, so we subtract 1 day to only report that month's data.
LocalDate lastDayOfMonth = yearMonth.toLocalDate(1).plusMonths(1).minusDays(1);
@ -102,7 +107,7 @@ public final class ActivityReportingQueryBuilder implements QueryBuilder {
String eppQuery =
SqlTemplate.create(getQueryFromFile("epp_metrics.sql"))
.put("PROJECT_ID", projectId)
.put("ICANN_REPORTING_DATA_SET", BIGQUERY_DATA_SET)
.put("ICANN_REPORTING_DATA_SET", icannReportingDataSet)
.put("MONTHLY_LOGS_TABLE", getTableName(MONTHLY_LOGS, yearMonth))
// All metadata logs for reporting come from google.registry.flows.FlowReporter.
.put(
@ -114,7 +119,7 @@ public final class ActivityReportingQueryBuilder implements QueryBuilder {
String whoisQuery =
SqlTemplate.create(getQueryFromFile("whois_counts.sql"))
.put("PROJECT_ID", projectId)
.put("ICANN_REPORTING_DATA_SET", BIGQUERY_DATA_SET)
.put("ICANN_REPORTING_DATA_SET", icannReportingDataSet)
.put("MONTHLY_LOGS_TABLE", getTableName(MONTHLY_LOGS, yearMonth))
.build();
queriesBuilder.put(getTableName(WHOIS_COUNTS, yearMonth), whoisQuery);
@ -129,7 +134,7 @@ public final class ActivityReportingQueryBuilder implements QueryBuilder {
.put(
"REGISTRAR_OPERATING_STATUS_TABLE",
getTableName(REGISTRAR_OPERATING_STATUS, yearMonth))
.put("ICANN_REPORTING_DATA_SET", BIGQUERY_DATA_SET)
.put("ICANN_REPORTING_DATA_SET", icannReportingDataSet)
.put("DNS_COUNTS_TABLE", getTableName(DNS_COUNTS, yearMonth))
.put("EPP_METRICS_TABLE", getTableName(EPP_METRICS, yearMonth))
.put("WHOIS_COUNTS_TABLE", getTableName(WHOIS_COUNTS, yearMonth));
@ -147,7 +152,7 @@ public final class ActivityReportingQueryBuilder implements QueryBuilder {
return queriesBuilder.build();
}
public void prepareForQuery(YearMonth yearMonth) throws InterruptedException {
void prepareForQuery(YearMonth yearMonth) throws InterruptedException {
dnsCountQueryCoordinator.prepareForQuery(yearMonth);
}
}

View file

@ -14,7 +14,6 @@
package google.registry.reporting.icann;
import static google.registry.persistence.transaction.TransactionManagerFactory.tm;
import static google.registry.request.RequestParameters.extractOptionalParameter;
import static google.registry.request.RequestParameters.extractRequiredParameter;
import static google.registry.request.RequestParameters.extractSetOfEnumParameters;
@ -24,9 +23,11 @@ import com.google.common.util.concurrent.MoreExecutors;
import dagger.Module;
import dagger.Provides;
import google.registry.bigquery.BigqueryConnection;
import google.registry.persistence.transaction.TransactionManager;
import google.registry.request.HttpException.BadRequestException;
import google.registry.request.Parameter;
import java.util.Optional;
import javax.inject.Named;
import javax.servlet.http.HttpServletRequest;
import org.joda.time.Duration;
@ -42,8 +43,7 @@ public final class IcannReportingModule {
static final String PARAM_SUBDIR = "subdir";
static final String PARAM_REPORT_TYPES = "reportTypes";
static final String ICANN_REPORTING_DATA_SET =
tm().isOfy() ? "icann_reporting" : "cloud_sql_icann_reporting";
static final String ICANN_REPORTING_DATA_SET = "icannReportingDataSet";
static final String DATASTORE_EXPORT_DATA_SET = "latest_datastore_export";
static final String MANIFEST_FILE_NAME = "MANIFEST.txt";
@ -88,11 +88,12 @@ public final class IcannReportingModule {
*/
@Provides
static BigqueryConnection provideBigqueryConnection(
BigqueryConnection.Builder bigQueryConnectionBuilder) {
BigqueryConnection.Builder bigQueryConnectionBuilder,
@Named(ICANN_REPORTING_DATA_SET) String icannReportingDataSet) {
try {
return bigQueryConnectionBuilder
.setExecutorService(MoreExecutors.newDirectExecutorService())
.setDatasetId(ICANN_REPORTING_DATA_SET)
.setDatasetId(icannReportingDataSet)
.setOverwrite(true)
.setPollInterval(Duration.standardSeconds(1))
.build();
@ -100,4 +101,10 @@ public final class IcannReportingModule {
throw new RuntimeException("Could not initialize BigqueryConnection!", e);
}
}
@Provides
@Named(ICANN_REPORTING_DATA_SET)
static String provideIcannReportingDataSet(TransactionManager tm) {
return tm.isOfy() ? "icann_reporting" : "cloud_sql_icann_reporting";
}
}

View file

@ -23,6 +23,7 @@ import com.google.common.collect.ImmutableMap;
import google.registry.config.RegistryConfig.Config;
import google.registry.util.SqlTemplate;
import javax.inject.Inject;
import javax.inject.Named;
import org.joda.time.DateTime;
import org.joda.time.LocalTime;
import org.joda.time.YearMonth;
@ -34,9 +35,16 @@ import org.joda.time.format.DateTimeFormatter;
*/
public final class TransactionsReportingQueryBuilder implements QueryBuilder {
@Inject @Config("projectId") String projectId;
final String projectId;
private final String icannReportingDataSet;
@Inject TransactionsReportingQueryBuilder() {}
@Inject
TransactionsReportingQueryBuilder(
@Config("projectId") String projectId,
@Named(ICANN_REPORTING_DATA_SET) String icannReportingDataSet) {
this.projectId = projectId;
this.icannReportingDataSet = icannReportingDataSet;
}
static final String TRANSACTIONS_REPORT_AGGREGATION = "transactions_report_aggregation";
static final String REGISTRAR_IANA_ID = "registrar_iana_id";
@ -51,9 +59,7 @@ public final class TransactionsReportingQueryBuilder implements QueryBuilder {
public String getReportQuery(YearMonth yearMonth) {
return String.format(
"#standardSQL\nSELECT * FROM `%s.%s.%s`",
projectId,
ICANN_REPORTING_DATA_SET,
getTableName(TRANSACTIONS_REPORT_AGGREGATION, yearMonth));
projectId, icannReportingDataSet, getTableName(TRANSACTIONS_REPORT_AGGREGATION, yearMonth));
}
/** Sets the month we're doing transactions reporting for, and returns the view query map. */
@ -151,7 +157,7 @@ public final class TransactionsReportingQueryBuilder implements QueryBuilder {
.put("PROJECT_ID", projectId)
.put("DATASTORE_EXPORT_DATA_SET", DATASTORE_EXPORT_DATA_SET)
.put("REGISTRY_TABLE", "Registry")
.put("ICANN_REPORTING_DATA_SET", ICANN_REPORTING_DATA_SET)
.put("ICANN_REPORTING_DATA_SET", icannReportingDataSet)
.put("REGISTRAR_IANA_ID_TABLE", getTableName(REGISTRAR_IANA_ID, yearMonth))
.put("TOTAL_DOMAINS_TABLE", getTableName(TOTAL_DOMAINS, yearMonth))
.put("TOTAL_NAMESERVERS_TABLE", getTableName(TOTAL_NAMESERVERS, yearMonth))

View file

@ -16,7 +16,7 @@ package google.registry.beam.initsql;
import static com.google.common.base.Preconditions.checkState;
import static google.registry.model.ofy.ObjectifyService.auditedOfy;
import static google.registry.persistence.transaction.TransactionManagerFactory.tm;
import static google.registry.persistence.transaction.TransactionManagerFactory.ofyTm;
import com.google.appengine.api.datastore.DatastoreService;
import com.google.appengine.api.datastore.DatastoreServiceFactory;
@ -75,7 +75,8 @@ public final class BackupTestStore implements AutoCloseable {
/** Returns the timestamp of the transaction. */
long transact(Iterable<Object> deletes, Iterable<Object> newOrUpdated) {
long timestamp = fakeClock.nowUtc().getMillis();
tm().transact(
ofyTm()
.transact(
() -> {
auditedOfy().delete().entities(deletes);
auditedOfy().save().entities(newOrUpdated);
@ -91,7 +92,7 @@ public final class BackupTestStore implements AutoCloseable {
@SafeVarargs
public final long insertOrUpdate(Object... entities) {
long timestamp = fakeClock.nowUtc().getMillis();
tm().transact(() -> auditedOfy().save().entities(entities).now());
ofyTm().transact(() -> auditedOfy().save().entities(entities).now());
fakeClock.advanceOneMilli();
return timestamp;
}
@ -100,7 +101,7 @@ public final class BackupTestStore implements AutoCloseable {
@SafeVarargs
public final long delete(Object... entities) {
long timestamp = fakeClock.nowUtc().getMillis();
tm().transact(() -> auditedOfy().delete().entities(entities).now());
ofyTm().transact(() -> auditedOfy().delete().entities(entities).now());
fakeClock.advanceOneMilli();
return timestamp;
}

View file

@ -16,7 +16,6 @@ package google.registry.model.ofy;
import static com.google.common.truth.Truth.assertThat;
import static google.registry.model.ofy.ObjectifyService.auditedOfy;
import static google.registry.persistence.transaction.TransactionManagerFactory.tm;
import static google.registry.testing.DatabaseHelper.createTld;
import com.google.appengine.api.datastore.Entity;
@ -66,7 +65,7 @@ public class CommitLogMutationTest {
Entity rawEntity = convertToEntityInTxn(someObject);
// Needs to be in a transaction so that registry-saving-to-entity will work.
CommitLogMutation mutation =
tm().transact(() -> CommitLogMutation.create(manifestKey, someObject));
auditedOfy().transact(() -> CommitLogMutation.create(manifestKey, someObject));
assertThat(Key.create(mutation))
.isEqualTo(CommitLogMutation.createKey(manifestKey, Key.create(someObject)));
assertThat(mutation.getEntity()).isEqualTo(rawEntity);
@ -86,6 +85,6 @@ public class CommitLogMutationTest {
}
private static Entity convertToEntityInTxn(final ImmutableObject object) {
return tm().transact(() -> auditedOfy().save().toEntity(object));
return auditedOfy().transact(() -> auditedOfy().save().toEntity(object));
}
}

View file

@ -39,18 +39,17 @@ class ActivityReportingQueryBuilderTest {
private final YearMonth yearMonth = new YearMonth(2017, 9);
private ActivityReportingQueryBuilder getQueryBuilder() {
ActivityReportingQueryBuilder queryBuilder = new ActivityReportingQueryBuilder();
queryBuilder.projectId = "domain-registry-alpha";
queryBuilder.dnsCountQueryCoordinator =
private ActivityReportingQueryBuilder createQueryBuilder(String datasetName) {
return new ActivityReportingQueryBuilder(
"domain-registry-alpha",
datasetName,
new BasicDnsCountQueryCoordinator(
new BasicDnsCountQueryCoordinator.Params(null, queryBuilder.projectId));
return queryBuilder;
new BasicDnsCountQueryCoordinator.Params(null, "domain-registry-alpha")));
}
@TestOfyOnly
void testAggregateQueryMatch_datastore() {
ActivityReportingQueryBuilder queryBuilder = getQueryBuilder();
ActivityReportingQueryBuilder queryBuilder = createQueryBuilder("icann_reporting");
assertThat(queryBuilder.getReportQuery(yearMonth))
.isEqualTo(
"#standardSQL\nSELECT * FROM "
@ -58,8 +57,8 @@ class ActivityReportingQueryBuilderTest {
}
@TestSqlOnly
void testAggregateQueryMatch_cloud_sql() {
ActivityReportingQueryBuilder queryBuilder = getQueryBuilder();
void testAggregateQueryMatch_cloudSql() {
ActivityReportingQueryBuilder queryBuilder = createQueryBuilder("cloud_sql_icann_reporting");
assertThat(queryBuilder.getReportQuery(yearMonth))
.isEqualTo(
"#standardSQL\n"
@ -78,7 +77,7 @@ class ActivityReportingQueryBuilderTest {
ActivityReportingQueryBuilder.WHOIS_COUNTS,
ActivityReportingQueryBuilder.ACTIVITY_REPORT_AGGREGATION);
ActivityReportingQueryBuilder queryBuilder = getQueryBuilder();
ActivityReportingQueryBuilder queryBuilder = createQueryBuilder("icann_reporting");
ImmutableMap<String, String> actualQueries = queryBuilder.getViewQueryMap(yearMonth);
for (String queryName : expectedQueryNames) {
String actualTableName = String.format("%s_201709", queryName);
@ -99,7 +98,7 @@ class ActivityReportingQueryBuilderTest {
ActivityReportingQueryBuilder.WHOIS_COUNTS,
ActivityReportingQueryBuilder.ACTIVITY_REPORT_AGGREGATION);
ActivityReportingQueryBuilder queryBuilder = getQueryBuilder();
ActivityReportingQueryBuilder queryBuilder = createQueryBuilder("cloud_sql_icann_reporting");
ImmutableMap<String, String> actualQueries = queryBuilder.getViewQueryMap(yearMonth);
for (String queryName : expectedQueryNames) {
String actualTableName = String.format("%s_201709", queryName);

View file

@ -18,34 +18,55 @@ import static com.google.common.truth.Truth.assertThat;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;
import google.registry.persistence.transaction.JpaTestExtensions;
import google.registry.persistence.transaction.JpaTestExtensions.JpaUnitTestExtension;
import javax.servlet.http.HttpServletRequest;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.RegisterExtension;
/** Unit tests for {@link google.registry.reporting.icann.IcannReportingModule}. */
class IcannReportingModuleTest {
@Test
void testProvideReportTypes() {
HttpServletRequest req = mock(HttpServletRequest.class);
@RegisterExtension
JpaUnitTestExtension jpaExtension = new JpaTestExtensions.Builder().buildUnitTestExtension();
@Test
void testProvideReportTypes_null() {
HttpServletRequest req = mock(HttpServletRequest.class);
when(req.getParameter("reportTypes")).thenReturn(null);
assertThat(IcannReportingModule.provideReportTypes(req))
.containsExactly(
IcannReportingModule.ReportType.ACTIVITY, IcannReportingModule.ReportType.TRANSACTIONS);
}
@Test
void testProvideReportTypes_empty() {
HttpServletRequest req = mock(HttpServletRequest.class);
when(req.getParameter("reportTypes")).thenReturn("");
assertThat(IcannReportingModule.provideReportTypes(req))
.containsExactly(
IcannReportingModule.ReportType.ACTIVITY, IcannReportingModule.ReportType.TRANSACTIONS);
}
@Test
void testProvideReportTypes_activity() {
HttpServletRequest req = mock(HttpServletRequest.class);
when(req.getParameter("reportTypes")).thenReturn("activity");
assertThat(IcannReportingModule.provideReportTypes(req))
.containsExactly(IcannReportingModule.ReportType.ACTIVITY);
}
@Test
void testProvideReportTypes_transactions() {
HttpServletRequest req = mock(HttpServletRequest.class);
when(req.getParameter("reportTypes")).thenReturn("transactions");
assertThat(IcannReportingModule.provideReportTypes(req))
.containsExactly(IcannReportingModule.ReportType.TRANSACTIONS);
}
@Test
void testProvideReportTypes_bothTypes() {
HttpServletRequest req = mock(HttpServletRequest.class);
when(req.getParameter("reportTypes")).thenReturn("activity,transactions");
assertThat(IcannReportingModule.provideReportTypes(req))
.containsExactly(

View file

@ -54,13 +54,12 @@ class IcannReportingStagerTest {
private IcannReportingStager createStager() {
IcannReportingStager action = new IcannReportingStager();
ActivityReportingQueryBuilder activityBuilder = new ActivityReportingQueryBuilder();
activityBuilder.projectId = "test-project";
activityBuilder.dnsCountQueryCoordinator = new BasicDnsCountQueryCoordinator(null);
ActivityReportingQueryBuilder activityBuilder =
new ActivityReportingQueryBuilder(
"test-project", "icann_reporting", new BasicDnsCountQueryCoordinator(null));
action.activityQueryBuilder = activityBuilder;
TransactionsReportingQueryBuilder transactionsBuilder = new TransactionsReportingQueryBuilder();
transactionsBuilder.projectId = "test-project";
action.transactionsQueryBuilder = transactionsBuilder;
action.transactionsQueryBuilder =
new TransactionsReportingQueryBuilder("test-project", "icann_reporting");
action.reportingBucket = "test-bucket";
action.bigquery = bigquery;
action.gcsUtils = gcsUtils;

View file

@ -27,8 +27,8 @@ class TransactionsReportingQueryBuilderTest {
private final YearMonth yearMonth = new YearMonth(2017, 9);
private TransactionsReportingQueryBuilder getQueryBuilder() {
TransactionsReportingQueryBuilder queryBuilder = new TransactionsReportingQueryBuilder();
queryBuilder.projectId = "domain-registry-alpha";
TransactionsReportingQueryBuilder queryBuilder =
new TransactionsReportingQueryBuilder("domain-registry-alpha", "icann_reporting");
return queryBuilder;
}