Record number of skipped executions during aggregation #486

kathy-t · 2024-02-26T14:46:53Z

Description
Corresponding PRs:

This PR modifies the metrics aggregator so that it aggregates workflow executions even when there are invalid executions (it previously skipped aggregating the metric entirely) and records the number of executions that were skipped. It validates executions using the validation constraints defined in the webservice metrics objects.

I did some minor refactoring that involved changing the interfaces to abstract classes to remove unnecessary task methods from the Validation Status aggregator which don't have the concept of task metrics.

Review Instructions
See dockstore/dockstore-ui2#1938

Issue
SEAB-6046

Security
If there are any concerns that require extra attention from the security team, highlight them here.

Please make sure that you've checked the following before submitting your pull request. Thanks!

Check that you pass the basic style checks and unit tests by running mvn clean install in the project that you have modified (until https://ucsc-cgl.atlassian.net/browse/SEAB-5300 adds multi-module support properly)
Ensure that the PR targets the correct branch. Check the milestone or fix version of the ticket.
If you are changing dependencies, check with dependabot to ensure you are not introducing new high/critical vulnerabilities
If this PR is for a user-facing feature, create and link a documentation ticket for this feature (usually in the same milestone as the linked issue). Style points if you create a documentation PR directly and link that instead.

codecov · 2024-02-26T15:14:39Z

Codecov Report

Attention: Patch coverage is 87.85047% with 13 lines in your changes are missing coverage. Please review.

Project coverage is 56.02%. Comparing base (64ddd19) to head (6696cc6).

Files	Patch %	Lines
...store/metricsaggregator/helper/CostAggregator.java	72.72%	1 Missing and 2 partials ⚠️
...csaggregator/helper/ExecutionStatusAggregator.java	78.57%	1 Missing and 2 partials ⚠️
...saggregator/helper/ValidationStatusAggregator.java	62.50%	2 Missing and 1 partial ⚠️
...ore/metricsaggregator/helper/MemoryAggregator.java	77.77%	1 Missing and 1 partial ⚠️
...kstore/metricsaggregator/helper/CpuAggregator.java	87.50%	1 Missing ⚠️
...ricsaggregator/helper/ExecutionTimeAggregator.java	90.00%	1 Missing ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             develop     #486      +/-   ##
=============================================
- Coverage      56.41%   56.02%   -0.40%     
- Complexity       295      297       +2     
=============================================
  Files             32       33       +1     
  Lines           1964     2001      +37     
  Branches         166      163       -3     
=============================================
+ Hits            1108     1121      +13     
- Misses           779      796      +17     
- Partials          77       84       +7

Flag	Coverage Δ
metricsaggregator	`48.22% <87.85%> (-0.25%)`	⬇️
toolbackup	`30.73% <67.28%> (+0.03%)`	⬆️
tooltester	`22.93% <67.28%> (+0.17%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

kathy-t · 2024-02-26T15:13:19Z

metricsaggregator/src/main/java/io/dockstore/metricsaggregator/helper/ExecutionAggregator.java

-        return executions.stream()
-                .map(this::getMetricFromExecution)
+
+    public final Optional<M> getAggregatedMetricFromExecutions(List<T> executions) {


Method that calculates the aggregated metric plus sets the number of executions skipped.

kathy-t · 2024-02-26T15:13:22Z

metricsaggregator/src/main/java/io/dockstore/metricsaggregator/helper/ExecutionAggregator.java

+     * @param aggregatedMetrics
+     * @return
+     */
+    public final Optional<M> getAggregatedMetricsFromAggregatedMetrics(List<M> aggregatedMetrics) {


Method that calculates the aggregated metric plus sets the number of executions skipped.

Since it's calculating a single aggregated metric, should it be named getAggregatedMetricFromAggregatedMetrics?

There is an instance where it calculates multiple aggregated metrics into a single aggregated metric:

dockstore-support/metricsaggregator/src/main/java/io/dockstore/metricsaggregator/MetricsAggregatorS3Client.java

Lines 131 to 133 in 64ddd19

// Calculate metrics across all platforms by aggregating the aggregated metrics from each platform

try {

getAggregatedMetrics(allMetrics).ifPresent(metrics -> {

kathy-t · 2024-02-26T15:15:32Z

...csaggregator/src/main/java/io/dockstore/metricsaggregator/helper/RunExecutionAggregator.java

-     * @param metrics
-     * @return
-     */
-    M getMetricFromMetrics(Metrics metrics);


This method was moved into ExecutionAggregator

kathy-t · 2024-02-26T15:16:12Z

...csaggregator/src/main/java/io/dockstore/metricsaggregator/helper/RunExecutionAggregator.java

-     * @param metricsList
-     * @return
-     */
-    default Optional<M> getAggregatedMetricFromMetricsList(List<Metrics> metricsList) {


This method was moved to ExecutionAggregator

kathy-t · 2024-02-26T15:18:15Z

...csaggregator/src/main/java/io/dockstore/metricsaggregator/helper/RunExecutionAggregator.java

     * @return
     */
-    List<T> getExecutionsFromExecutionRequestBody(ExecutionsRequestBody executionsRequestBody);


Was moved into ExecutionAggregator

kathy-t · 2024-02-26T15:19:47Z

...gator/src/main/java/io/dockstore/metricsaggregator/helper/ValidationExecutionAggregator.java

+     * @param allSubmissions
+     * @return
+     */
+    public Optional<M> getAggregatedMetricFromAllSubmissions(ExecutionsRequestBody allSubmissions) {


This is like the getAggregatedMetricFromAllSubmissions default method that was in the original ExecutionsRequestBodyAggregator interface except it completely removes the invokation of the task methods since tasks are not applicable to validation executions

kathy-t · 2024-02-26T15:20:15Z

pom.xml

        <changelist>.0-SNAPSHOT</changelist>

        <github.url>scm:git:git@github.com:dockstore/dockstore-support.git</github.url>
        <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
-        <dockstore-core.version>1.15.0-rc.2</dockstore-core.version>
+        <dockstore-core.version>1.16.0-SNAPSHOT</dockstore-core.version>


Will update this to a tag once the webservice PR is merged

…nstraints

svonworl · 2024-02-29T18:19:40Z

metricsaggregator/src/main/java/io/dockstore/metricsaggregator/helper/ExecutionAggregator.java

+     */
+    public boolean isValid(T execution) {
+        Set<ConstraintViolation<T>> violations = validator.validate(execution);
+        return violations.stream().noneMatch(violation -> violation.getPropertyPath().toString().startsWith(getPropertyPathToValidate()));


Is the string-based comparison vulnerable to problems like getPropertyPath().toString() being foobar and getPropertyPathToValidate() being foo? Violation has a getNode method containing the name, I think, might be safer use that. In practice, probably doesn't matter tho.

Yes it's possible, but I intentionally used the startsWith comparison because for a Cost value violation, the property path is cost.value, not just cost. I could define the CostAggregator's getPropertyPathToValidate() as cost.value, but we might allow users to provide their own currency in the future so I figured it would be best to be generic and use startsWith

metricsaggregator/src/main/java/io/dockstore/metricsaggregator/helper/ExecutionAggregator.java

svonworl · 2024-02-29T18:27:31Z

metricsaggregator/src/main/java/io/dockstore/metricsaggregator/helper/ExecutionAggregator.java

+     * @param aggregatedMetrics
+     * @return
+     */
+    public final Optional<M> getAggregatedMetricsFromAggregatedMetrics(List<M> aggregatedMetrics) {


Since it's calculating a single aggregated metric, should it be named getAggregatedMetricFromAggregatedMetrics?

denis-yuen

Potential follow-up ticket, but before that (and the aggregator getting much more sophisticated, we should do some AWS Athena prototyping)

denis-yuen · 2024-03-06T15:58:03Z

metricsaggregator/pom.xml

@@ -297,6 +301,7 @@
                                <usedDependency>ch.qos.logback:logback-classic</usedDependency>
                                <usedDependency>ch.qos.logback:logback-core</usedDependency>
                                <usedDependency>com.google.guava:guava</usedDependency>
+                                <usedDependency>jakarta.validation:jakarta.validation-api</usedDependency>


A little weird since you have

import jakarta.validation.ConstraintViolation; import jakarta.validation.Validation; import jakarta.validation.Validator;

pretty straightforward in the code

Yeah I don't know why the build doesn't recognize that the dependency is being used 🤔

denis-yuen · 2024-03-06T16:01:14Z

metricsaggregator/src/test/java/io/dockstore/metricsaggregator/helper/CpuAggregatorTest.java

@@ -70,4 +71,10 @@ void testGetAggregatedMetricFromWorkflowExecutions() {
        assertEquals(4.0, cpuMetric.get().getAverage());
        assertEquals(3, cpuMetric.get().getNumberOfDataPointsForAverage());
    }
+
+    private RunExecution createRunExecution(Integer cpuRequirements) {


Kinda liked the old way, could use a proper builder object design pattern though.
Could be followup ticket.

denis-yuen · 2024-03-06T16:02:05Z

...gator/src/test/java/io/dockstore/metricsaggregator/helper/ExecutionStatusAggregatorTest.java

        costMetric = executionStatusMetric.get().getCount().get(SUCCESSFUL.name()).getCost();
        assertNotNull(costMetric);
        assertEquals(2.0, costMetric.getMinimum());
        assertEquals(6.0, costMetric.getMaximum()); // The max is the cost of the two tasks summed together
        assertEquals(4.0, costMetric.getAverage());
        assertEquals(2, costMetric.getNumberOfDataPointsForAverage()); // Two data points: 1 from the workflow execution and 1 for the list of tasks
    }
+
+    private RunExecution createRunExecutionWithExecutionTime(ExecutionStatus executionStatus, String executionTime) {


Ditto here, I think this would be cleaner with a builder

kathy-t · 2024-03-07T16:41:33Z

(build will fail until I update the webservice version to a tag)

sonarcloud · 2024-03-07T20:22:42Z

Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

kathy-t added 3 commits February 23, 2024 12:55

Count number of skipped executions

5e68df0

Clean up

26df5da

Refactor some things

11a89d6

kathy-t self-assigned this Feb 26, 2024

kathy-t added 3 commits February 26, 2024 09:56

Clean up

648f988

Add clarification

cadb648

Put this back

28e6686

kathy-t commented Feb 26, 2024

View reviewed changes

This was referenced Feb 26, 2024

Record number of skipped executions for aggregated metrics dockstore/dockstore#5822

Merged

Display skipped executions warning for metrics dockstore/dockstore-ui2#1938

Merged

kathy-t added 2 commits February 28, 2024 11:33

Merge branch 'develop' into feature/seab-6046/aggregate-even-if-invalid

3fd2cd5

Validate execution metrics using webservice's Java Bean validation co…

cdccbb5

…nstraints

kathy-t marked this pull request as ready for review February 29, 2024 14:54

kathy-t requested review from svonworl, denis-yuen, coverbeck and hyunnaye February 29, 2024 14:55

svonworl reviewed Feb 29, 2024

View reviewed changes

coverbeck approved these changes Mar 1, 2024

View reviewed changes

Merge branch 'develop' into feature/seab-6046/aggregate-even-if-invalid

6f31449

denis-yuen approved these changes Mar 6, 2024

View reviewed changes

PR feedback

f007056

kathy-t requested a review from svonworl March 7, 2024 16:41

Use 1.16.0-alpha.2 webservice version

6696cc6

svonworl approved these changes Mar 8, 2024

View reviewed changes

kathy-t merged commit c751c2e into develop Mar 8, 2024
9 of 10 checks passed

kathy-t deleted the feature/seab-6046/aggregate-even-if-invalid branch March 8, 2024 17:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Record number of skipped executions during aggregation #486

Record number of skipped executions during aggregation #486

kathy-t commented Feb 26, 2024 •

edited

Loading

codecov bot commented Feb 26, 2024 •

edited

Loading

kathy-t Feb 26, 2024

kathy-t Feb 26, 2024

svonworl Feb 29, 2024

kathy-t Mar 6, 2024

kathy-t Feb 26, 2024

kathy-t Feb 26, 2024

kathy-t Feb 26, 2024

kathy-t Feb 26, 2024 •

edited

Loading

kathy-t Feb 26, 2024

svonworl Feb 29, 2024

kathy-t Mar 6, 2024

svonworl Feb 29, 2024

denis-yuen left a comment

denis-yuen Mar 6, 2024

kathy-t Mar 7, 2024

denis-yuen Mar 6, 2024

denis-yuen Mar 6, 2024

kathy-t commented Mar 7, 2024

sonarcloud bot commented Mar 7, 2024

	// Calculate metrics across all platforms by aggregating the aggregated metrics from each platform
	try {
	getAggregatedMetrics(allMetrics).ifPresent(metrics -> {

Record number of skipped executions during aggregation #486

Record number of skipped executions during aggregation #486

Conversation

kathy-t commented Feb 26, 2024 • edited Loading

codecov bot commented Feb 26, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kathy-t Feb 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

denis-yuen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kathy-t commented Mar 7, 2024

sonarcloud bot commented Mar 7, 2024

Quality Gate failed

kathy-t commented Feb 26, 2024 •

edited

Loading

codecov bot commented Feb 26, 2024 •

edited

Loading

kathy-t Feb 26, 2024 •

edited

Loading