Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DR-3476] Include additional fields in DRS getAccessURL Bard logs #1753

Merged
merged 3 commits into from
Aug 5, 2024
Merged

[DR-3476] Include additional fields in DRS getAccessURL Bard logs #1753

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
93f996f
Include more info in DRS getAccessURL Bard logs
samanehsan Jul 31, 2024
df6f95a
Add more fields to Bard logging
samanehsan Aug 5, 2024
69adc4c
code review comments
samanehsan Aug 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/main/java/bio/terra/app/usermetrics/BardEventProperties.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,10 @@ private BardEventProperties() {}
public static final String BILLING_PROFILE_ID_FIELD_NAME = "billingProfileId";
public static final String METHOD_FIELD_NAME = "method";
public static final String PATH_FIELD_NAME = "path";
public static final String SNAPSHOT_ID_FIELD_NAME = "snapshotId";
public static final String SNAPSHOT_NAME_FIELD_NAME = "snapshotName";

public static final String DATASET_ID_FIELD_NAME = "datasetId";
public static final String DATASET_NAME_FIELD_NAME = "datasetName";
public static final String CLOUD_PLATFORM_FIELD_NAME = "cloudPlatform";
}
49 changes: 48 additions & 1 deletion src/main/java/bio/terra/service/filedata/DrsService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
import bio.terra.app.controller.exception.TooManyRequestsException;
import bio.terra.app.logging.PerformanceLogger;
import bio.terra.app.model.GoogleRegion;
import bio.terra.app.usermetrics.BardEventProperties;
import bio.terra.app.usermetrics.UserLoggingMetrics;
import bio.terra.common.CloudPlatformWrapper;
import bio.terra.common.FutureUtils;
import bio.terra.common.exception.FeatureNotImplementedException;
Expand Down Expand Up @@ -111,6 +113,7 @@ public class DrsService {
private final DrsDao drsDao;
private final DrsMetricsService drsMetricsService;
private final AsyncTaskExecutor executor;
private final UserLoggingMetrics loggingMetrics;

private final Map<UUID, SnapshotProject> snapshotProjectsCache =
Collections.synchronizedMap(new PassiveExpiringMap<>(15, TimeUnit.MINUTES));
Expand All @@ -133,7 +136,8 @@ public DrsService(
EcmConfiguration ecmConfiguration,
DrsDao drsDao,
DrsMetricsService drsMetricsService,
@Qualifier("drsResolutionThreadpool") AsyncTaskExecutor executor) {
@Qualifier("drsResolutionThreadpool") AsyncTaskExecutor executor,
UserLoggingMetrics loggingMetrics) {
this.snapshotService = snapshotService;
this.fileService = fileService;
this.drsIdService = drsIdService;
Expand All @@ -148,6 +152,7 @@ public DrsService(
this.drsDao = drsDao;
this.drsMetricsService = drsMetricsService;
this.executor = executor;
this.loggingMetrics = loggingMetrics;
}

private class DrsRequestResource implements AutoCloseable {
Expand Down Expand Up @@ -486,6 +491,8 @@ private DRSAccessURL getAccessURL(

BillingProfileModel billingProfileModel = cachedSnapshot.datasetBillingProfileModel;

logAdditionalProperties(cachedSnapshot);

assertAccessMethodMatchingAccessId(accessId, drsObject);

FSFile fsFile;
Expand All @@ -510,6 +517,24 @@ private DRSAccessURL getAccessURL(
}
}

/**
* Include additional properties in the event log sent to Bard
*
* @param cachedSnapshot
*/
private void logAdditionalProperties(SnapshotCacheResult cachedSnapshot) {
loggingMetrics.set(BardEventProperties.DATASET_ID_FIELD_NAME, cachedSnapshot.getDatasetId());
loggingMetrics.set(
BardEventProperties.DATASET_NAME_FIELD_NAME, cachedSnapshot.getDatasetName());
loggingMetrics.set(BardEventProperties.SNAPSHOT_ID_FIELD_NAME, cachedSnapshot.getId());
loggingMetrics.set(BardEventProperties.SNAPSHOT_NAME_FIELD_NAME, cachedSnapshot.getName());
loggingMetrics.set(
BardEventProperties.BILLING_PROFILE_ID_FIELD_NAME,
cachedSnapshot.getSnapshotBillingProfileId());
loggingMetrics.set(
BardEventProperties.CLOUD_PLATFORM_FIELD_NAME, cachedSnapshot.getCloudPlatform());
}

@VisibleForTesting
DrsId resolveDrsObjectId(String drsObjectId) {
// If the drsObjectId is not a TDR generated DRS ID, then assume it's an alias and resolve it
Expand Down Expand Up @@ -985,16 +1010,20 @@ static <R> List<R> extractDistinctListOfDrsObjectValues(
@VisibleForTesting
static class SnapshotCacheResult {
private final UUID id;
private final String name;
private final boolean isSelfHosted;
private final BillingProfileModel datasetBillingProfileModel;
private final UUID snapshotBillingProfileId;
private final CloudPlatform cloudPlatform;
private final String googleProjectId;
private final String datasetProjectId;
private final UUID datasetId;
private final String datasetName;
private final boolean globalFileIds;

public SnapshotCacheResult(Snapshot snapshot) {
this.id = snapshot.getId();
this.name = snapshot.getName();
this.isSelfHosted = snapshot.isSelfHosted();
this.globalFileIds = snapshot.hasGlobalFileIds();
this.datasetBillingProfileModel =
Expand All @@ -1013,14 +1042,32 @@ public SnapshotCacheResult(Snapshot snapshot) {
} else {
this.datasetProjectId = null;
}
this.datasetId = snapshot.getSourceDataset().getId();
this.datasetName = snapshot.getSourceDataset().getName();
}

public UUID getId() {
return this.id;
}

public String getName() {
return this.name;
}

public UUID getDatasetId() {
return this.datasetId;
}

public String getDatasetName() {
return this.datasetName;
}

public UUID getSnapshotBillingProfileId() {
return snapshotBillingProfileId;
}

public CloudPlatform getCloudPlatform() {
return this.cloudPlatform;
}
}
}
22 changes: 20 additions & 2 deletions src/test/java/bio/terra/app/usermetrics/UserMetricsInterceptorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import bio.terra.common.fixtures.AuthenticationFixtures;
import bio.terra.common.iam.AuthenticatedUserRequest;
import bio.terra.common.iam.AuthenticatedUserRequestFactory;
import bio.terra.model.CloudPlatform;
import jakarta.servlet.http.HttpServletRequest;
import jakarta.servlet.http.HttpServletResponse;
import java.util.List;
Expand Down Expand Up @@ -69,6 +70,7 @@ class UserMetricsInterceptorTest {

@BeforeEach
void setUp() {
eventProperties.get().clear();
when(metricsConfig.ignorePaths()).thenReturn(List.of());
when(metricsConfig.appId()).thenReturn(APP_ID);
when(metricsConfig.bardBasePath()).thenReturn(BARD_BASE_PATH);
Expand Down Expand Up @@ -125,9 +127,20 @@ void testSendEvent() throws Exception {
}

@Test
void testSendEventWithBillingProfileId() throws Exception {
void testSendEventWithAdditionalProperties() throws Exception {
UUID datasetId = UUID.randomUUID();
String datasetName = "datasetName";
UUID snapshotId = UUID.randomUUID();
String snapshotName = "snapshotName";
String billingProfileId = UUID.randomUUID().toString();
CloudPlatform cloudPlatform = CloudPlatform.GCP;

eventProperties.set(BardEventProperties.DATASET_ID_FIELD_NAME, datasetId);
eventProperties.set(BardEventProperties.DATASET_NAME_FIELD_NAME, datasetName);
eventProperties.set(BardEventProperties.SNAPSHOT_ID_FIELD_NAME, snapshotId);
eventProperties.set(BardEventProperties.SNAPSHOT_NAME_FIELD_NAME, snapshotName);
eventProperties.set(BardEventProperties.BILLING_PROFILE_ID_FIELD_NAME, billingProfileId);
eventProperties.set(BardEventProperties.CLOUD_PLATFORM_FIELD_NAME, cloudPlatform);

mockRequestAuth(request);

Expand All @@ -142,7 +155,12 @@ void testSendEventWithBillingProfileId() throws Exception {
Map.of(
BardEventProperties.METHOD_FIELD_NAME, METHOD.toUpperCase(),
BardEventProperties.PATH_FIELD_NAME, REQUEST_URI,
BardEventProperties.BILLING_PROFILE_ID_FIELD_NAME, billingProfileId),
BardEventProperties.DATASET_ID_FIELD_NAME, datasetId,
BardEventProperties.DATASET_NAME_FIELD_NAME, datasetName,
BardEventProperties.SNAPSHOT_ID_FIELD_NAME, snapshotId,
BardEventProperties.SNAPSHOT_NAME_FIELD_NAME, snapshotName,
BardEventProperties.BILLING_PROFILE_ID_FIELD_NAME, billingProfileId,
BardEventProperties.CLOUD_PLATFORM_FIELD_NAME, cloudPlatform),
APP_ID,
DNS_NAME)));

Expand Down
5 changes: 4 additions & 1 deletion src/test/java/bio/terra/service/filedata/DrsServiceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import bio.terra.app.model.AzureRegion;
import bio.terra.app.model.CloudRegion;
import bio.terra.app.model.GoogleRegion;
import bio.terra.app.usermetrics.UserLoggingMetrics;
import bio.terra.common.TestUtils;
import bio.terra.common.UriUtils;
import bio.terra.common.category.Unit;
Expand Down Expand Up @@ -137,6 +138,7 @@ class DrsServiceTest {
@Mock private DrsDao drsDao;
@Mock private ApplicationConfiguration appConfig;
@Mock private DrsMetricsService drsMetricsService;
@Mock private UserLoggingMetrics loggingMetrics;

private DrsIdService drsIdService;
private DrsService drsService;
Expand Down Expand Up @@ -176,7 +178,8 @@ void before() throws Exception {
ecmConfiguration,
drsDao,
drsMetricsService,
new SimpleAsyncTaskExecutor()));
new SimpleAsyncTaskExecutor(),
loggingMetrics));
when(jobService.getActivePodCount()).thenReturn(1);
when(drsConfiguration.maxDrsLookups()).thenReturn(1);

Expand Down
Loading