From efbd2ee96d71a9e7d07fe8efe2bc79a78f7c10cf Mon Sep 17 00:00:00 2001 From: Adina Shanholtz Date: Thu, 18 Jan 2024 11:56:38 -0800 Subject: [PATCH 01/14] add observability to quartzjob --- service/build.gradle | 1 + .../dataimport/pfb/PfbQuartzJob.java | 5 ++- .../dataimport/tdr/TdrManifestQuartzJob.java | 5 ++- .../jobexec/QuartzJob.java | 28 +++++++++++++++- .../metrics/MetricsConfig.java | 9 ++++++ .../metrics/MetricsDefinitions.java | 25 ++++++++++++++- .../dataimport/pfb/PfbQuartzJobE2ETest.java | 17 +++++++--- .../dataimport/pfb/PfbQuartzJobTest.java | 32 ++++++++++++++++--- .../dataimport/pfb/PfbTestUtils.java | 12 +++++-- .../dataimport/tdr/TdrTestSupport.java | 5 ++- 10 files changed, 122 insertions(+), 17 deletions(-) diff --git a/service/build.gradle b/service/build.gradle index 1a4b087f1..70eabc616 100644 --- a/service/build.gradle +++ b/service/build.gradle @@ -54,6 +54,7 @@ dependencies { implementation 'org.springframework.boot:spring-boot-starter-batch' implementation 'org.springframework.boot:spring-boot-starter-quartz' implementation 'org.springframework.boot:spring-boot-starter-actuator' + implementation 'org.springframework.boot:spring-boot-starter-aop' implementation 'io.micrometer:micrometer-registry-prometheus' implementation 'org.springframework.integration:spring-integration-jdbc' implementation 'org.aspectj:aspectjweaver' // required by spring-retry, not used directly diff --git a/service/src/main/java/org/databiosphere/workspacedataservice/dataimport/pfb/PfbQuartzJob.java b/service/src/main/java/org/databiosphere/workspacedataservice/dataimport/pfb/PfbQuartzJob.java index 6d469f2e5..724e96bf9 100644 --- a/service/src/main/java/org/databiosphere/workspacedataservice/dataimport/pfb/PfbQuartzJob.java +++ b/service/src/main/java/org/databiosphere/workspacedataservice/dataimport/pfb/PfbQuartzJob.java @@ -7,6 +7,7 @@ import static org.databiosphere.workspacedataservice.shared.model.Schedulable.ARG_URL; import bio.terra.pfb.PfbReader; +import io.micrometer.observation.ObservationRegistry; import java.net.URL; import java.util.Objects; import java.util.Optional; @@ -58,7 +59,9 @@ public PfbQuartzJob( RestClientRetry restClientRetry, BatchWriteService batchWriteService, ActivityLogger activityLogger, - @Value("${twds.instance.workspace-id}") UUID workspaceId) { + @Value("${twds.instance.workspace-id}") UUID workspaceId, + ObservationRegistry observationRegistry) { + super(observationRegistry); this.jobDao = jobDao; this.wsmDao = wsmDao; this.restClientRetry = restClientRetry; diff --git a/service/src/main/java/org/databiosphere/workspacedataservice/dataimport/tdr/TdrManifestQuartzJob.java b/service/src/main/java/org/databiosphere/workspacedataservice/dataimport/tdr/TdrManifestQuartzJob.java index 7c4027486..38215ea9a 100644 --- a/service/src/main/java/org/databiosphere/workspacedataservice/dataimport/tdr/TdrManifestQuartzJob.java +++ b/service/src/main/java/org/databiosphere/workspacedataservice/dataimport/tdr/TdrManifestQuartzJob.java @@ -10,6 +10,7 @@ import com.fasterxml.jackson.databind.ObjectMapper; import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.Multimap; +import io.micrometer.observation.ObservationRegistry; import java.io.File; import java.io.IOException; import java.net.MalformedURLException; @@ -69,7 +70,9 @@ public TdrManifestQuartzJob( BatchWriteService batchWriteService, ActivityLogger activityLogger, @Value("${twds.instance.workspace-id}") UUID workspaceId, - ObjectMapper mapper) { + ObjectMapper mapper, + ObservationRegistry observationRegistry) { + super(observationRegistry); this.jobDao = jobDao; this.wsmDao = wsmDao; this.restClientRetry = restClientRetry; diff --git a/service/src/main/java/org/databiosphere/workspacedataservice/jobexec/QuartzJob.java b/service/src/main/java/org/databiosphere/workspacedataservice/jobexec/QuartzJob.java index 6eb8166fe..b9b54fce2 100644 --- a/service/src/main/java/org/databiosphere/workspacedataservice/jobexec/QuartzJob.java +++ b/service/src/main/java/org/databiosphere/workspacedataservice/jobexec/QuartzJob.java @@ -1,8 +1,17 @@ package org.databiosphere.workspacedataservice.jobexec; +import static org.databiosphere.workspacedataservice.metrics.MetricsDefinitions.EVENT_JOB_FAILED; +import static org.databiosphere.workspacedataservice.metrics.MetricsDefinitions.EVENT_JOB_RUNNING; +import static org.databiosphere.workspacedataservice.metrics.MetricsDefinitions.EVENT_JOB_SUCCEEDED; +import static org.databiosphere.workspacedataservice.metrics.MetricsDefinitions.NAME_JOB_EXECUTION; +import static org.databiosphere.workspacedataservice.metrics.MetricsDefinitions.OBSERVE_JOB_EXECUTE; +import static org.databiosphere.workspacedataservice.metrics.MetricsDefinitions.TAG_JOB_ID; +import static org.databiosphere.workspacedataservice.metrics.MetricsDefinitions.TAG_JOB_TYPE; import static org.databiosphere.workspacedataservice.sam.BearerTokenFilter.ATTRIBUTE_NAME_TOKEN; import static org.databiosphere.workspacedataservice.shared.model.Schedulable.ARG_TOKEN; +import io.micrometer.observation.Observation; +import io.micrometer.observation.ObservationRegistry; import java.net.URL; import java.util.UUID; import org.databiosphere.workspacedataservice.dao.JobDao; @@ -29,6 +38,12 @@ // note this implements Quartz's `Job`, not WDS's own `Job` public abstract class QuartzJob implements Job { + private final ObservationRegistry observationRegistry; + + public QuartzJob(ObservationRegistry observationRegistry) { + this.observationRegistry = observationRegistry; + } + /** implementing classes are expected to be beans that inject a JobDao */ protected abstract JobDao getJobDao(); @@ -36,9 +51,16 @@ public abstract class QuartzJob implements Job { public void execute(JobExecutionContext context) throws org.quartz.JobExecutionException { // retrieve jobId UUID jobId = UUID.fromString(context.getJobDetail().getKey().getName()); - try { + Observation observation = + Observation.start(OBSERVE_JOB_EXECUTE, observationRegistry) + .contextualName(NAME_JOB_EXECUTION) + .lowCardinalityKeyValue(TAG_JOB_TYPE, getClass().getName()) + .highCardinalityKeyValue(TAG_JOB_ID, jobId.toString()); + + try (Observation.Scope scope = observation.openScope()) { // mark this job as running getJobDao().running(jobId); + observation.event(Observation.Event.of(EVENT_JOB_RUNNING)); // look for an auth token in the Quartz JobDataMap String authToken = getJobDataString(context.getMergedJobDataMap(), ARG_TOKEN); // and stash the auth token into job context @@ -50,11 +72,15 @@ public void execute(JobExecutionContext context) throws org.quartz.JobExecutionE executeInternal(jobId, context); // if we reached here, mark this job as successful getJobDao().succeeded(jobId); + observation.event(Observation.Event.of(EVENT_JOB_SUCCEEDED)); } catch (Exception e) { // on any otherwise-unhandled exception, mark the job as failed getJobDao().fail(jobId, e); + observation.error(e); + observation.event(Observation.Event.of(EVENT_JOB_FAILED)); } finally { JobContextHolder.destroy(); + observation.stop(); } } diff --git a/service/src/main/java/org/databiosphere/workspacedataservice/metrics/MetricsConfig.java b/service/src/main/java/org/databiosphere/workspacedataservice/metrics/MetricsConfig.java index 28ab8e723..9386ece6e 100644 --- a/service/src/main/java/org/databiosphere/workspacedataservice/metrics/MetricsConfig.java +++ b/service/src/main/java/org/databiosphere/workspacedataservice/metrics/MetricsConfig.java @@ -7,6 +7,8 @@ import com.google.common.collect.ImmutableList; import io.micrometer.core.instrument.MeterRegistry; import io.micrometer.core.instrument.Tag; +import io.micrometer.observation.ObservationRegistry; +import io.micrometer.observation.aop.ObservedAspect; import java.util.Set; import org.springframework.boot.actuate.autoconfigure.metrics.MeterRegistryCustomizer; import org.springframework.boot.info.BuildProperties; @@ -35,6 +37,7 @@ public MetricsConfig(BuildProperties buildProperties) { "jvm", "logback", "process", + "quartz", "spring", "system", "tomcat", @@ -59,4 +62,10 @@ MeterRegistryCustomizer addWdsVersionTag() { .add(Tag.of("wds.version", buildProperties.getVersion())) .build()); } + + // Observation support for metrics + @Bean + ObservedAspect observedAspect(ObservationRegistry observationRegistry) { + return new ObservedAspect(observationRegistry); + } } diff --git a/service/src/main/java/org/databiosphere/workspacedataservice/metrics/MetricsDefinitions.java b/service/src/main/java/org/databiosphere/workspacedataservice/metrics/MetricsDefinitions.java index d84acc996..80b991452 100644 --- a/service/src/main/java/org/databiosphere/workspacedataservice/metrics/MetricsDefinitions.java +++ b/service/src/main/java/org/databiosphere/workspacedataservice/metrics/MetricsDefinitions.java @@ -4,8 +4,9 @@ public final class MetricsDefinitions { private MetricsDefinitions() {} + // Record Metrics /** counter for column schema changes, i.e. "alter column" sql statements */ - public static final String COUNTER_COL_CHANGE = "column_change_datatype"; + public static final String COUNTER_COL_CHANGE = "column.change.datatype"; /** tag for a {@link org.databiosphere.workspacedataservice.shared.model.RecordType} */ public static final String TAG_RECORD_TYPE = "RecordType"; @@ -15,4 +16,26 @@ private MetricsDefinitions() {} /** tag for a record attribute name */ public static final String TAG_ATTRIBUTE_NAME = "AttributeName"; + + // Quartz Job Metrics + /** observable name for running job */ + public static final String OBSERVE_JOB_EXECUTE = "wds.job.execute"; + + /** event name for job succeeded */ + public static final String EVENT_JOB_SUCCEEDED = "job.succeeded"; + + /** event name for job running */ + public static final String EVENT_JOB_RUNNING = "job.running"; + + /** event name for job failed */ + public static final String EVENT_JOB_FAILED = "job.failed"; + + /** name for job execution (used in span) */ + public static final String NAME_JOB_EXECUTION = "job-execution"; + + /** tag for the type of job */ + public static final String TAG_JOB_TYPE = "jobType"; + + /** tag for the type of job */ + public static final String TAG_JOB_ID = "jobId"; } diff --git a/service/src/test/java/org/databiosphere/workspacedataservice/dataimport/pfb/PfbQuartzJobE2ETest.java b/service/src/test/java/org/databiosphere/workspacedataservice/dataimport/pfb/PfbQuartzJobE2ETest.java index f0392eca4..1d6c12770 100644 --- a/service/src/test/java/org/databiosphere/workspacedataservice/dataimport/pfb/PfbQuartzJobE2ETest.java +++ b/service/src/test/java/org/databiosphere/workspacedataservice/dataimport/pfb/PfbQuartzJobE2ETest.java @@ -12,6 +12,7 @@ import bio.terra.workspace.model.ResourceList; import com.google.common.collect.ImmutableMap; +import io.micrometer.observation.ObservationRegistry; import java.io.IOException; import java.math.BigDecimal; import java.util.List; @@ -70,6 +71,7 @@ class PfbQuartzJobE2ETest { @Autowired RecordOrchestratorService recordOrchestratorService; @Autowired ImportService importService; @Autowired InstanceService instanceService; + @Autowired ObservationRegistry observationRegistry; @MockBean SchedulerDao schedulerDao; @MockBean WorkspaceManagerDao wsmDao; @@ -127,7 +129,8 @@ void importTestResource() throws IOException, JobExecutionException { when(wsmDao.enumerateDataRepoSnapshotReferences(any(), anyInt(), anyInt())) .thenReturn(new ResourceList()); - buildPfbQuartzJob(jobDao, wsmDao, restClientRetry, batchWriteService, activityLogger) + buildPfbQuartzJob( + jobDao, wsmDao, restClientRetry, batchWriteService, activityLogger, observationRegistry) .execute(mockContext); /* the testAvroResource should insert: @@ -184,7 +187,8 @@ void importFourRowsResource() throws IOException, JobExecutionException { when(wsmDao.enumerateDataRepoSnapshotReferences(any(), anyInt(), anyInt())) .thenReturn(new ResourceList()); - buildPfbQuartzJob(jobDao, wsmDao, restClientRetry, batchWriteService, activityLogger) + buildPfbQuartzJob( + jobDao, wsmDao, restClientRetry, batchWriteService, activityLogger, observationRegistry) .execute(mockContext); /* the fourRowsAvroResource should insert: @@ -231,7 +235,8 @@ void numberPrecision() throws IOException, JobExecutionException { when(wsmDao.enumerateDataRepoSnapshotReferences(any(), anyInt(), anyInt())) .thenReturn(new ResourceList()); - buildPfbQuartzJob(jobDao, wsmDao, restClientRetry, batchWriteService, activityLogger) + buildPfbQuartzJob( + jobDao, wsmDao, restClientRetry, batchWriteService, activityLogger, observationRegistry) .execute(mockContext); // this record, within the precision.avro file, is known to have numbers with high decimal @@ -271,7 +276,8 @@ void importWithForwardRelations() throws IOException, JobExecutionException { when(wsmDao.enumerateDataRepoSnapshotReferences(any(), anyInt(), anyInt())) .thenReturn(new ResourceList()); - buildPfbQuartzJob(jobDao, wsmDao, restClientRetry, batchWriteService, activityLogger) + buildPfbQuartzJob( + jobDao, wsmDao, restClientRetry, batchWriteService, activityLogger, observationRegistry) .execute(mockContext); /* the forwardRelationsAvroResource should insert: @@ -321,7 +327,8 @@ void importCyclicalRelations() throws IOException, JobExecutionException { when(wsmDao.enumerateDataRepoSnapshotReferences(any(), anyInt(), anyInt())) .thenReturn(new ResourceList()); - buildPfbQuartzJob(jobDao, wsmDao, restClientRetry, batchWriteService, activityLogger) + buildPfbQuartzJob( + jobDao, wsmDao, restClientRetry, batchWriteService, activityLogger, observationRegistry) .execute(mockContext); RecordTypeSchema dataReleaseSchema = diff --git a/service/src/test/java/org/databiosphere/workspacedataservice/dataimport/pfb/PfbQuartzJobTest.java b/service/src/test/java/org/databiosphere/workspacedataservice/dataimport/pfb/PfbQuartzJobTest.java index 2f794826f..0c8a0e2f2 100644 --- a/service/src/test/java/org/databiosphere/workspacedataservice/dataimport/pfb/PfbQuartzJobTest.java +++ b/service/src/test/java/org/databiosphere/workspacedataservice/dataimport/pfb/PfbQuartzJobTest.java @@ -17,6 +17,7 @@ import bio.terra.workspace.model.ResourceAttributesUnion; import bio.terra.workspace.model.ResourceDescription; import bio.terra.workspace.model.ResourceList; +import io.micrometer.observation.ObservationRegistry; import java.io.IOException; import java.util.List; import java.util.Random; @@ -51,6 +52,7 @@ class PfbQuartzJobTest { @MockBean BatchWriteService batchWriteService; @MockBean ActivityLogger activityLogger; @Autowired RestClientRetry restClientRetry; + @Autowired ObservationRegistry observationRegistry; // test resources used below @Value("classpath:avro/minimal_data.avro") @@ -72,7 +74,13 @@ void linkAllNewSnapshots() { // call linkSnapshots PfbQuartzJob pfbQuartzJob = - buildPfbQuartzJob(jobDao, wsmDao, restClientRetry, batchWriteService, activityLogger); + buildPfbQuartzJob( + jobDao, + wsmDao, + restClientRetry, + batchWriteService, + activityLogger, + observationRegistry); pfbQuartzJob.linkSnapshots(input); // capture calls ArgumentCaptor argumentCaptor = ArgumentCaptor.forClass(SnapshotModel.class); @@ -101,7 +109,13 @@ void linkNothingWhenAllExist() { // call linkSnapshots PfbQuartzJob pfbQuartzJob = - buildPfbQuartzJob(jobDao, wsmDao, restClientRetry, batchWriteService, activityLogger); + buildPfbQuartzJob( + jobDao, + wsmDao, + restClientRetry, + batchWriteService, + activityLogger, + observationRegistry); pfbQuartzJob.linkSnapshots(input); // should not call WSM's create-snapshot-reference at all verify(wsmDao, times(0)).linkSnapshotForPolicy(any()); @@ -130,7 +144,13 @@ void linkSomeWhenSomeExist() { // call linkSnapshots PfbQuartzJob pfbQuartzJob = - buildPfbQuartzJob(jobDao, wsmDao, restClientRetry, batchWriteService, activityLogger); + buildPfbQuartzJob( + jobDao, + wsmDao, + restClientRetry, + batchWriteService, + activityLogger, + observationRegistry); pfbQuartzJob.linkSnapshots(input); // should call WSM's create-snapshot-reference only for the references that didn't already exist @@ -158,7 +178,8 @@ void doNotFailOnMissingSnapshotId() throws JobExecutionException, IOException { when(batchWriteService.batchWritePfbStream(any(), any(), any(), eq(BASE_ATTRIBUTES))) .thenReturn(BatchWriteResult.empty()); - buildPfbQuartzJob(jobDao, wsmDao, restClientRetry, batchWriteService, activityLogger) + buildPfbQuartzJob( + jobDao, wsmDao, restClientRetry, batchWriteService, activityLogger, observationRegistry) .execute(mockContext); // Should not call wsm dao @@ -179,7 +200,8 @@ void snapshotIdsAreParsed() throws JobExecutionException, IOException { when(batchWriteService.batchWritePfbStream(any(), any(), any(), eq(BASE_ATTRIBUTES))) .thenReturn(BatchWriteResult.empty()); - buildPfbQuartzJob(jobDao, wsmDao, restClientRetry, batchWriteService, activityLogger) + buildPfbQuartzJob( + jobDao, wsmDao, restClientRetry, batchWriteService, activityLogger, observationRegistry) .execute(mockContext); // The "790795c4..." UUID below is the snapshotId found in the "test.avro" resource used diff --git a/service/src/test/java/org/databiosphere/workspacedataservice/dataimport/pfb/PfbTestUtils.java b/service/src/test/java/org/databiosphere/workspacedataservice/dataimport/pfb/PfbTestUtils.java index b2a22da03..7cb7e6d8d 100644 --- a/service/src/test/java/org/databiosphere/workspacedataservice/dataimport/pfb/PfbTestUtils.java +++ b/service/src/test/java/org/databiosphere/workspacedataservice/dataimport/pfb/PfbTestUtils.java @@ -8,6 +8,7 @@ import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; +import io.micrometer.observation.ObservationRegistry; import java.io.IOException; import java.util.ArrayList; import java.util.List; @@ -179,8 +180,15 @@ public static PfbQuartzJob buildPfbQuartzJob( WorkspaceManagerDao wsmDao, RestClientRetry restClientRetry, BatchWriteService batchWriteService, - ActivityLogger activityLogger) { + ActivityLogger activityLogger, + ObservationRegistry observationRegistry) { return new PfbQuartzJob( - jobDao, wsmDao, restClientRetry, batchWriteService, activityLogger, UUID.randomUUID()); + jobDao, + wsmDao, + restClientRetry, + batchWriteService, + activityLogger, + UUID.randomUUID(), + observationRegistry); } } diff --git a/service/src/test/java/org/databiosphere/workspacedataservice/dataimport/tdr/TdrTestSupport.java b/service/src/test/java/org/databiosphere/workspacedataservice/dataimport/tdr/TdrTestSupport.java index 68c54b30c..6a8cb4930 100644 --- a/service/src/test/java/org/databiosphere/workspacedataservice/dataimport/tdr/TdrTestSupport.java +++ b/service/src/test/java/org/databiosphere/workspacedataservice/dataimport/tdr/TdrTestSupport.java @@ -1,6 +1,7 @@ package org.databiosphere.workspacedataservice.dataimport.tdr; import com.fasterxml.jackson.databind.ObjectMapper; +import io.micrometer.observation.ObservationRegistry; import java.net.URL; import java.util.UUID; import org.databiosphere.workspacedataservice.activitylog.ActivityLogger; @@ -19,6 +20,7 @@ class TdrTestSupport { @Autowired private BatchWriteService batchWriteService; @Autowired private ActivityLogger activityLogger; @Autowired private ObjectMapper objectMapper; + @Autowired private ObservationRegistry observationRegistry; /** Returns a TdrManifestQuartzJob that is capable of pulling parquet files from the classpath. */ TdrManifestQuartzJob buildTdrManifestQuartzJob(UUID workspaceId) { @@ -29,7 +31,8 @@ TdrManifestQuartzJob buildTdrManifestQuartzJob(UUID workspaceId) { batchWriteService, activityLogger, workspaceId, - objectMapper) { + objectMapper, + observationRegistry) { @Override protected URL parseUrl(String path) { if (path.startsWith("classpath:")) { From 426724e9ef0d68bc5b874b637d0fedd04dc45707 Mon Sep 17 00:00:00 2001 From: Adina Shanholtz Date: Thu, 18 Jan 2024 12:01:59 -0800 Subject: [PATCH 02/14] remove quartz from allowed metrics prefix --- .../workspacedataservice/metrics/MetricsConfig.java | 1 - 1 file changed, 1 deletion(-) diff --git a/service/src/main/java/org/databiosphere/workspacedataservice/metrics/MetricsConfig.java b/service/src/main/java/org/databiosphere/workspacedataservice/metrics/MetricsConfig.java index 9386ece6e..0ab2b108e 100644 --- a/service/src/main/java/org/databiosphere/workspacedataservice/metrics/MetricsConfig.java +++ b/service/src/main/java/org/databiosphere/workspacedataservice/metrics/MetricsConfig.java @@ -37,7 +37,6 @@ public MetricsConfig(BuildProperties buildProperties) { "jvm", "logback", "process", - "quartz", "spring", "system", "tomcat", From 92694866f4ad507bd78bc32efb443d3850170142 Mon Sep 17 00:00:00 2001 From: Adina Shanholtz Date: Fri, 19 Jan 2024 16:18:46 -0800 Subject: [PATCH 03/14] applied josh's prefactor and registered a meterregistry with the observationregistry --- .../dataimport/pfb/PfbQuartzJob.java | 4 ++-- .../workspacedataservice/jobexec/QuartzJob.java | 9 +++++++-- .../dataimport/pfb/PfbQuartzJobE2ETest.java | 1 - .../dataimport/pfb/PfbQuartzJobTest.java | 1 - .../dataimport/pfb/PfbTestSupport.java | 10 +++++++++- .../dataimport/pfb/PfbTestUtils.java | 1 - .../workspacedataservice/jobexec/QuartzJobTest.java | 4 ++++ 7 files changed, 22 insertions(+), 8 deletions(-) diff --git a/service/src/main/java/org/databiosphere/workspacedataservice/dataimport/pfb/PfbQuartzJob.java b/service/src/main/java/org/databiosphere/workspacedataservice/dataimport/pfb/PfbQuartzJob.java index 724e96bf9..49fc7908e 100644 --- a/service/src/main/java/org/databiosphere/workspacedataservice/dataimport/pfb/PfbQuartzJob.java +++ b/service/src/main/java/org/databiosphere/workspacedataservice/dataimport/pfb/PfbQuartzJob.java @@ -59,8 +59,8 @@ public PfbQuartzJob( RestClientRetry restClientRetry, BatchWriteService batchWriteService, ActivityLogger activityLogger, - @Value("${twds.instance.workspace-id}") UUID workspaceId, - ObservationRegistry observationRegistry) { + ObservationRegistry observationRegistry, + @Value("${twds.instance.workspace-id}") UUID workspaceId) { super(observationRegistry); this.jobDao = jobDao; this.wsmDao = wsmDao; diff --git a/service/src/main/java/org/databiosphere/workspacedataservice/jobexec/QuartzJob.java b/service/src/main/java/org/databiosphere/workspacedataservice/jobexec/QuartzJob.java index b9b54fce2..830a41f93 100644 --- a/service/src/main/java/org/databiosphere/workspacedataservice/jobexec/QuartzJob.java +++ b/service/src/main/java/org/databiosphere/workspacedataservice/jobexec/QuartzJob.java @@ -10,6 +10,8 @@ import static org.databiosphere.workspacedataservice.sam.BearerTokenFilter.ATTRIBUTE_NAME_TOKEN; import static org.databiosphere.workspacedataservice.shared.model.Schedulable.ARG_TOKEN; +import io.micrometer.core.instrument.observation.DefaultMeterObservationHandler; +import io.micrometer.core.instrument.simple.SimpleMeterRegistry; import io.micrometer.observation.Observation; import io.micrometer.observation.ObservationRegistry; import java.net.URL; @@ -51,12 +53,15 @@ public QuartzJob(ObservationRegistry observationRegistry) { public void execute(JobExecutionContext context) throws org.quartz.JobExecutionException { // retrieve jobId UUID jobId = UUID.fromString(context.getJobDetail().getKey().getName()); + // configure observation to collect counter, timer, and longtasktimer metrics + observationRegistry + .observationConfig() + .observationHandler(new DefaultMeterObservationHandler(new SimpleMeterRegistry())); Observation observation = Observation.start(OBSERVE_JOB_EXECUTE, observationRegistry) .contextualName(NAME_JOB_EXECUTION) - .lowCardinalityKeyValue(TAG_JOB_TYPE, getClass().getName()) + .lowCardinalityKeyValue(TAG_JOB_TYPE, getClass().getSimpleName()) .highCardinalityKeyValue(TAG_JOB_ID, jobId.toString()); - try (Observation.Scope scope = observation.openScope()) { // mark this job as running getJobDao().running(jobId); diff --git a/service/src/test/java/org/databiosphere/workspacedataservice/dataimport/pfb/PfbQuartzJobE2ETest.java b/service/src/test/java/org/databiosphere/workspacedataservice/dataimport/pfb/PfbQuartzJobE2ETest.java index 6fe1653f7..d58f584d1 100644 --- a/service/src/test/java/org/databiosphere/workspacedataservice/dataimport/pfb/PfbQuartzJobE2ETest.java +++ b/service/src/test/java/org/databiosphere/workspacedataservice/dataimport/pfb/PfbQuartzJobE2ETest.java @@ -11,7 +11,6 @@ import bio.terra.workspace.model.ResourceList; import com.google.common.collect.ImmutableMap; -import io.micrometer.observation.ObservationRegistry; import java.io.IOException; import java.math.BigDecimal; import java.util.List; diff --git a/service/src/test/java/org/databiosphere/workspacedataservice/dataimport/pfb/PfbQuartzJobTest.java b/service/src/test/java/org/databiosphere/workspacedataservice/dataimport/pfb/PfbQuartzJobTest.java index 157ed92cd..936cfd405 100644 --- a/service/src/test/java/org/databiosphere/workspacedataservice/dataimport/pfb/PfbQuartzJobTest.java +++ b/service/src/test/java/org/databiosphere/workspacedataservice/dataimport/pfb/PfbQuartzJobTest.java @@ -16,7 +16,6 @@ import bio.terra.workspace.model.ResourceAttributesUnion; import bio.terra.workspace.model.ResourceDescription; import bio.terra.workspace.model.ResourceList; -import io.micrometer.observation.ObservationRegistry; import java.io.IOException; import java.util.List; import java.util.Random; diff --git a/service/src/test/java/org/databiosphere/workspacedataservice/dataimport/pfb/PfbTestSupport.java b/service/src/test/java/org/databiosphere/workspacedataservice/dataimport/pfb/PfbTestSupport.java index b98ede6b3..7ff623e38 100644 --- a/service/src/test/java/org/databiosphere/workspacedataservice/dataimport/pfb/PfbTestSupport.java +++ b/service/src/test/java/org/databiosphere/workspacedataservice/dataimport/pfb/PfbTestSupport.java @@ -1,6 +1,7 @@ package org.databiosphere.workspacedataservice.dataimport.pfb; import com.fasterxml.jackson.databind.ObjectMapper; +import io.micrometer.observation.ObservationRegistry; import java.util.UUID; import org.databiosphere.workspacedataservice.activitylog.ActivityLogger; import org.databiosphere.workspacedataservice.dao.JobDao; @@ -18,10 +19,17 @@ class PfbTestSupport { @Autowired private BatchWriteService batchWriteService; @Autowired private ActivityLogger activityLogger; @Autowired private ObjectMapper objectMapper; + @Autowired private ObservationRegistry observationRegistry; PfbQuartzJob buildPfbQuartzJob(UUID workspaceId) { return new PfbQuartzJob( - jobDao, wsmDao, restClientRetry, batchWriteService, activityLogger, workspaceId); + jobDao, + wsmDao, + restClientRetry, + batchWriteService, + activityLogger, + observationRegistry, + workspaceId); } PfbQuartzJob buildPfbQuartzJob() { diff --git a/service/src/test/java/org/databiosphere/workspacedataservice/dataimport/pfb/PfbTestUtils.java b/service/src/test/java/org/databiosphere/workspacedataservice/dataimport/pfb/PfbTestUtils.java index fbe041ff6..24a3090ea 100644 --- a/service/src/test/java/org/databiosphere/workspacedataservice/dataimport/pfb/PfbTestUtils.java +++ b/service/src/test/java/org/databiosphere/workspacedataservice/dataimport/pfb/PfbTestUtils.java @@ -8,7 +8,6 @@ import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; -import io.micrometer.observation.ObservationRegistry; import java.io.IOException; import java.util.ArrayList; import java.util.List; diff --git a/service/src/test/java/org/databiosphere/workspacedataservice/jobexec/QuartzJobTest.java b/service/src/test/java/org/databiosphere/workspacedataservice/jobexec/QuartzJobTest.java index c089a752c..010437e7a 100644 --- a/service/src/test/java/org/databiosphere/workspacedataservice/jobexec/QuartzJobTest.java +++ b/service/src/test/java/org/databiosphere/workspacedataservice/jobexec/QuartzJobTest.java @@ -8,6 +8,7 @@ import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; +import io.micrometer.observation.ObservationRegistry; import java.time.OffsetDateTime; import java.time.ZoneId; import java.util.Map; @@ -24,6 +25,7 @@ import org.quartz.JobExecutionException; import org.quartz.JobKey; import org.quartz.impl.JobDetailImpl; +import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.test.context.SpringBootTest; import org.springframework.boot.test.mock.mockito.MockBean; @@ -32,6 +34,7 @@ class QuartzJobTest { @MockBean JobDao jobDao; + @Autowired ObservationRegistry observationRegistry; @BeforeAll void beforeAll() { @@ -62,6 +65,7 @@ class TestableQuartzJob extends QuartzJob { private final String expectedToken; public TestableQuartzJob(String expectedToken) { + super(observationRegistry); this.expectedToken = expectedToken; } From 77387a00251d90cc09daf548f47c13fc15a6b23f Mon Sep 17 00:00:00 2001 From: Adina Shanholtz Date: Fri, 19 Jan 2024 16:58:06 -0800 Subject: [PATCH 04/14] inline metric strings, remove @observe support --- service/build.gradle | 1 - .../jobexec/QuartzJob.java | 23 ++++------- .../metrics/MetricsConfig.java | 8 ---- .../metrics/MetricsDefinitions.java | 41 ------------------- .../service/RecordService.java | 12 ++---- 5 files changed, 12 insertions(+), 73 deletions(-) delete mode 100644 service/src/main/java/org/databiosphere/workspacedataservice/metrics/MetricsDefinitions.java diff --git a/service/build.gradle b/service/build.gradle index 70eabc616..1a4b087f1 100644 --- a/service/build.gradle +++ b/service/build.gradle @@ -54,7 +54,6 @@ dependencies { implementation 'org.springframework.boot:spring-boot-starter-batch' implementation 'org.springframework.boot:spring-boot-starter-quartz' implementation 'org.springframework.boot:spring-boot-starter-actuator' - implementation 'org.springframework.boot:spring-boot-starter-aop' implementation 'io.micrometer:micrometer-registry-prometheus' implementation 'org.springframework.integration:spring-integration-jdbc' implementation 'org.aspectj:aspectjweaver' // required by spring-retry, not used directly diff --git a/service/src/main/java/org/databiosphere/workspacedataservice/jobexec/QuartzJob.java b/service/src/main/java/org/databiosphere/workspacedataservice/jobexec/QuartzJob.java index 830a41f93..69a33dd93 100644 --- a/service/src/main/java/org/databiosphere/workspacedataservice/jobexec/QuartzJob.java +++ b/service/src/main/java/org/databiosphere/workspacedataservice/jobexec/QuartzJob.java @@ -1,12 +1,5 @@ package org.databiosphere.workspacedataservice.jobexec; -import static org.databiosphere.workspacedataservice.metrics.MetricsDefinitions.EVENT_JOB_FAILED; -import static org.databiosphere.workspacedataservice.metrics.MetricsDefinitions.EVENT_JOB_RUNNING; -import static org.databiosphere.workspacedataservice.metrics.MetricsDefinitions.EVENT_JOB_SUCCEEDED; -import static org.databiosphere.workspacedataservice.metrics.MetricsDefinitions.NAME_JOB_EXECUTION; -import static org.databiosphere.workspacedataservice.metrics.MetricsDefinitions.OBSERVE_JOB_EXECUTE; -import static org.databiosphere.workspacedataservice.metrics.MetricsDefinitions.TAG_JOB_ID; -import static org.databiosphere.workspacedataservice.metrics.MetricsDefinitions.TAG_JOB_TYPE; import static org.databiosphere.workspacedataservice.sam.BearerTokenFilter.ATTRIBUTE_NAME_TOKEN; import static org.databiosphere.workspacedataservice.shared.model.Schedulable.ARG_TOKEN; @@ -42,7 +35,7 @@ public abstract class QuartzJob implements Job { private final ObservationRegistry observationRegistry; - public QuartzJob(ObservationRegistry observationRegistry) { + protected QuartzJob(ObservationRegistry observationRegistry) { this.observationRegistry = observationRegistry; } @@ -58,14 +51,14 @@ public void execute(JobExecutionContext context) throws org.quartz.JobExecutionE .observationConfig() .observationHandler(new DefaultMeterObservationHandler(new SimpleMeterRegistry())); Observation observation = - Observation.start(OBSERVE_JOB_EXECUTE, observationRegistry) - .contextualName(NAME_JOB_EXECUTION) - .lowCardinalityKeyValue(TAG_JOB_TYPE, getClass().getSimpleName()) - .highCardinalityKeyValue(TAG_JOB_ID, jobId.toString()); + Observation.start("wds.job.execute", observationRegistry) + .contextualName("job-execution") + .lowCardinalityKeyValue("jobType", getClass().getSimpleName()) + .highCardinalityKeyValue("jobId", jobId.toString()); try (Observation.Scope scope = observation.openScope()) { // mark this job as running getJobDao().running(jobId); - observation.event(Observation.Event.of(EVENT_JOB_RUNNING)); + observation.event(Observation.Event.of("job.running")); // look for an auth token in the Quartz JobDataMap String authToken = getJobDataString(context.getMergedJobDataMap(), ARG_TOKEN); // and stash the auth token into job context @@ -77,12 +70,12 @@ public void execute(JobExecutionContext context) throws org.quartz.JobExecutionE executeInternal(jobId, context); // if we reached here, mark this job as successful getJobDao().succeeded(jobId); - observation.event(Observation.Event.of(EVENT_JOB_SUCCEEDED)); + observation.event(Observation.Event.of("job.succeeded")); } catch (Exception e) { // on any otherwise-unhandled exception, mark the job as failed getJobDao().fail(jobId, e); observation.error(e); - observation.event(Observation.Event.of(EVENT_JOB_FAILED)); + observation.event(Observation.Event.of("job.failed")); } finally { JobContextHolder.destroy(); observation.stop(); diff --git a/service/src/main/java/org/databiosphere/workspacedataservice/metrics/MetricsConfig.java b/service/src/main/java/org/databiosphere/workspacedataservice/metrics/MetricsConfig.java index 0ab2b108e..28ab8e723 100644 --- a/service/src/main/java/org/databiosphere/workspacedataservice/metrics/MetricsConfig.java +++ b/service/src/main/java/org/databiosphere/workspacedataservice/metrics/MetricsConfig.java @@ -7,8 +7,6 @@ import com.google.common.collect.ImmutableList; import io.micrometer.core.instrument.MeterRegistry; import io.micrometer.core.instrument.Tag; -import io.micrometer.observation.ObservationRegistry; -import io.micrometer.observation.aop.ObservedAspect; import java.util.Set; import org.springframework.boot.actuate.autoconfigure.metrics.MeterRegistryCustomizer; import org.springframework.boot.info.BuildProperties; @@ -61,10 +59,4 @@ MeterRegistryCustomizer addWdsVersionTag() { .add(Tag.of("wds.version", buildProperties.getVersion())) .build()); } - - // Observation support for metrics - @Bean - ObservedAspect observedAspect(ObservationRegistry observationRegistry) { - return new ObservedAspect(observationRegistry); - } } diff --git a/service/src/main/java/org/databiosphere/workspacedataservice/metrics/MetricsDefinitions.java b/service/src/main/java/org/databiosphere/workspacedataservice/metrics/MetricsDefinitions.java deleted file mode 100644 index 80b991452..000000000 --- a/service/src/main/java/org/databiosphere/workspacedataservice/metrics/MetricsDefinitions.java +++ /dev/null @@ -1,41 +0,0 @@ -package org.databiosphere.workspacedataservice.metrics; - -public final class MetricsDefinitions { - - private MetricsDefinitions() {} - - // Record Metrics - /** counter for column schema changes, i.e. "alter column" sql statements */ - public static final String COUNTER_COL_CHANGE = "column.change.datatype"; - - /** tag for a {@link org.databiosphere.workspacedataservice.shared.model.RecordType} */ - public static final String TAG_RECORD_TYPE = "RecordType"; - - /** tag for an instance id */ - public static final String TAG_INSTANCE = "Instance"; - - /** tag for a record attribute name */ - public static final String TAG_ATTRIBUTE_NAME = "AttributeName"; - - // Quartz Job Metrics - /** observable name for running job */ - public static final String OBSERVE_JOB_EXECUTE = "wds.job.execute"; - - /** event name for job succeeded */ - public static final String EVENT_JOB_SUCCEEDED = "job.succeeded"; - - /** event name for job running */ - public static final String EVENT_JOB_RUNNING = "job.running"; - - /** event name for job failed */ - public static final String EVENT_JOB_FAILED = "job.failed"; - - /** name for job execution (used in span) */ - public static final String NAME_JOB_EXECUTION = "job-execution"; - - /** tag for the type of job */ - public static final String TAG_JOB_TYPE = "jobType"; - - /** tag for the type of job */ - public static final String TAG_JOB_ID = "jobId"; -} diff --git a/service/src/main/java/org/databiosphere/workspacedataservice/service/RecordService.java b/service/src/main/java/org/databiosphere/workspacedataservice/service/RecordService.java index 9843bb8b6..7cf6583b0 100644 --- a/service/src/main/java/org/databiosphere/workspacedataservice/service/RecordService.java +++ b/service/src/main/java/org/databiosphere/workspacedataservice/service/RecordService.java @@ -1,9 +1,5 @@ package org.databiosphere.workspacedataservice.service; -import static org.databiosphere.workspacedataservice.metrics.MetricsDefinitions.COUNTER_COL_CHANGE; -import static org.databiosphere.workspacedataservice.metrics.MetricsDefinitions.TAG_ATTRIBUTE_NAME; -import static org.databiosphere.workspacedataservice.metrics.MetricsDefinitions.TAG_INSTANCE; -import static org.databiosphere.workspacedataservice.metrics.MetricsDefinitions.TAG_RECORD_TYPE; import static org.databiosphere.workspacedataservice.service.model.ReservedNames.RESERVED_NAME_PREFIX; import bio.terra.common.db.WriteTransaction; @@ -220,10 +216,10 @@ public Map addOrUpdateColumnIfNeeded( // update a metrics counter with this schema change Counter counter = - Counter.builder(COUNTER_COL_CHANGE) - .tag(TAG_RECORD_TYPE, recordType.getName()) - .tag(TAG_ATTRIBUTE_NAME, column) - .tag(TAG_INSTANCE, instanceId.toString()) + Counter.builder("column.change.datatype") + .tag("RecordType", recordType.getName()) + .tag("AttributeName", column) + .tag("Instance", instanceId.toString()) .tag("OldDataType", valueDifference.leftValue().toString()) .tag("NewDataType", updatedColType.toString()) .description("Column schema changes") From 4eac46c9977de7883c8c09c4b9f7edb6231b4fd3 Mon Sep 17 00:00:00 2001 From: Josh Ladieu Date: Mon, 22 Jan 2024 10:07:22 -0500 Subject: [PATCH 05/14] Add `ObservationConfig` This intends to make the default `ObservationRegistry` automatically wired up to the same `MeterRegistry` provided by springboot. --- .../jobexec/QuartzJob.java | 7 +----- .../metrics/ObservationConfig.java | 25 +++++++++++++++++++ 2 files changed, 26 insertions(+), 6 deletions(-) create mode 100644 service/src/main/java/org/databiosphere/workspacedataservice/metrics/ObservationConfig.java diff --git a/service/src/main/java/org/databiosphere/workspacedataservice/jobexec/QuartzJob.java b/service/src/main/java/org/databiosphere/workspacedataservice/jobexec/QuartzJob.java index 69a33dd93..3a7ad642e 100644 --- a/service/src/main/java/org/databiosphere/workspacedataservice/jobexec/QuartzJob.java +++ b/service/src/main/java/org/databiosphere/workspacedataservice/jobexec/QuartzJob.java @@ -3,8 +3,6 @@ import static org.databiosphere.workspacedataservice.sam.BearerTokenFilter.ATTRIBUTE_NAME_TOKEN; import static org.databiosphere.workspacedataservice.shared.model.Schedulable.ARG_TOKEN; -import io.micrometer.core.instrument.observation.DefaultMeterObservationHandler; -import io.micrometer.core.instrument.simple.SimpleMeterRegistry; import io.micrometer.observation.Observation; import io.micrometer.observation.ObservationRegistry; import java.net.URL; @@ -46,10 +44,7 @@ protected QuartzJob(ObservationRegistry observationRegistry) { public void execute(JobExecutionContext context) throws org.quartz.JobExecutionException { // retrieve jobId UUID jobId = UUID.fromString(context.getJobDetail().getKey().getName()); - // configure observation to collect counter, timer, and longtasktimer metrics - observationRegistry - .observationConfig() - .observationHandler(new DefaultMeterObservationHandler(new SimpleMeterRegistry())); + Observation observation = Observation.start("wds.job.execute", observationRegistry) .contextualName("job-execution") diff --git a/service/src/main/java/org/databiosphere/workspacedataservice/metrics/ObservationConfig.java b/service/src/main/java/org/databiosphere/workspacedataservice/metrics/ObservationConfig.java new file mode 100644 index 000000000..7662b7000 --- /dev/null +++ b/service/src/main/java/org/databiosphere/workspacedataservice/metrics/ObservationConfig.java @@ -0,0 +1,25 @@ +package org.databiosphere.workspacedataservice.metrics; + +import io.micrometer.core.instrument.MeterRegistry; +import io.micrometer.core.instrument.observation.DefaultMeterObservationHandler; +import io.micrometer.observation.ObservationRegistry; +import org.springframework.boot.actuate.autoconfigure.observation.ObservationRegistryCustomizer; +import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Configuration; + +@Configuration +public class ObservationConfig { + private final MeterRegistry meterRegistry; + + public ObservationConfig(MeterRegistry meterRegistry) { + this.meterRegistry = meterRegistry; + } + + @Bean + ObservationRegistryCustomizer useAutowiredMeterRegistry() { + return observationRegistry -> + observationRegistry + .observationConfig() + .observationHandler(new DefaultMeterObservationHandler(meterRegistry)); + } +} From a0ab15e4504b6ccebb8f807a83bd4a42fad15af6 Mon Sep 17 00:00:00 2001 From: Adina Shanholtz Date: Wed, 24 Jan 2024 06:53:50 -0800 Subject: [PATCH 06/14] test --- .../jobexec/QuartzJob.java | 6 +- .../jobexec/QuartzJobTest.java | 82 ++++++++++++++++--- .../service/RecordServiceTest.java | 9 +- 3 files changed, 78 insertions(+), 19 deletions(-) diff --git a/service/src/main/java/org/databiosphere/workspacedataservice/jobexec/QuartzJob.java b/service/src/main/java/org/databiosphere/workspacedataservice/jobexec/QuartzJob.java index 3a7ad642e..cb32eabe1 100644 --- a/service/src/main/java/org/databiosphere/workspacedataservice/jobexec/QuartzJob.java +++ b/service/src/main/java/org/databiosphere/workspacedataservice/jobexec/QuartzJob.java @@ -50,7 +50,7 @@ public void execute(JobExecutionContext context) throws org.quartz.JobExecutionE .contextualName("job-execution") .lowCardinalityKeyValue("jobType", getClass().getSimpleName()) .highCardinalityKeyValue("jobId", jobId.toString()); - try (Observation.Scope scope = observation.openScope()) { + try { // mark this job as running getJobDao().running(jobId); observation.event(Observation.Event.of("job.running")); @@ -65,12 +65,12 @@ public void execute(JobExecutionContext context) throws org.quartz.JobExecutionE executeInternal(jobId, context); // if we reached here, mark this job as successful getJobDao().succeeded(jobId); - observation.event(Observation.Event.of("job.succeeded")); + observation.lowCardinalityKeyValue("outcome", "success"); } catch (Exception e) { // on any otherwise-unhandled exception, mark the job as failed getJobDao().fail(jobId, e); observation.error(e); - observation.event(Observation.Event.of("job.failed")); + observation.lowCardinalityKeyValue("outcome", "failure"); } finally { JobContextHolder.destroy(); observation.stop(); diff --git a/service/src/test/java/org/databiosphere/workspacedataservice/jobexec/QuartzJobTest.java b/service/src/test/java/org/databiosphere/workspacedataservice/jobexec/QuartzJobTest.java index 010437e7a..eff18a253 100644 --- a/service/src/test/java/org/databiosphere/workspacedataservice/jobexec/QuartzJobTest.java +++ b/service/src/test/java/org/databiosphere/workspacedataservice/jobexec/QuartzJobTest.java @@ -2,21 +2,30 @@ import static org.databiosphere.workspacedataservice.shared.model.Schedulable.ARG_TOKEN; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotEquals; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; +import io.micrometer.core.instrument.LongTaskTimer; +import io.micrometer.core.instrument.MeterRegistry; +import io.micrometer.core.instrument.Metrics; +import io.micrometer.core.instrument.Timer; import io.micrometer.observation.ObservationRegistry; +import io.micrometer.observation.tck.TestObservationRegistry; +import io.micrometer.observation.tck.TestObservationRegistryAssert; import java.time.OffsetDateTime; import java.time.ZoneId; import java.util.Map; import java.util.UUID; +import java.util.concurrent.TimeUnit; import org.apache.commons.lang3.RandomStringUtils; import org.databiosphere.workspacedataservice.dao.JobDao; import org.databiosphere.workspacedataservice.generated.GenericJobServerModel; import org.databiosphere.workspacedataservice.sam.TokenContextUtil; +import org.junit.jupiter.api.AfterAll; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.TestInstance; @@ -35,6 +44,8 @@ class QuartzJobTest { @MockBean JobDao jobDao; @Autowired ObservationRegistry observationRegistry; + @Autowired MeterRegistry meterRegistry; + private TestObservationRegistry testObservationRegistry; @BeforeAll void beforeAll() { @@ -53,6 +64,16 @@ void beforeAll() { .thenThrow(new RuntimeException("test failed via jobDao.fail()")); when(jobDao.fail(any(), anyString())) .thenThrow(new RuntimeException("test failed via jobDao.fail()")); + // set up metrics registries + testObservationRegistry = TestObservationRegistry.create(); + Metrics.globalRegistry.add(meterRegistry); + } + + @AfterAll + void afterAll() { + testObservationRegistry.clear(); + meterRegistry.clear(); + Metrics.globalRegistry.clear(); } /** @@ -64,8 +85,8 @@ class TestableQuartzJob extends QuartzJob { private final String expectedToken; - public TestableQuartzJob(String expectedToken) { - super(observationRegistry); + public TestableQuartzJob(String expectedToken, ObservationRegistry registry) { + super(registry); this.expectedToken = expectedToken; } @@ -84,21 +105,62 @@ protected void executeInternal(UUID jobId, JobExecutionContext context) { void tokenIsStashedAndCleaned() throws JobExecutionException { // set an example token, via mock, into the Quartz JobDataMap String expectedToken = RandomStringUtils.randomAlphanumeric(10); - JobExecutionContext mockContext = mock(JobExecutionContext.class); - when(mockContext.getMergedJobDataMap()) - .thenReturn(new JobDataMap(Map.of(ARG_TOKEN, expectedToken))); - JobDetailImpl jobDetail = new JobDetailImpl(); - jobDetail.setKey(new JobKey(UUID.randomUUID().toString(), "bar")); - when(mockContext.getJobDetail()).thenReturn(jobDetail); - + JobExecutionContext mockContext = setUpTestJob(expectedToken, UUID.randomUUID().toString()); // assert that no token exists via TokenContextUtil assertTrue( TokenContextUtil.getToken().isEmpty(), "no token should exist before running the job"); // execute the TestableQuartzJob, which asserts that the token passed properly from the // JobDataMap into job context and is therefore retrievable via TokenContextUtil - new TestableQuartzJob(expectedToken).execute(mockContext); + new TestableQuartzJob(expectedToken, observationRegistry).execute(mockContext); // assert that the token is cleaned up and no longer reachable via TokenContextUtil assertTrue( TokenContextUtil.getToken().isEmpty(), "no token should exist after running the job"); } + + @Test + void correctObservation() throws JobExecutionException { + String randomToken = RandomStringUtils.randomAlphanumeric(10); + String jobUuid = UUID.randomUUID().toString(); + JobExecutionContext mockContext = setUpTestJob(randomToken, jobUuid); + + // execute the TestableQuartzJob, then use testObservationRegistry to confirm observation + new TestableQuartzJob(randomToken, testObservationRegistry).execute(mockContext); + + TestObservationRegistryAssert.assertThat(testObservationRegistry) + .doesNotHaveAnyRemainingCurrentObservation() + .hasObservationWithNameEqualTo("wds.job.execute") + .that() + .hasHighCardinalityKeyValue("jobId", jobUuid) + .hasLowCardinalityKeyValue("jobType", "TestableQuartzJob") + .hasLowCardinalityKeyValue("outcome", "success") + .hasBeenStarted() + .hasBeenStopped(); + } + + @Test + void correctMetrics() throws JobExecutionException { + String randomToken = RandomStringUtils.randomAlphanumeric(10); + JobExecutionContext mockContext = setUpTestJob(randomToken, UUID.randomUUID().toString()); + + // execute the TestableQuartzJob, then confirm metrics provisioned correctly + new TestableQuartzJob(randomToken, observationRegistry).execute(mockContext); + + // metrics provisioned should be longTaskTimer and a Timer, confirm both ran + LongTaskTimer longTaskTimer = meterRegistry.find("wds.job.execute.active").longTaskTimer(); + assertNotEquals(longTaskTimer.duration(TimeUnit.SECONDS), 0); + Timer timer = meterRegistry.find("wds.job.execute").timer(); + assertNotEquals(timer.count(), 0); + } + + // sets up a job and returns the job context + private JobExecutionContext setUpTestJob(String randomToken, String jobUuid) { + // String jobUuid = UUID.randomUUID().toString(); + JobExecutionContext mockContext = mock(JobExecutionContext.class); + when(mockContext.getMergedJobDataMap()) + .thenReturn(new JobDataMap(Map.of(ARG_TOKEN, randomToken))); + JobDetailImpl jobDetail = new JobDetailImpl(); + jobDetail.setKey(new JobKey(jobUuid, "bar")); + when(mockContext.getJobDetail()).thenReturn(jobDetail); + return mockContext; + } } diff --git a/service/src/test/java/org/databiosphere/workspacedataservice/service/RecordServiceTest.java b/service/src/test/java/org/databiosphere/workspacedataservice/service/RecordServiceTest.java index 134eb86c5..78a06f670 100644 --- a/service/src/test/java/org/databiosphere/workspacedataservice/service/RecordServiceTest.java +++ b/service/src/test/java/org/databiosphere/workspacedataservice/service/RecordServiceTest.java @@ -1,8 +1,5 @@ package org.databiosphere.workspacedataservice.service; -import static org.databiosphere.workspacedataservice.metrics.MetricsDefinitions.TAG_ATTRIBUTE_NAME; -import static org.databiosphere.workspacedataservice.metrics.MetricsDefinitions.TAG_INSTANCE; -import static org.databiosphere.workspacedataservice.metrics.MetricsDefinitions.TAG_RECORD_TYPE; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertInstanceOf; @@ -93,9 +90,9 @@ void schemaChangesIncrementMetricsCounter() { // assert the counter has been incremented once assertEquals(1, counter.count()); // validate the tags for that counter - assertEquals(recordType.getName(), counter.getId().getTag(TAG_RECORD_TYPE)); - assertEquals("myAttr", counter.getId().getTag(TAG_ATTRIBUTE_NAME)); - assertEquals(instanceId.toString(), counter.getId().getTag(TAG_INSTANCE)); + assertEquals(recordType.getName(), counter.getId().getTag("RecordType")); + assertEquals("myAttr", counter.getId().getTag("AttributeName")); + assertEquals(instanceId.toString(), counter.getId().getTag("Instance")); assertEquals(DataTypeMapping.NUMBER.toString(), counter.getId().getTag("OldDataType")); assertEquals(DataTypeMapping.STRING.toString(), counter.getId().getTag("NewDataType")); } From 4869a463e5007544512b540bdda4f7c1e019f830 Mon Sep 17 00:00:00 2001 From: Adina Shanholtz Date: Wed, 24 Jan 2024 07:01:20 -0800 Subject: [PATCH 07/14] whoops forgot to add library --- service/build.gradle | 1 + 1 file changed, 1 insertion(+) diff --git a/service/build.gradle b/service/build.gradle index 1a4b087f1..7a9c59108 100644 --- a/service/build.gradle +++ b/service/build.gradle @@ -113,6 +113,7 @@ dependencies { annotationProcessor 'org.springframework.boot:spring-boot-configuration-processor' runtimeOnly 'org.webjars.npm:swagger-ui-dist:5.9.0' + testImplementation 'io.micrometer:micrometer-observation-test' testImplementation 'org.springframework.boot:spring-boot-starter-test' testImplementation 'org.junit.jupiter:junit-jupiter' testImplementation project(':client') From 8a5c0d7d900a26447b0d9b79449e467f6a637b41 Mon Sep 17 00:00:00 2001 From: Adina Shanholtz Date: Wed, 24 Jan 2024 07:40:01 -0800 Subject: [PATCH 08/14] sonar is right --- .../workspacedataservice/jobexec/QuartzJobTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/service/src/test/java/org/databiosphere/workspacedataservice/jobexec/QuartzJobTest.java b/service/src/test/java/org/databiosphere/workspacedataservice/jobexec/QuartzJobTest.java index eff18a253..cd0fea5db 100644 --- a/service/src/test/java/org/databiosphere/workspacedataservice/jobexec/QuartzJobTest.java +++ b/service/src/test/java/org/databiosphere/workspacedataservice/jobexec/QuartzJobTest.java @@ -147,9 +147,9 @@ void correctMetrics() throws JobExecutionException { // metrics provisioned should be longTaskTimer and a Timer, confirm both ran LongTaskTimer longTaskTimer = meterRegistry.find("wds.job.execute.active").longTaskTimer(); - assertNotEquals(longTaskTimer.duration(TimeUnit.SECONDS), 0); + assertNotEquals(0, longTaskTimer.duration(TimeUnit.SECONDS)); Timer timer = meterRegistry.find("wds.job.execute").timer(); - assertNotEquals(timer.count(), 0); + assertNotEquals(0, timer.count()); } // sets up a job and returns the job context From 5066e045a43e4a42d9569bcde6eebf85c914b753 Mon Sep 17 00:00:00 2001 From: David An Date: Mon, 29 Jan 2024 15:54:36 -0500 Subject: [PATCH 09/14] suggested tweaks to observability setup (#475) observability tweaks to remove observationRegistry bean and add testobservationRegistry --- .../metrics/ObservationConfig.java | 25 -------- .../jobexec/QuartzJobTest.java | 60 +++++++++++++++---- .../TestObservationRegistryConfig.java | 22 +++++++ 3 files changed, 70 insertions(+), 37 deletions(-) delete mode 100644 service/src/main/java/org/databiosphere/workspacedataservice/metrics/ObservationConfig.java create mode 100644 service/src/test/java/org/databiosphere/workspacedataservice/observability/TestObservationRegistryConfig.java diff --git a/service/src/main/java/org/databiosphere/workspacedataservice/metrics/ObservationConfig.java b/service/src/main/java/org/databiosphere/workspacedataservice/metrics/ObservationConfig.java deleted file mode 100644 index 7662b7000..000000000 --- a/service/src/main/java/org/databiosphere/workspacedataservice/metrics/ObservationConfig.java +++ /dev/null @@ -1,25 +0,0 @@ -package org.databiosphere.workspacedataservice.metrics; - -import io.micrometer.core.instrument.MeterRegistry; -import io.micrometer.core.instrument.observation.DefaultMeterObservationHandler; -import io.micrometer.observation.ObservationRegistry; -import org.springframework.boot.actuate.autoconfigure.observation.ObservationRegistryCustomizer; -import org.springframework.context.annotation.Bean; -import org.springframework.context.annotation.Configuration; - -@Configuration -public class ObservationConfig { - private final MeterRegistry meterRegistry; - - public ObservationConfig(MeterRegistry meterRegistry) { - this.meterRegistry = meterRegistry; - } - - @Bean - ObservationRegistryCustomizer useAutowiredMeterRegistry() { - return observationRegistry -> - observationRegistry - .observationConfig() - .observationHandler(new DefaultMeterObservationHandler(meterRegistry)); - } -} diff --git a/service/src/test/java/org/databiosphere/workspacedataservice/jobexec/QuartzJobTest.java b/service/src/test/java/org/databiosphere/workspacedataservice/jobexec/QuartzJobTest.java index cd0fea5db..9a2df9e15 100644 --- a/service/src/test/java/org/databiosphere/workspacedataservice/jobexec/QuartzJobTest.java +++ b/service/src/test/java/org/databiosphere/workspacedataservice/jobexec/QuartzJobTest.java @@ -1,8 +1,11 @@ package org.databiosphere.workspacedataservice.jobexec; +import static org.assertj.core.api.Assertions.assertThat; import static org.databiosphere.workspacedataservice.shared.model.Schedulable.ARG_TOKEN; import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertNotEquals; +import static org.junit.jupiter.api.Assertions.assertInstanceOf; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyString; @@ -24,9 +27,10 @@ import org.apache.commons.lang3.RandomStringUtils; import org.databiosphere.workspacedataservice.dao.JobDao; import org.databiosphere.workspacedataservice.generated.GenericJobServerModel; +import org.databiosphere.workspacedataservice.observability.TestObservationRegistryConfig; import org.databiosphere.workspacedataservice.sam.TokenContextUtil; -import org.junit.jupiter.api.AfterAll; import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.TestInstance; import org.quartz.JobDataMap; @@ -37,15 +41,17 @@ import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.test.context.SpringBootTest; import org.springframework.boot.test.mock.mockito.MockBean; +import org.springframework.context.annotation.Import; @SpringBootTest @TestInstance(TestInstance.Lifecycle.PER_CLASS) +@Import(TestObservationRegistryConfig.class) class QuartzJobTest { @MockBean JobDao jobDao; - @Autowired ObservationRegistry observationRegistry; @Autowired MeterRegistry meterRegistry; - private TestObservationRegistry testObservationRegistry; + // overridden with a TestObservationRegistry + @Autowired private ObservationRegistry observationRegistry; @BeforeAll void beforeAll() { @@ -64,16 +70,14 @@ void beforeAll() { .thenThrow(new RuntimeException("test failed via jobDao.fail()")); when(jobDao.fail(any(), anyString())) .thenThrow(new RuntimeException("test failed via jobDao.fail()")); - // set up metrics registries - testObservationRegistry = TestObservationRegistry.create(); - Metrics.globalRegistry.add(meterRegistry); } - @AfterAll - void afterAll() { - testObservationRegistry.clear(); + /** clear all observations and metrics prior to each test */ + @BeforeEach + void beforeEach() { meterRegistry.clear(); Metrics.globalRegistry.clear(); + ((TestObservationRegistry) observationRegistry).clear(); } /** @@ -123,6 +127,10 @@ void correctObservation() throws JobExecutionException { String jobUuid = UUID.randomUUID().toString(); JobExecutionContext mockContext = setUpTestJob(randomToken, jobUuid); + // this test requires a TestObservationRegistry + TestObservationRegistry testObservationRegistry = + assertInstanceOf(TestObservationRegistry.class, observationRegistry); + // execute the TestableQuartzJob, then use testObservationRegistry to confirm observation new TestableQuartzJob(randomToken, testObservationRegistry).execute(mockContext); @@ -142,14 +150,42 @@ void correctMetrics() throws JobExecutionException { String randomToken = RandomStringUtils.randomAlphanumeric(10); JobExecutionContext mockContext = setUpTestJob(randomToken, UUID.randomUUID().toString()); + // LongTaskTimer and Timer should both be null before we run the job + assertNull(meterRegistry.find("wds.job.execute.active").longTaskTimer()); + assertNull(meterRegistry.find("wds.job.execute").timer()); + // execute the TestableQuartzJob, then confirm metrics provisioned correctly new TestableQuartzJob(randomToken, observationRegistry).execute(mockContext); // metrics provisioned should be longTaskTimer and a Timer, confirm both ran LongTaskTimer longTaskTimer = meterRegistry.find("wds.job.execute.active").longTaskTimer(); - assertNotEquals(0, longTaskTimer.duration(TimeUnit.SECONDS)); + assertNotNull(longTaskTimer); Timer timer = meterRegistry.find("wds.job.execute").timer(); - assertNotEquals(0, timer.count()); + assertNotNull(timer); + + // the LongTaskTimer for wds.job.execute.active tracks "in-flight long-running tasks". + // since the job we just ran is complete, we expect it to show zero active tasks + // and zero duration. + assertEquals(0, longTaskTimer.activeTasks()); + assertEquals(0, longTaskTimer.duration(TimeUnit.SECONDS)); + + // the Timer for wds.job.execute tracks history of completed jobs. We expect its duration + // to be nonzero and its count to be 1. + assertThat(timer.totalTime(TimeUnit.SECONDS)).isPositive(); + // with one observation, max and total should be the same + assertEquals(timer.totalTime(TimeUnit.SECONDS), timer.max(TimeUnit.SECONDS)); + assertEquals(1, timer.count()); + + // execute the TestableQuartzJob a few more times to further increment the timer count + // and the timer total + double previousTotal = timer.totalTime(TimeUnit.SECONDS); + for (int i = 2; i < 10; i++) { + new TestableQuartzJob(randomToken, observationRegistry).execute(mockContext); + assertEquals(i, timer.count()); + // current total should be greater than previous total + assertThat(timer.totalTime(TimeUnit.SECONDS)).isGreaterThan(previousTotal); + previousTotal = timer.totalTime(TimeUnit.SECONDS); + } } // sets up a job and returns the job context diff --git a/service/src/test/java/org/databiosphere/workspacedataservice/observability/TestObservationRegistryConfig.java b/service/src/test/java/org/databiosphere/workspacedataservice/observability/TestObservationRegistryConfig.java new file mode 100644 index 000000000..6fdfd9923 --- /dev/null +++ b/service/src/test/java/org/databiosphere/workspacedataservice/observability/TestObservationRegistryConfig.java @@ -0,0 +1,22 @@ +package org.databiosphere.workspacedataservice.observability; + +import io.micrometer.observation.ObservationRegistry; +import io.micrometer.observation.tck.TestObservationRegistry; +import org.springframework.boot.test.context.TestConfiguration; +import org.springframework.context.annotation.Bean; + +/** + * Provides a TestObservationRegistry bean which can be used in place of Spring's default + * ObservationRegistry. {@link TestObservationRegistry} provides conveniences for unit test + * assertions. + * + *

To use this in your unit test, add an {@code @Import(TestObservationRegistryConfig.class)} + * annotation to your class. + */ +@TestConfiguration +public class TestObservationRegistryConfig { + @Bean + ObservationRegistry testObservationRegistry() { + return TestObservationRegistry.create(); + } +} From 2d5f6f06de765245f0a1f28c616ad82d867af2ea Mon Sep 17 00:00:00 2001 From: Adina Shanholtz Date: Tue, 30 Jan 2024 21:01:11 -0800 Subject: [PATCH 10/14] partial failure test, use statusenum, no magic strings --- .../jobexec/QuartzJob.java | 5 +- .../service/RecordService.java | 13 ++++- .../jobexec/QuartzJobTest.java | 58 ++++++++++++++++--- .../service/RecordServiceTest.java | 15 +++-- 4 files changed, 74 insertions(+), 17 deletions(-) diff --git a/service/src/main/java/org/databiosphere/workspacedataservice/jobexec/QuartzJob.java b/service/src/main/java/org/databiosphere/workspacedataservice/jobexec/QuartzJob.java index cb32eabe1..a1f0f3ffb 100644 --- a/service/src/main/java/org/databiosphere/workspacedataservice/jobexec/QuartzJob.java +++ b/service/src/main/java/org/databiosphere/workspacedataservice/jobexec/QuartzJob.java @@ -1,5 +1,6 @@ package org.databiosphere.workspacedataservice.jobexec; +import static org.databiosphere.workspacedataservice.generated.GenericJobServerModel.StatusEnum; import static org.databiosphere.workspacedataservice.sam.BearerTokenFilter.ATTRIBUTE_NAME_TOKEN; import static org.databiosphere.workspacedataservice.shared.model.Schedulable.ARG_TOKEN; @@ -65,12 +66,12 @@ public void execute(JobExecutionContext context) throws org.quartz.JobExecutionE executeInternal(jobId, context); // if we reached here, mark this job as successful getJobDao().succeeded(jobId); - observation.lowCardinalityKeyValue("outcome", "success"); + observation.lowCardinalityKeyValue("outcome", StatusEnum.SUCCEEDED.getValue()); } catch (Exception e) { // on any otherwise-unhandled exception, mark the job as failed getJobDao().fail(jobId, e); observation.error(e); - observation.lowCardinalityKeyValue("outcome", "failure"); + observation.lowCardinalityKeyValue("outcome", StatusEnum.ERROR.getValue()); } finally { JobContextHolder.destroy(); observation.stop(); diff --git a/service/src/main/java/org/databiosphere/workspacedataservice/service/RecordService.java b/service/src/main/java/org/databiosphere/workspacedataservice/service/RecordService.java index 7cf6583b0..e05680fd6 100644 --- a/service/src/main/java/org/databiosphere/workspacedataservice/service/RecordService.java +++ b/service/src/main/java/org/databiosphere/workspacedataservice/service/RecordService.java @@ -42,6 +42,13 @@ @Service public class RecordService { + // strings used for metrics + public static final String COUNTER_COL_CHANGE = "column.change.datatype"; + public static final String TAG_RECORD_TYPE = "RecordType"; + public static final String TAG_INSTANCE = "Instance"; + public static final String TAG_ATTRIBUTE_NAME = "AttributeName"; + public static final String TAG_OLD_DATATYPE = "OldDataType"; + public static final String TAG_NEW_DATATYPE = "NewDataType"; private final RecordDao recordDao; @@ -216,10 +223,10 @@ public Map addOrUpdateColumnIfNeeded( // update a metrics counter with this schema change Counter counter = - Counter.builder("column.change.datatype") - .tag("RecordType", recordType.getName()) + Counter.builder(COUNTER_COL_CHANGE) + .tag(TAG_RECORD_TYPE, recordType.getName()) .tag("AttributeName", column) - .tag("Instance", instanceId.toString()) + .tag(TAG_INSTANCE, instanceId.toString()) .tag("OldDataType", valueDifference.leftValue().toString()) .tag("NewDataType", updatedColType.toString()) .description("Column schema changes") diff --git a/service/src/test/java/org/databiosphere/workspacedataservice/jobexec/QuartzJobTest.java b/service/src/test/java/org/databiosphere/workspacedataservice/jobexec/QuartzJobTest.java index 9a2df9e15..e99328434 100644 --- a/service/src/test/java/org/databiosphere/workspacedataservice/jobexec/QuartzJobTest.java +++ b/service/src/test/java/org/databiosphere/workspacedataservice/jobexec/QuartzJobTest.java @@ -1,11 +1,13 @@ package org.databiosphere.workspacedataservice.jobexec; import static org.assertj.core.api.Assertions.assertThat; +import static org.databiosphere.workspacedataservice.generated.GenericJobServerModel.StatusEnum; import static org.databiosphere.workspacedataservice.shared.model.Schedulable.ARG_TOKEN; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertInstanceOf; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyString; @@ -35,7 +37,6 @@ import org.junit.jupiter.api.TestInstance; import org.quartz.JobDataMap; import org.quartz.JobExecutionContext; -import org.quartz.JobExecutionException; import org.quartz.JobKey; import org.quartz.impl.JobDetailImpl; import org.springframework.beans.factory.annotation.Autowired; @@ -66,8 +67,6 @@ void beforeAll() { when(jobDao.createJob(any())).thenReturn(genericJobServerModel); when(jobDao.getJob(any())).thenReturn(genericJobServerModel); when(jobDao.updateStatus(any(), any())).thenReturn(genericJobServerModel); - when(jobDao.fail(any(), any(Exception.class))) - .thenThrow(new RuntimeException("test failed via jobDao.fail()")); when(jobDao.fail(any(), anyString())) .thenThrow(new RuntimeException("test failed via jobDao.fail()")); } @@ -88,12 +87,19 @@ void beforeEach() { class TestableQuartzJob extends QuartzJob { private final String expectedToken; + private boolean shouldThrowError = false; public TestableQuartzJob(String expectedToken, ObservationRegistry registry) { super(registry); this.expectedToken = expectedToken; } + public TestableQuartzJob( + String expectedToken, ObservationRegistry registry, boolean shouldThrowError) { + this(expectedToken, registry); + this.shouldThrowError = shouldThrowError; + } + @Override protected JobDao getJobDao() { return jobDao; @@ -102,11 +108,18 @@ protected JobDao getJobDao() { @Override protected void executeInternal(UUID jobId, JobExecutionContext context) { assertEquals(expectedToken, TokenContextUtil.getToken().getValue()); + try { + if (shouldThrowError) { + throw new JobExecutionException("Forced job to fail"); + } + } catch (Exception e) { + throw new JobExecutionException("Forced job to fail", e); + } } } @Test - void tokenIsStashedAndCleaned() throws JobExecutionException { + void tokenIsStashedAndCleaned() throws org.quartz.JobExecutionException { // set an example token, via mock, into the Quartz JobDataMap String expectedToken = RandomStringUtils.randomAlphanumeric(10); JobExecutionContext mockContext = setUpTestJob(expectedToken, UUID.randomUUID().toString()); @@ -122,7 +135,7 @@ void tokenIsStashedAndCleaned() throws JobExecutionException { } @Test - void correctObservation() throws JobExecutionException { + void correctObservation() throws org.quartz.JobExecutionException { String randomToken = RandomStringUtils.randomAlphanumeric(10); String jobUuid = UUID.randomUUID().toString(); JobExecutionContext mockContext = setUpTestJob(randomToken, jobUuid); @@ -136,17 +149,18 @@ void correctObservation() throws JobExecutionException { TestObservationRegistryAssert.assertThat(testObservationRegistry) .doesNotHaveAnyRemainingCurrentObservation() + .hasNumberOfObservationsWithNameEqualTo("wds.job.execute", 1) .hasObservationWithNameEqualTo("wds.job.execute") .that() .hasHighCardinalityKeyValue("jobId", jobUuid) .hasLowCardinalityKeyValue("jobType", "TestableQuartzJob") - .hasLowCardinalityKeyValue("outcome", "success") + .hasLowCardinalityKeyValue("outcome", StatusEnum.SUCCEEDED.getValue()) .hasBeenStarted() .hasBeenStopped(); } @Test - void correctMetrics() throws JobExecutionException { + void correctMetrics() throws org.quartz.JobExecutionException { String randomToken = RandomStringUtils.randomAlphanumeric(10); JobExecutionContext mockContext = setUpTestJob(randomToken, UUID.randomUUID().toString()); @@ -188,6 +202,36 @@ void correctMetrics() throws JobExecutionException { } } + @Test + void observationLogsFailure() throws JobExecutionException { + String randomToken = RandomStringUtils.randomAlphanumeric(10); + String jobUuid = UUID.randomUUID().toString(); + JobExecutionContext mockContext = setUpTestJob(randomToken, jobUuid); + + // this test requires a TestObservationRegistry + TestObservationRegistry testObservationRegistry = + assertInstanceOf(TestObservationRegistry.class, observationRegistry); + + // execute the TestableQuartzJob, then confirm observation recorded failure + assertThrows( + JobExecutionException.class, + () -> + new TestableQuartzJob(randomToken, observationRegistry, /* shouldThrowError= */ true) + .execute(mockContext)); + + TestObservationRegistryAssert.assertThat(testObservationRegistry) + .doesNotHaveAnyRemainingCurrentObservation() + .hasNumberOfObservationsWithNameEqualTo("wds.job.execute", 1) + .hasObservationWithNameEqualTo("wds.job.execute") + .that() + .hasHighCardinalityKeyValue("jobId", jobUuid) + .hasLowCardinalityKeyValue("jobType", "TestableQuartzJob") + .hasLowCardinalityKeyValue("outcome", StatusEnum.ERROR.getValue()) + .hasLowCardinalityKeyValue("error", "Forced job to fail") + .hasBeenStarted() + .hasBeenStopped(); + } + // sets up a job and returns the job context private JobExecutionContext setUpTestJob(String randomToken, String jobUuid) { // String jobUuid = UUID.randomUUID().toString(); diff --git a/service/src/test/java/org/databiosphere/workspacedataservice/service/RecordServiceTest.java b/service/src/test/java/org/databiosphere/workspacedataservice/service/RecordServiceTest.java index 78a06f670..dfe36bae6 100644 --- a/service/src/test/java/org/databiosphere/workspacedataservice/service/RecordServiceTest.java +++ b/service/src/test/java/org/databiosphere/workspacedataservice/service/RecordServiceTest.java @@ -1,5 +1,10 @@ package org.databiosphere.workspacedataservice.service; +import static org.databiosphere.workspacedataservice.service.RecordService.TAG_ATTRIBUTE_NAME; +import static org.databiosphere.workspacedataservice.service.RecordService.TAG_INSTANCE; +import static org.databiosphere.workspacedataservice.service.RecordService.TAG_NEW_DATATYPE; +import static org.databiosphere.workspacedataservice.service.RecordService.TAG_OLD_DATATYPE; +import static org.databiosphere.workspacedataservice.service.RecordService.TAG_RECORD_TYPE; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertInstanceOf; @@ -90,10 +95,10 @@ void schemaChangesIncrementMetricsCounter() { // assert the counter has been incremented once assertEquals(1, counter.count()); // validate the tags for that counter - assertEquals(recordType.getName(), counter.getId().getTag("RecordType")); - assertEquals("myAttr", counter.getId().getTag("AttributeName")); - assertEquals(instanceId.toString(), counter.getId().getTag("Instance")); - assertEquals(DataTypeMapping.NUMBER.toString(), counter.getId().getTag("OldDataType")); - assertEquals(DataTypeMapping.STRING.toString(), counter.getId().getTag("NewDataType")); + assertEquals(recordType.getName(), counter.getId().getTag(TAG_RECORD_TYPE)); + assertEquals("myAttr", counter.getId().getTag(TAG_ATTRIBUTE_NAME)); + assertEquals(instanceId.toString(), counter.getId().getTag(TAG_INSTANCE)); + assertEquals(DataTypeMapping.NUMBER.toString(), counter.getId().getTag(TAG_OLD_DATATYPE)); + assertEquals(DataTypeMapping.STRING.toString(), counter.getId().getTag(TAG_NEW_DATATYPE)); } } From 369523fb6fffc9ce0732f35e9161348d9c5beb85 Mon Sep 17 00:00:00 2001 From: Adina Shanholtz Date: Wed, 31 Jan 2024 10:52:18 -0800 Subject: [PATCH 11/14] first attempt to fix test --- .../workspacedataservice/jobexec/QuartzJobTest.java | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/service/src/test/java/org/databiosphere/workspacedataservice/jobexec/QuartzJobTest.java b/service/src/test/java/org/databiosphere/workspacedataservice/jobexec/QuartzJobTest.java index e99328434..d433013ce 100644 --- a/service/src/test/java/org/databiosphere/workspacedataservice/jobexec/QuartzJobTest.java +++ b/service/src/test/java/org/databiosphere/workspacedataservice/jobexec/QuartzJobTest.java @@ -7,7 +7,6 @@ import static org.junit.jupiter.api.Assertions.assertInstanceOf; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNull; -import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyString; @@ -203,7 +202,7 @@ void correctMetrics() throws org.quartz.JobExecutionException { } @Test - void observationLogsFailure() throws JobExecutionException { + void observationLogsFailure() throws org.quartz.JobExecutionException { String randomToken = RandomStringUtils.randomAlphanumeric(10); String jobUuid = UUID.randomUUID().toString(); JobExecutionContext mockContext = setUpTestJob(randomToken, jobUuid); @@ -213,11 +212,8 @@ void observationLogsFailure() throws JobExecutionException { assertInstanceOf(TestObservationRegistry.class, observationRegistry); // execute the TestableQuartzJob, then confirm observation recorded failure - assertThrows( - JobExecutionException.class, - () -> - new TestableQuartzJob(randomToken, observationRegistry, /* shouldThrowError= */ true) - .execute(mockContext)); + new TestableQuartzJob(randomToken, observationRegistry, /* shouldThrowError= */ true) + .execute(mockContext); TestObservationRegistryAssert.assertThat(testObservationRegistry) .doesNotHaveAnyRemainingCurrentObservation() From 3df240c0f0d7e9fa8c194cd2663e1e45f667ef75 Mon Sep 17 00:00:00 2001 From: Adina Shanholtz Date: Wed, 31 Jan 2024 12:34:01 -0800 Subject: [PATCH 12/14] error tag not populated on test, use haserror instead --- .../workspacedataservice/jobexec/QuartzJobTest.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/service/src/test/java/org/databiosphere/workspacedataservice/jobexec/QuartzJobTest.java b/service/src/test/java/org/databiosphere/workspacedataservice/jobexec/QuartzJobTest.java index d433013ce..0f8e75528 100644 --- a/service/src/test/java/org/databiosphere/workspacedataservice/jobexec/QuartzJobTest.java +++ b/service/src/test/java/org/databiosphere/workspacedataservice/jobexec/QuartzJobTest.java @@ -223,9 +223,11 @@ void observationLogsFailure() throws org.quartz.JobExecutionException { .hasHighCardinalityKeyValue("jobId", jobUuid) .hasLowCardinalityKeyValue("jobType", "TestableQuartzJob") .hasLowCardinalityKeyValue("outcome", StatusEnum.ERROR.getValue()) - .hasLowCardinalityKeyValue("error", "Forced job to fail") .hasBeenStarted() - .hasBeenStopped(); + .hasBeenStopped() + .hasError() + .thenError() + .hasMessage("Forced job to fail"); } // sets up a job and returns the job context From e1499f3ce5f8bdb8a4298f0133d873bd78ec0e0f Mon Sep 17 00:00:00 2001 From: Adina Shanholtz Date: Wed, 31 Jan 2024 12:48:05 -0800 Subject: [PATCH 13/14] whoops missed some --- .../workspacedataservice/service/RecordService.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/service/src/main/java/org/databiosphere/workspacedataservice/service/RecordService.java b/service/src/main/java/org/databiosphere/workspacedataservice/service/RecordService.java index e05680fd6..32e08dd39 100644 --- a/service/src/main/java/org/databiosphere/workspacedataservice/service/RecordService.java +++ b/service/src/main/java/org/databiosphere/workspacedataservice/service/RecordService.java @@ -225,10 +225,10 @@ public Map addOrUpdateColumnIfNeeded( Counter counter = Counter.builder(COUNTER_COL_CHANGE) .tag(TAG_RECORD_TYPE, recordType.getName()) - .tag("AttributeName", column) + .tag(TAG_ATTRIBUTE_NAME, column) .tag(TAG_INSTANCE, instanceId.toString()) - .tag("OldDataType", valueDifference.leftValue().toString()) - .tag("NewDataType", updatedColType.toString()) + .tag(TAG_OLD_DATATYPE, valueDifference.leftValue().toString()) + .tag(TAG_NEW_DATATYPE, updatedColType.toString()) .description("Column schema changes") .register(meterRegistry); counter.increment(); From f84030bdd3e473b5c7fadf23fb0821a2a186e55d Mon Sep 17 00:00:00 2001 From: Adina Shanholtz Date: Thu, 1 Feb 2024 09:44:24 -0800 Subject: [PATCH 14/14] simplify exception Co-authored-by: David An --- .../workspacedataservice/jobexec/QuartzJobTest.java | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/service/src/test/java/org/databiosphere/workspacedataservice/jobexec/QuartzJobTest.java b/service/src/test/java/org/databiosphere/workspacedataservice/jobexec/QuartzJobTest.java index 0f8e75528..1a6dff44f 100644 --- a/service/src/test/java/org/databiosphere/workspacedataservice/jobexec/QuartzJobTest.java +++ b/service/src/test/java/org/databiosphere/workspacedataservice/jobexec/QuartzJobTest.java @@ -107,12 +107,8 @@ protected JobDao getJobDao() { @Override protected void executeInternal(UUID jobId, JobExecutionContext context) { assertEquals(expectedToken, TokenContextUtil.getToken().getValue()); - try { - if (shouldThrowError) { - throw new JobExecutionException("Forced job to fail"); - } - } catch (Exception e) { - throw new JobExecutionException("Forced job to fail", e); + if (shouldThrowError) { + throw new JobExecutionException("Forced job to fail"); } } }