[OPIK-287] Add project level aggregations #894

thiagohora · 2024-12-14T15:52:39Z

Details

Add project-level aggregations

Issues

OPIK-287

apps/opik-backend/src/main/java/com/comet/opik/domain/ProjectService.java

...-backend/src/test/java/com/comet/opik/api/resources/utils/resources/TraceResourceClient.java

apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/ProjectsResourceTest.java

idoberko2

Left some comments related to lastUpdatedTraceAt logic. Other than that, LGTM.

This reverts commit d9a1f68.

andrescrz

Let's double check the floating type for the total cost before moving forward with this PR.

andrescrz · 2024-12-19T09:50:39Z

apps/opik-backend/src/main/java/com/comet/opik/api/Project.java

+        @JsonView({
+                Project.View.Public.class}) @Schema(accessMode = Schema.AccessMode.READ_ONLY) @Nullable PercentageValues duration,
+        @JsonView({
+                Project.View.Public.class}) @Schema(accessMode = Schema.AccessMode.READ_ONLY) @Nullable Double totalEstimatedCost,


Monetary cost as floating type. For other aggregations such as avg, it wouldn't be that bad. But for a total, I assume is a sum, better as BigDecimal.

The name is a bit misleading. This holds the average total cost of a trace in a project. This is calculated here:

opik/apps/opik-backend/src/main/java/com/comet/opik/domain/TraceDAO.java

Line 615 in ab0f8bb

avgIf(total_estimated_cost, total_estimated_cost > 0) AS total_estimated_cost_,

In addition, you can see that the coverage for this logic asserts on an average:

opik/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/ProjectsResourceTest.java

Lines 1358 to 1361 in ab0f8bb

return traces.stream()

.map(Trace::totalEstimatedCost)

.reduce(BigDecimal.ZERO, BigDecimal::add)

.divide(BigDecimal.valueOf(count), ValidationUtils.SCALE, RoundingMode.HALF_UP).doubleValue();

Can you rename it to something more significative and less confusing then? e.g: avgTotalEstimatedCost or similar.

These are arguable not clear names, in comparison with other aggregations such as lastUpdatedTraceAt. On the other hand, List<FeedbackScoreAverage> feedbackScores, already exists in Experiments.

Not a blocker so far.

apps/opik-backend/src/main/java/com/comet/opik/domain/ProjectService.java

apps/opik-backend/src/main/java/com/comet/opik/domain/TraceDAO.java

apps/opik-backend/src/test/java/com/comet/opik/api/resources/utils/BigDecimalCollectors.java

andrescrz · 2024-12-19T09:57:22Z

apps/opik-backend/src/test/java/com/comet/opik/api/resources/utils/StatsUtils.java

@@ -318,4 +319,50 @@ public static int bigDecimalComparator(BigDecimal v1, BigDecimal v2) {
        return strippedV1.toBigInteger().compareTo(strippedV2.toBigInteger());
    }

+    public static int closeToEpsilonComparator(Object v1, Object v2) {


Most likely, we don't need this. There are assertJ assertions such as isCloseTo that allow different types of approximations e.g: based on percentage, boundaries etc.

Actually I can't find a way to make it using assertj's isCloseTo due to the recursive comparison. I can try to simplify closeToEpsilonComparator's logic if you think it's worth it.

It's fine to keep this then, as it's just a test. We attempt to get rid of it later.

andrescrz

Just the debate on the names. Everything else is good to go.

andrescrz · 2024-12-23T11:47:33Z

apps/opik-backend/src/main/java/com/comet/opik/api/Project.java

+        @JsonView({
+                Project.View.Public.class}) @Schema(accessMode = Schema.AccessMode.READ_ONLY) @Nullable PercentageValues duration,
+        @JsonView({
+                Project.View.Public.class}) @Schema(accessMode = Schema.AccessMode.READ_ONLY) @Nullable Double totalEstimatedCost,


These are arguable not clear names, in comparison with other aggregations such as lastUpdatedTraceAt. On the other hand, List<FeedbackScoreAverage> feedbackScores, already exists in Experiments.

Not a blocker so far.

andrescrz · 2024-12-23T11:48:02Z

apps/opik-backend/src/test/java/com/comet/opik/api/resources/utils/StatsUtils.java

@@ -318,4 +319,50 @@ public static int bigDecimalComparator(BigDecimal v1, BigDecimal v2) {
        return strippedV1.toBigInteger().compareTo(strippedV2.toBigInteger());
    }

+    public static int closeToEpsilonComparator(Object v1, Object v2) {


It's fine to keep this then, as it's just a test. We attempt to get rid of it later.

thiagohora force-pushed the thiagohora/OPIK-287_add_project_level_aggregations branch from 46b6d4f to fb94fbd Compare December 14, 2024 15:58

thiagohora marked this pull request as ready for review December 14, 2024 15:59

thiagohora requested a review from a team as a code owner December 14, 2024 15:59