From 0329a54a72e6f8964143386252e162408b059c79 Mon Sep 17 00:00:00 2001 From: Thiago Hora Date: Thu, 12 Dec 2024 15:34:57 +0100 Subject: [PATCH 01/12] [OPIK-446] Add durartion to trace_span --- .../src/main/java/com/comet/opik/api/Span.java | 14 ++++++++++++++ .../src/main/java/com/comet/opik/api/Trace.java | 14 ++++++++++++++ .../api/resources/v1/priv/TracesResourceTest.java | 2 ++ 3 files changed, 30 insertions(+) diff --git a/apps/opik-backend/src/main/java/com/comet/opik/api/Span.java b/apps/opik-backend/src/main/java/com/comet/opik/api/Span.java index 2ebc3b1cd9..d31afd38ea 100644 --- a/apps/opik-backend/src/main/java/com/comet/opik/api/Span.java +++ b/apps/opik-backend/src/main/java/com/comet/opik/api/Span.java @@ -2,6 +2,7 @@ import com.comet.opik.domain.SpanType; import com.fasterxml.jackson.annotation.JsonIgnoreProperties; +import com.fasterxml.jackson.annotation.JsonProperty; import com.fasterxml.jackson.annotation.JsonView; import com.fasterxml.jackson.databind.JsonNode; import com.fasterxml.jackson.databind.PropertyNamingStrategies; @@ -13,6 +14,7 @@ import lombok.Builder; import java.math.BigDecimal; +import java.time.Duration; import java.time.Instant; import java.util.List; import java.util.Map; @@ -73,4 +75,16 @@ public static class Write { public static class Public { } } + + @JsonProperty + @JsonView({Span.View.Public.class}) + @Schema(accessMode = Schema.AccessMode.READ_ONLY) + public Double duration() { + if (endTime == null) { + return null; + } + + long micros = Duration.between(startTime, endTime).toNanos() / 1_000; + return micros / 1_000.0; + } } diff --git a/apps/opik-backend/src/main/java/com/comet/opik/api/Trace.java b/apps/opik-backend/src/main/java/com/comet/opik/api/Trace.java index b5f04a696a..056f7b07b7 100644 --- a/apps/opik-backend/src/main/java/com/comet/opik/api/Trace.java +++ b/apps/opik-backend/src/main/java/com/comet/opik/api/Trace.java @@ -1,6 +1,7 @@ package com.comet.opik.api; import com.fasterxml.jackson.annotation.JsonIgnoreProperties; +import com.fasterxml.jackson.annotation.JsonProperty; import com.fasterxml.jackson.annotation.JsonView; import com.fasterxml.jackson.databind.JsonNode; import com.fasterxml.jackson.databind.PropertyNamingStrategies; @@ -12,6 +13,7 @@ import lombok.Builder; import java.math.BigDecimal; +import java.time.Duration; import java.time.Instant; import java.util.List; import java.util.Map; @@ -68,4 +70,16 @@ public static class Write { public static class Public { } } + + @JsonProperty + @JsonView({Span.View.Public.class}) + @Schema(accessMode = Schema.AccessMode.READ_ONLY) + public Double duration() { + if (endTime == null) { + return null; + } + + long micros = Duration.between(startTime, endTime).toNanos() / 1_000; + return micros / 1_000.0; + } } diff --git a/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/TracesResourceTest.java b/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/TracesResourceTest.java index 3c57167082..d3c2f13276 100644 --- a/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/TracesResourceTest.java +++ b/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/TracesResourceTest.java @@ -19,6 +19,8 @@ import com.comet.opik.api.filter.Field; import com.comet.opik.api.filter.Filter; import com.comet.opik.api.filter.Operator; +import com.comet.opik.api.filter.SpanField; +import com.comet.opik.api.filter.SpanFilter; import com.comet.opik.api.filter.TraceField; import com.comet.opik.api.filter.TraceFilter; import com.comet.opik.api.resources.utils.AuthTestUtils; From 1ba968fea3a552eea2cafbc6151d54132f093200 Mon Sep 17 00:00:00 2001 From: Thiago Hora Date: Thu, 12 Dec 2024 15:38:39 +0100 Subject: [PATCH 02/12] Fix calc --- .../main/java/com/comet/opik/api/Span.java | 11 +++------ .../main/java/com/comet/opik/api/Trace.java | 11 +++------ .../com/comet/opik/utils/DurationUtils.java | 23 +++++++++++++++++++ 3 files changed, 29 insertions(+), 16 deletions(-) create mode 100644 apps/opik-backend/src/main/java/com/comet/opik/utils/DurationUtils.java diff --git a/apps/opik-backend/src/main/java/com/comet/opik/api/Span.java b/apps/opik-backend/src/main/java/com/comet/opik/api/Span.java index d31afd38ea..427607cdb2 100644 --- a/apps/opik-backend/src/main/java/com/comet/opik/api/Span.java +++ b/apps/opik-backend/src/main/java/com/comet/opik/api/Span.java @@ -1,6 +1,7 @@ package com.comet.opik.api; import com.comet.opik.domain.SpanType; +import com.comet.opik.utils.DurationUtils; import com.fasterxml.jackson.annotation.JsonIgnoreProperties; import com.fasterxml.jackson.annotation.JsonProperty; import com.fasterxml.jackson.annotation.JsonView; @@ -14,7 +15,6 @@ import lombok.Builder; import java.math.BigDecimal; -import java.time.Duration; import java.time.Instant; import java.util.List; import java.util.Map; @@ -78,13 +78,8 @@ public static class Public { @JsonProperty @JsonView({Span.View.Public.class}) - @Schema(accessMode = Schema.AccessMode.READ_ONLY) + @Schema(accessMode = Schema.AccessMode.READ_ONLY, description = "Duration in milliseconds as a decimal number to support sub-millisecond precision") public Double duration() { - if (endTime == null) { - return null; - } - - long micros = Duration.between(startTime, endTime).toNanos() / 1_000; - return micros / 1_000.0; + return DurationUtils.getDurationInSeconds(startTime, endTime); } } diff --git a/apps/opik-backend/src/main/java/com/comet/opik/api/Trace.java b/apps/opik-backend/src/main/java/com/comet/opik/api/Trace.java index 056f7b07b7..6ab72f6fa1 100644 --- a/apps/opik-backend/src/main/java/com/comet/opik/api/Trace.java +++ b/apps/opik-backend/src/main/java/com/comet/opik/api/Trace.java @@ -1,5 +1,6 @@ package com.comet.opik.api; +import com.comet.opik.utils.DurationUtils; import com.fasterxml.jackson.annotation.JsonIgnoreProperties; import com.fasterxml.jackson.annotation.JsonProperty; import com.fasterxml.jackson.annotation.JsonView; @@ -13,7 +14,6 @@ import lombok.Builder; import java.math.BigDecimal; -import java.time.Duration; import java.time.Instant; import java.util.List; import java.util.Map; @@ -73,13 +73,8 @@ public static class Public { @JsonProperty @JsonView({Span.View.Public.class}) - @Schema(accessMode = Schema.AccessMode.READ_ONLY) + @Schema(accessMode = Schema.AccessMode.READ_ONLY, description = "Duration in milliseconds as a decimal number to support sub-millisecond precision") public Double duration() { - if (endTime == null) { - return null; - } - - long micros = Duration.between(startTime, endTime).toNanos() / 1_000; - return micros / 1_000.0; + return DurationUtils.getDurationInSeconds(startTime, endTime); } } diff --git a/apps/opik-backend/src/main/java/com/comet/opik/utils/DurationUtils.java b/apps/opik-backend/src/main/java/com/comet/opik/utils/DurationUtils.java new file mode 100644 index 0000000000..2fd55f9fd5 --- /dev/null +++ b/apps/opik-backend/src/main/java/com/comet/opik/utils/DurationUtils.java @@ -0,0 +1,23 @@ +package com.comet.opik.utils; + +import lombok.NonNull; +import lombok.experimental.UtilityClass; + +import java.time.Duration; +import java.time.Instant; + +@UtilityClass +public class DurationUtils { + + public static final Double TIME_UNIT = 1_000.0; + + public static Double getDurationInSeconds(@NonNull Instant startTime, Instant endTime) { + if (endTime == null) { + return null; + } + + long micros = Duration.between(startTime, endTime).toNanos() / TIME_UNIT.longValue(); + return micros / TIME_UNIT; + } + +} From 115f6e29268eb35dfe85e6ae6831d334177ac90a Mon Sep 17 00:00:00 2001 From: Thiago Hora Date: Fri, 13 Dec 2024 09:39:04 +0100 Subject: [PATCH 03/12] Fix pr review --- .../main/java/com/comet/opik/api/Span.java | 2 +- .../main/java/com/comet/opik/api/Trace.java | 2 +- .../com/comet/opik/utils/DurationUtils.java | 2 +- .../resources/v1/priv/SpansResourceTest.java | 6 +- .../resources/v1/priv/TracesResourceTest.java | 87 +++++++++++++++++-- 5 files changed, 89 insertions(+), 10 deletions(-) diff --git a/apps/opik-backend/src/main/java/com/comet/opik/api/Span.java b/apps/opik-backend/src/main/java/com/comet/opik/api/Span.java index 427607cdb2..d75842f042 100644 --- a/apps/opik-backend/src/main/java/com/comet/opik/api/Span.java +++ b/apps/opik-backend/src/main/java/com/comet/opik/api/Span.java @@ -80,6 +80,6 @@ public static class Public { @JsonView({Span.View.Public.class}) @Schema(accessMode = Schema.AccessMode.READ_ONLY, description = "Duration in milliseconds as a decimal number to support sub-millisecond precision") public Double duration() { - return DurationUtils.getDurationInSeconds(startTime, endTime); + return DurationUtils.getDurationInMillisWithSubMilliPrecision(startTime, endTime); } } diff --git a/apps/opik-backend/src/main/java/com/comet/opik/api/Trace.java b/apps/opik-backend/src/main/java/com/comet/opik/api/Trace.java index 6ab72f6fa1..c42a403644 100644 --- a/apps/opik-backend/src/main/java/com/comet/opik/api/Trace.java +++ b/apps/opik-backend/src/main/java/com/comet/opik/api/Trace.java @@ -75,6 +75,6 @@ public static class Public { @JsonView({Span.View.Public.class}) @Schema(accessMode = Schema.AccessMode.READ_ONLY, description = "Duration in milliseconds as a decimal number to support sub-millisecond precision") public Double duration() { - return DurationUtils.getDurationInSeconds(startTime, endTime); + return DurationUtils.getDurationInMillisWithSubMilliPrecision(startTime, endTime); } } diff --git a/apps/opik-backend/src/main/java/com/comet/opik/utils/DurationUtils.java b/apps/opik-backend/src/main/java/com/comet/opik/utils/DurationUtils.java index 2fd55f9fd5..74ddd85010 100644 --- a/apps/opik-backend/src/main/java/com/comet/opik/utils/DurationUtils.java +++ b/apps/opik-backend/src/main/java/com/comet/opik/utils/DurationUtils.java @@ -11,7 +11,7 @@ public class DurationUtils { public static final Double TIME_UNIT = 1_000.0; - public static Double getDurationInSeconds(@NonNull Instant startTime, Instant endTime) { + public static Double getDurationInMillisWithSubMilliPrecision(@NonNull Instant startTime, Instant endTime) { if (endTime == null) { return null; } diff --git a/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/SpansResourceTest.java b/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/SpansResourceTest.java index 6cb42d1055..3ecefeb7a5 100644 --- a/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/SpansResourceTest.java +++ b/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/SpansResourceTest.java @@ -7605,7 +7605,8 @@ static Stream getSpanStats__whenFilterInvalidOperatorForFieldType__thenR .field(SpanField.DURATION) .operator(Operator.STARTS_WITH) .value("1") - .build()); + .build() + ); } @ParameterizedTest @@ -7719,7 +7720,8 @@ static Stream getSpanStats__whenFilterInvalidValueOrKeyForFieldType__the .field(SpanField.DURATION) .operator(Operator.EQUAL) .value(RandomStringUtils.randomAlphanumeric(5)) - .build()); + .build() + ); } @ParameterizedTest diff --git a/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/TracesResourceTest.java b/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/TracesResourceTest.java index d3c2f13276..5b51ebe6ac 100644 --- a/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/TracesResourceTest.java +++ b/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/TracesResourceTest.java @@ -19,8 +19,6 @@ import com.comet.opik.api.filter.Field; import com.comet.opik.api.filter.Filter; import com.comet.opik.api.filter.Operator; -import com.comet.opik.api.filter.SpanField; -import com.comet.opik.api.filter.SpanFilter; import com.comet.opik.api.filter.TraceField; import com.comet.opik.api.filter.TraceFilter; import com.comet.opik.api.resources.utils.AuthTestUtils; @@ -3082,7 +3080,8 @@ static Stream getTracesByProject__whenFilterInvalidOperatorForFieldType_ .field(TraceField.DURATION) .operator(Operator.NOT_CONTAINS) .value("1") - .build()); + .build() + ); } @ParameterizedTest @@ -6817,6 +6816,82 @@ void getTraceStats__whenFilterByDuration__thenReturnTracesFiltered(Operator oper getStatsAndAssert(projectName, null, filters, apiKey, workspaceName, expectedStats); } + Stream getTraceStats__whenFilterByDuration__thenReturnTracesFiltered() { + return Stream.of( + arguments(Operator.EQUAL, + Duration.ofMillis(1L).toNanos() / 1000, 1.0), + arguments(Operator.GREATER_THAN, + Duration.ofMillis(8L).toNanos() / 1000, 7.0), + arguments(Operator.GREATER_THAN_EQUAL, + Duration.ofMillis(1L).toNanos() / 1000, 1.0), + arguments(Operator.GREATER_THAN_EQUAL, + Duration.ofMillis(1L).plusNanos(1000).toNanos() / 1000, 1.0), + arguments(Operator.LESS_THAN, + Duration.ofMillis(1L).plusNanos(1).toNanos() / 1000, 2.0), + arguments(Operator.LESS_THAN_EQUAL, + Duration.ofMillis(1L).toNanos() / 1000, 1.0), + arguments(Operator.LESS_THAN_EQUAL, + Duration.ofMillis(1L).toNanos() / 1000, 2.0)); + } + + @ParameterizedTest + @MethodSource + void getTraceStats__whenFilterByDuration__thenReturnTracesFiltered(Operator operator, long end, double duration) { + String workspaceName = UUID.randomUUID().toString(); + String workspaceId = UUID.randomUUID().toString(); + String apiKey = UUID.randomUUID().toString(); + + mockTargetWorkspace(apiKey, workspaceName, workspaceId); + + var projectName = generator.generate().toString(); + var traces = PodamFactoryUtils.manufacturePojoList(factory, Trace.class) + .stream() + .map(trace -> { + Instant now = Instant.now(); + return trace.toBuilder() + .projectId(null) + .usage(null) + .projectName(projectName) + .feedbackScores(null) + .totalEstimatedCost(BigDecimal.ZERO) + .startTime(now) + .endTime(Set.of(Operator.LESS_THAN, Operator.LESS_THAN_EQUAL).contains(operator) + ? Instant.now().plusSeconds(2) + : now.plusNanos(1000)) + .build(); + }) + .collect(Collectors.toCollection(ArrayList::new)); + + var start = Instant.now().truncatedTo(ChronoUnit.MILLIS); + traces.set(0, traces.getFirst().toBuilder() + .startTime(start) + .endTime(start.plus(end, ChronoUnit.MICROS)) + .build()); + + traces.forEach(expectedTrace -> create(expectedTrace, apiKey, workspaceName)); + + var expectedTraces = List.of(traces.getFirst()); + + var unexpectedTraces = PodamFactoryUtils.manufacturePojoList(factory, Trace.class).stream() + .map(span -> span.toBuilder() + .projectId(null) + .build()) + .toList(); + + unexpectedTraces.forEach(expectedTrace -> create(expectedTrace, apiKey, workspaceName)); + + var filters = List.of( + TraceFilter.builder() + .field(TraceField.DURATION) + .operator(operator) + .value(String.valueOf(duration)) + .build()); + + var expectedStats = getProjectTraceStatItems(expectedTraces); + + getStatsAndAssert(projectName, null, filters, apiKey, workspaceName, expectedStats); + } + private void getStatsAndAssert(String projectName, UUID projectId, List filters, String apiKey, String workspaceName, List> expectedStats) { WebTarget webTarget = client.target(URL_TEMPLATE.formatted(baseURI)) @@ -7632,7 +7707,8 @@ static Stream getTraceStats__whenFilterInvalidOperatorForFieldType__then .field(TraceField.DURATION) .operator(Operator.NOT_CONTAINS) .value("1") - .build()); + .build() + ); } @ParameterizedTest @@ -7733,7 +7809,8 @@ static Stream getTraceStats__whenFilterInvalidValueOrKeyForFieldType__th .field(TraceField.DURATION) .operator(Operator.EQUAL) .value(RandomStringUtils.randomAlphanumeric(5)) - .build()); + .build() + ); } @ParameterizedTest From ad8caa7d0c9f24037167ad1b64129cae2880d126 Mon Sep 17 00:00:00 2001 From: Thiago Hora Date: Fri, 13 Dec 2024 12:05:20 +0100 Subject: [PATCH 04/12] [OPIK-446] Add duration metric --- .../comet/opik/domain/ProjectMetricsDAO.java | 61 ++++++++ .../opik/domain/ProjectMetricsService.java | 30 ++-- .../000008_add_duration_columns.sql | 11 ++ .../v1/priv/ProjectMetricsResourceTest.java | 141 ++++++++++++++---- 4 files changed, 201 insertions(+), 42 deletions(-) create mode 100644 apps/opik-backend/src/main/resources/liquibase/db-app-analytics/migrations/000008_add_duration_columns.sql diff --git a/apps/opik-backend/src/main/java/com/comet/opik/domain/ProjectMetricsDAO.java b/apps/opik-backend/src/main/java/com/comet/opik/domain/ProjectMetricsDAO.java index 76a20f878e..5ccfd08c08 100644 --- a/apps/opik-backend/src/main/java/com/comet/opik/domain/ProjectMetricsDAO.java +++ b/apps/opik-backend/src/main/java/com/comet/opik/domain/ProjectMetricsDAO.java @@ -25,6 +25,7 @@ import java.util.Optional; import java.util.UUID; import java.util.function.Function; +import java.util.stream.Stream; import static com.comet.opik.domain.AsyncContextUtils.bindWorkspaceIdToMono; import static com.comet.opik.infrastructure.instrumentation.InstrumentAsyncUtils.endSegment; @@ -35,11 +36,16 @@ public interface ProjectMetricsDAO { String NAME_TRACES = "traces"; String NAME_COST = "cost"; + String NAME_DURATION_P50 = "duration.p50"; + String NAME_DURATION_P90 = "duration.p90"; + String NAME_DURATION_P99 = "duration.p99"; @Builder record Entry(String name, Instant time, Number value) { + } + Mono> getDuration(UUID projectId, ProjectMetricRequest request); Mono> getTraceCount(@NonNull UUID projectId, @NonNull ProjectMetricRequest request); Mono> getFeedbackScores(@NonNull UUID projectId, @NonNull ProjectMetricRequest request); Mono> getTokenUsage(@NonNull UUID projectId, @NonNull ProjectMetricRequest request); @@ -50,6 +56,7 @@ record Entry(String name, Instant time, Number value) { @Singleton @RequiredArgsConstructor(onConstructor_ = @Inject) class ProjectMetricsDAOImpl implements ProjectMetricsDAO { + private final @NonNull TransactionTemplateAsync template; private static final Map INTERVAL_TO_SQL = Map.of( @@ -57,6 +64,25 @@ class ProjectMetricsDAOImpl implements ProjectMetricsDAO { TimeInterval.DAILY, "toIntervalDay(1)", TimeInterval.HOURLY, "toIntervalHour(1)"); + private static final String GET_TRACE_DURATION = """ + SELECT AS bucket, + arrayMap(v -> + toDecimal64(if(isNaN(v), 0, v), 9), + quantiles(0.5, 0.9, 0.99)(duration) + ) AS duration + FROM traces + WHERE project_id = :project_id + AND workspace_id = :workspace_id + AND start_time >= parseDateTime64BestEffort(:start_time, 9) + AND start_time \\<= parseDateTime64BestEffort(:end_time, 9) + GROUP BY bucket + ORDER BY bucket + WITH FILL + FROM + TO parseDateTimeBestEffort(:end_time) + STEP ; + """; + private static final String GET_TRACE_COUNT = """ SELECT AS bucket, nullIf(count(DISTINCT id), 0) as count @@ -151,6 +177,41 @@ TO parseDateTimeBestEffort(:end_time) STEP ; """; + @Override + public Mono> getDuration(@NonNull UUID projectId, @NonNull ProjectMetricRequest request) { + return template.nonTransaction(connection -> getMetric(projectId, request, connection, + GET_TRACE_DURATION, "traceDuration") + .flatMapMany(result -> result.map((row, metadata) -> mapDuration(row))) + .reduce(Stream::concat) + .map(Stream::toList)); + } + + private Stream mapDuration(Row row) { + return Optional.ofNullable(row.get("duration", List.class)) + .map(durations -> Stream.of( + Entry.builder().name(NAME_DURATION_P50) + .time(row.get("bucket", Instant.class)) + .value(getP(durations, 0)) + .build(), + Entry.builder().name(NAME_DURATION_P90) + .time(row.get("bucket", Instant.class)) + .value(getP(durations, 1)) + .build(), + Entry.builder().name(NAME_DURATION_P99) + .time(row.get("bucket", Instant.class)) + .value(getP(durations, 2)) + .build())) + .orElse(Stream.empty()); + } + + private static BigDecimal getP(List durations, int index) { + if (durations.size() <= index) { + return null; + } + + return (BigDecimal) durations.get(index); + } + @Override public Mono> getTraceCount(@NonNull UUID projectId, @NonNull ProjectMetricRequest request) { return template.nonTransaction(connection -> getMetric(projectId, request, connection, diff --git a/apps/opik-backend/src/main/java/com/comet/opik/domain/ProjectMetricsService.java b/apps/opik-backend/src/main/java/com/comet/opik/domain/ProjectMetricsService.java index 49e3c1d729..1223dbc340 100644 --- a/apps/opik-backend/src/main/java/com/comet/opik/domain/ProjectMetricsService.java +++ b/apps/opik-backend/src/main/java/com/comet/opik/domain/ProjectMetricsService.java @@ -9,7 +9,6 @@ import jakarta.inject.Singleton; import jakarta.ws.rs.BadRequestException; import lombok.NonNull; -import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; import reactor.core.publisher.Mono; @@ -30,15 +29,23 @@ public interface ProjectMetricsService { @Slf4j @Singleton -@RequiredArgsConstructor(onConstructor_ = @Inject) class ProjectMetricsServiceImpl implements ProjectMetricsService { - private final @NonNull ProjectMetricsDAO projectMetricsDAO; + + private final @NonNull Map>>> metricHandler; + + @Inject + public ProjectMetricsServiceImpl(@NonNull ProjectMetricsDAO projectMetricsDAO) { + metricHandler = Map.of( + MetricType.TRACE_COUNT, projectMetricsDAO::getTraceCount, + MetricType.FEEDBACK_SCORES, projectMetricsDAO::getFeedbackScores, + MetricType.TOKEN_USAGE, projectMetricsDAO::getTokenUsage, + MetricType.COST, projectMetricsDAO::getCost, + MetricType.DURATION, projectMetricsDAO::getDuration); + } @Override public Mono> getProjectMetrics(UUID projectId, ProjectMetricRequest request) { return getMetricHandler(request.metricType()) - .orElseThrow( - () -> new BadRequestException(ERR_PROJECT_METRIC_NOT_SUPPORTED.formatted(request.metricType()))) .apply(projectId, request) .map(dataPoints -> ProjectMetricResponse.builder() .projectId(projectId) @@ -71,15 +78,8 @@ private List> entriesToResults(List>>> getMetricHandler( - MetricType metricType) { - Map>>> HANDLER_BY_TYPE = Map - .of( - MetricType.TRACE_COUNT, projectMetricsDAO::getTraceCount, - MetricType.FEEDBACK_SCORES, projectMetricsDAO::getFeedbackScores, - MetricType.TOKEN_USAGE, projectMetricsDAO::getTokenUsage, - MetricType.COST, projectMetricsDAO::getCost); - - return Optional.ofNullable(HANDLER_BY_TYPE.get(metricType)); + private BiFunction>> getMetricHandler(MetricType metricType) { + return Optional.ofNullable(metricHandler.get(metricType)) + .orElseThrow(() -> new BadRequestException(ERR_PROJECT_METRIC_NOT_SUPPORTED.formatted(metricType))); } } diff --git a/apps/opik-backend/src/main/resources/liquibase/db-app-analytics/migrations/000008_add_duration_columns.sql b/apps/opik-backend/src/main/resources/liquibase/db-app-analytics/migrations/000008_add_duration_columns.sql new file mode 100644 index 0000000000..a41046ab3f --- /dev/null +++ b/apps/opik-backend/src/main/resources/liquibase/db-app-analytics/migrations/000008_add_duration_columns.sql @@ -0,0 +1,11 @@ +--liquibase formatted sql +--changeset thiagohora:add_duration_columns + +ALTER TABLE ${ANALYTICS_DB_DATABASE_NAME}.spans + ADD COLUMN IF NOT EXISTS duration Nullable(Float64) MATERIALIZED if(end_time IS NOT NULL, (dateDiff('microsecond', start_time, end_time) / 1000.0), NULL); + +ALTER TABLE ${ANALYTICS_DB_DATABASE_NAME}.traces + ADD COLUMN IF NOT EXISTS duration Nullable(Float64) MATERIALIZED if(end_time IS NOT NULL, (dateDiff('microsecond', start_time, end_time) / 1000.0), NULL); + +--rollback ALTER TABLE ${ANALYTICS_DB_DATABASE_NAME}.spans DROP COLUMN IF EXISTS duration; +--rollback ALTER TABLE ${ANALYTICS_DB_DATABASE_NAME}.traces DROP COLUMN IF EXISTS duration; diff --git a/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/ProjectMetricsResourceTest.java b/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/ProjectMetricsResourceTest.java index f3836bda73..f20aaf6d6f 100644 --- a/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/ProjectMetricsResourceTest.java +++ b/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/ProjectMetricsResourceTest.java @@ -32,9 +32,9 @@ import jakarta.ws.rs.core.HttpHeaders; import jakarta.ws.rs.core.MediaType; import org.apache.commons.lang3.RandomStringUtils; +import org.apache.commons.lang3.reflect.TypeUtils; import org.apache.hc.core5.http.HttpStatus; import org.jdbi.v3.core.Jdbi; -import org.jetbrains.annotations.NotNull; import org.junit.jupiter.api.AfterAll; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.BeforeEach; @@ -55,7 +55,6 @@ import ru.vyarus.dropwizard.guice.test.jupiter.ext.TestDropwizardAppExtension; import uk.co.jemos.podam.api.PodamFactory; -import java.lang.reflect.ParameterizedType; import java.lang.reflect.Type; import java.math.BigDecimal; import java.math.RoundingMode; @@ -296,6 +295,7 @@ void getProjectMetrics__whenSessionTokenIsPresent__thenReturnProperResponse( @DisplayName("Number of traces") @TestInstance(TestInstance.Lifecycle.PER_CLASS) class NumberOfTracesTest { + @ParameterizedTest @EnumSource(TimeInterval.class) void happyPath(TimeInterval interval) { @@ -362,11 +362,8 @@ public static Stream invalidParameters() { arguments(named("start equal to end", validReq.toBuilder() .intervalStart(now) .intervalEnd(now) - .build()), ProjectMetricsService.ERR_START_BEFORE_END), - arguments(named("not supported metric", validReq.toBuilder() - .metricType(MetricType.DURATION) - .build()), ProjectMetricsService.ERR_PROJECT_METRIC_NOT_SUPPORTED.formatted( - MetricType.DURATION))); + .build()), ProjectMetricsService.ERR_START_BEFORE_END) + ); } @ParameterizedTest @@ -409,6 +406,7 @@ private void createTraces(String projectName, Instant marker, int count) { @DisplayName("Feedback scores") @TestInstance(TestInstance.Lifecycle.PER_CLASS) class FeedbackScoresTest { + @ParameterizedTest @EnumSource(TimeInterval.class) void happyPath(TimeInterval interval) { @@ -499,6 +497,7 @@ private static BigDecimal calcAverage(List scores) { @DisplayName("Token usage") @TestInstance(TestInstance.Lifecycle.PER_CLASS) class TokenUsageTest { + @ParameterizedTest @EnumSource(TimeInterval.class) void happyPath(TimeInterval interval) { @@ -609,6 +608,7 @@ private void getAndAssertEmpty(UUID projectId, TimeInterval interval, Instant ma @DisplayName("Cost") @TestInstance(TestInstance.Lifecycle.PER_CLASS) class CostTest { + @ParameterizedTest @EnumSource(TimeInterval.class) void happyPath(TimeInterval interval) { @@ -686,6 +686,111 @@ private BigDecimal createSpans( } } + @Nested + @DisplayName("Duration") + @TestInstance(TestInstance.Lifecycle.PER_CLASS) + class DurationTest { + + @ParameterizedTest + @EnumSource(TimeInterval.class) + void happyPath(TimeInterval interval) { + // setup + mockTargetWorkspace(); + + Instant marker = getIntervalStart(interval); + String projectName = RandomStringUtils.randomAlphabetic(10); + var projectId = projectResourceClient.createProject(projectName, API_KEY, WORKSPACE_NAME); + + List durationsMinus3 = createTraces(projectName, subtract(marker, TIME_BUCKET_3, interval)); + List durationsMinus1 = createTraces(projectName, subtract(marker, TIME_BUCKET_1, interval)); + List durationsCurrent = createTraces(projectName, marker); + + var durationMinus3 = Map.of( + ProjectMetricsDAO.NAME_DURATION_P50, durationsMinus3.get(0), + ProjectMetricsDAO.NAME_DURATION_P90, durationsMinus3.get(1), + ProjectMetricsDAO.NAME_DURATION_P99, durationsMinus3.getLast()); + var durationMinus1 = Map.of( + ProjectMetricsDAO.NAME_DURATION_P50, durationsMinus1.get(0), + ProjectMetricsDAO.NAME_DURATION_P90, durationsMinus1.get(1), + ProjectMetricsDAO.NAME_DURATION_P99, durationsMinus1.getLast()); + var durationCurrent = Map.of( + ProjectMetricsDAO.NAME_DURATION_P50, durationsCurrent.get(0), + ProjectMetricsDAO.NAME_DURATION_P90, durationsCurrent.get(1), + ProjectMetricsDAO.NAME_DURATION_P99, durationsCurrent.getLast()); + + getMetricsAndAssert( + projectId, + ProjectMetricRequest.builder() + .metricType(MetricType.DURATION) + .interval(interval) + .intervalStart(subtract(marker, TIME_BUCKET_4, interval)) + .intervalEnd(Instant.now()) + .build(), + marker, + List.of(ProjectMetricsDAO.NAME_DURATION_P50, ProjectMetricsDAO.NAME_DURATION_P90, ProjectMetricsDAO.NAME_DURATION_P99), + BigDecimal.class, + durationMinus3, + durationMinus1, + durationCurrent); + } + + @ParameterizedTest + @EnumSource(TimeInterval.class) + void emptyData(TimeInterval interval) { + // setup + mockTargetWorkspace(); + + Instant marker = getIntervalStart(interval); + String projectName = RandomStringUtils.randomAlphabetic(10); + var projectId = projectResourceClient.createProject(projectName, API_KEY, WORKSPACE_NAME); + + Map empty = new HashMap<>() { + { + put(ProjectMetricsDAO.NAME_DURATION_P50, null); + put(ProjectMetricsDAO.NAME_DURATION_P90, null); + put(ProjectMetricsDAO.NAME_DURATION_P99, null); + } + }; + + getMetricsAndAssert( + projectId, + ProjectMetricRequest.builder() + .metricType(MetricType.DURATION) + .interval(interval) + .intervalStart(subtract(marker, TIME_BUCKET_4, interval)) + .intervalEnd(Instant.now()) + .build(), + marker, + List.of(ProjectMetricsDAO.NAME_DURATION_P50, ProjectMetricsDAO.NAME_DURATION_P90, ProjectMetricsDAO.NAME_DURATION_P99), + BigDecimal.class, + empty, + empty, + empty + ); + } + + private List createTraces(String projectName, Instant marker) { + List traces = IntStream.range(0, 5) + .mapToObj(i -> factory.manufacturePojo(Trace.class).toBuilder() + .projectName(projectName) + .startTime(marker) + .endTime(marker.plusMillis(RANDOM.nextInt(1000))) + .build()) + .toList(); + + traceResourceClient.batchCreateTraces(traces, API_KEY, WORKSPACE_NAME); + + return StatsUtils.calculateQuantiles( + traces.stream() + .filter(entity -> entity.endTime() != null) + .map(entity -> entity.startTime().until(entity.endTime(), ChronoUnit.MICROS)) + .map(duration -> duration / 1_000.0) + .toList(), + List.of(0.50, 0.90, 0.99)); + } + + } + private ProjectMetricResponse getProjectMetrics( UUID projectId, ProjectMetricRequest request, Class aClass) { try (var response = client.target(URL_TEMPLATE.formatted(baseURI, projectId)) @@ -697,7 +802,8 @@ private ProjectMetricResponse getProjectMetrics( assertThat(response.getStatus()).isEqualTo(HttpStatus.SC_OK); assertThat(response.hasEntity()).isTrue(); - return response.readEntity(new GenericType<>(createParameterizedProjectMetricResponse(aClass))); + Type parameterize = TypeUtils.parameterize(ProjectMetricResponse.class, aClass); + return response.readEntity(new GenericType<>(parameterize)); } } @@ -722,25 +828,6 @@ private void getMetricsAndAssert( .isEqualTo(expected); } - private static Type createParameterizedProjectMetricResponse(Class genericArgument) { - return new ParameterizedType() { - @NotNull @Override - public Type[] getActualTypeArguments() { - return new Type[]{genericArgument}; - } - - @NotNull @Override - public Type getRawType() { - return ProjectMetricResponse.class; - } - - @Override - public Type getOwnerType() { - return null; - } - }; - } - private static List> createExpected( Instant marker, TimeInterval interval, List names, Map dataMinus3, Map dataMinus1, Map dataNow) { From be1001d762db0ace9ebd34090fd976f2df15ab5b Mon Sep 17 00:00:00 2001 From: Thiago Hora Date: Fri, 13 Dec 2024 12:06:51 +0100 Subject: [PATCH 05/12] Code format --- .../comet/opik/domain/ProjectMetricsDAO.java | 2 +- .../opik/domain/ProjectMetricsService.java | 13 +-- .../v1/priv/ProjectMetricsResourceTest.java | 32 +++---- .../resources/v1/priv/SpansResourceTest.java | 6 +- .../resources/v1/priv/TracesResourceTest.java | 85 +------------------ 5 files changed, 29 insertions(+), 109 deletions(-) diff --git a/apps/opik-backend/src/main/java/com/comet/opik/domain/ProjectMetricsDAO.java b/apps/opik-backend/src/main/java/com/comet/opik/domain/ProjectMetricsDAO.java index 5ccfd08c08..b384206d2a 100644 --- a/apps/opik-backend/src/main/java/com/comet/opik/domain/ProjectMetricsDAO.java +++ b/apps/opik-backend/src/main/java/com/comet/opik/domain/ProjectMetricsDAO.java @@ -186,7 +186,7 @@ public Mono> getDuration(@NonNull UUID projectId, @NonNull ProjectMe .map(Stream::toList)); } - private Stream mapDuration(Row row) { + private Stream mapDuration(Row row) { return Optional.ofNullable(row.get("duration", List.class)) .map(durations -> Stream.of( Entry.builder().name(NAME_DURATION_P50) diff --git a/apps/opik-backend/src/main/java/com/comet/opik/domain/ProjectMetricsService.java b/apps/opik-backend/src/main/java/com/comet/opik/domain/ProjectMetricsService.java index 1223dbc340..3e5c2de1a2 100644 --- a/apps/opik-backend/src/main/java/com/comet/opik/domain/ProjectMetricsService.java +++ b/apps/opik-backend/src/main/java/com/comet/opik/domain/ProjectMetricsService.java @@ -36,11 +36,11 @@ class ProjectMetricsServiceImpl implements ProjectMetricsService { @Inject public ProjectMetricsServiceImpl(@NonNull ProjectMetricsDAO projectMetricsDAO) { metricHandler = Map.of( - MetricType.TRACE_COUNT, projectMetricsDAO::getTraceCount, - MetricType.FEEDBACK_SCORES, projectMetricsDAO::getFeedbackScores, - MetricType.TOKEN_USAGE, projectMetricsDAO::getTokenUsage, - MetricType.COST, projectMetricsDAO::getCost, - MetricType.DURATION, projectMetricsDAO::getDuration); + MetricType.TRACE_COUNT, projectMetricsDAO::getTraceCount, + MetricType.FEEDBACK_SCORES, projectMetricsDAO::getFeedbackScores, + MetricType.TOKEN_USAGE, projectMetricsDAO::getTokenUsage, + MetricType.COST, projectMetricsDAO::getCost, + MetricType.DURATION, projectMetricsDAO::getDuration); } @Override @@ -78,7 +78,8 @@ private List> entriesToResults(List>> getMetricHandler(MetricType metricType) { + private BiFunction>> getMetricHandler( + MetricType metricType) { return Optional.ofNullable(metricHandler.get(metricType)) .orElseThrow(() -> new BadRequestException(ERR_PROJECT_METRIC_NOT_SUPPORTED.formatted(metricType))); } diff --git a/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/ProjectMetricsResourceTest.java b/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/ProjectMetricsResourceTest.java index f20aaf6d6f..d172456036 100644 --- a/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/ProjectMetricsResourceTest.java +++ b/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/ProjectMetricsResourceTest.java @@ -362,8 +362,7 @@ public static Stream invalidParameters() { arguments(named("start equal to end", validReq.toBuilder() .intervalStart(now) .intervalEnd(now) - .build()), ProjectMetricsService.ERR_START_BEFORE_END) - ); + .build()), ProjectMetricsService.ERR_START_BEFORE_END)); } @ParameterizedTest @@ -721,13 +720,14 @@ void happyPath(TimeInterval interval) { getMetricsAndAssert( projectId, ProjectMetricRequest.builder() - .metricType(MetricType.DURATION) - .interval(interval) - .intervalStart(subtract(marker, TIME_BUCKET_4, interval)) - .intervalEnd(Instant.now()) - .build(), + .metricType(MetricType.DURATION) + .interval(interval) + .intervalStart(subtract(marker, TIME_BUCKET_4, interval)) + .intervalEnd(Instant.now()) + .build(), marker, - List.of(ProjectMetricsDAO.NAME_DURATION_P50, ProjectMetricsDAO.NAME_DURATION_P90, ProjectMetricsDAO.NAME_DURATION_P99), + List.of(ProjectMetricsDAO.NAME_DURATION_P50, ProjectMetricsDAO.NAME_DURATION_P90, + ProjectMetricsDAO.NAME_DURATION_P99), BigDecimal.class, durationMinus3, durationMinus1, @@ -755,18 +755,18 @@ void emptyData(TimeInterval interval) { getMetricsAndAssert( projectId, ProjectMetricRequest.builder() - .metricType(MetricType.DURATION) - .interval(interval) - .intervalStart(subtract(marker, TIME_BUCKET_4, interval)) - .intervalEnd(Instant.now()) - .build(), + .metricType(MetricType.DURATION) + .interval(interval) + .intervalStart(subtract(marker, TIME_BUCKET_4, interval)) + .intervalEnd(Instant.now()) + .build(), marker, - List.of(ProjectMetricsDAO.NAME_DURATION_P50, ProjectMetricsDAO.NAME_DURATION_P90, ProjectMetricsDAO.NAME_DURATION_P99), + List.of(ProjectMetricsDAO.NAME_DURATION_P50, ProjectMetricsDAO.NAME_DURATION_P90, + ProjectMetricsDAO.NAME_DURATION_P99), BigDecimal.class, empty, empty, - empty - ); + empty); } private List createTraces(String projectName, Instant marker) { diff --git a/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/SpansResourceTest.java b/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/SpansResourceTest.java index 3ecefeb7a5..6cb42d1055 100644 --- a/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/SpansResourceTest.java +++ b/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/SpansResourceTest.java @@ -7605,8 +7605,7 @@ static Stream getSpanStats__whenFilterInvalidOperatorForFieldType__thenR .field(SpanField.DURATION) .operator(Operator.STARTS_WITH) .value("1") - .build() - ); + .build()); } @ParameterizedTest @@ -7720,8 +7719,7 @@ static Stream getSpanStats__whenFilterInvalidValueOrKeyForFieldType__the .field(SpanField.DURATION) .operator(Operator.EQUAL) .value(RandomStringUtils.randomAlphanumeric(5)) - .build() - ); + .build()); } @ParameterizedTest diff --git a/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/TracesResourceTest.java b/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/TracesResourceTest.java index 5b51ebe6ac..db1b6d743f 100644 --- a/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/TracesResourceTest.java +++ b/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/TracesResourceTest.java @@ -3080,8 +3080,7 @@ static Stream getTracesByProject__whenFilterInvalidOperatorForFieldType_ .field(TraceField.DURATION) .operator(Operator.NOT_CONTAINS) .value("1") - .build() - ); + .build()); } @ParameterizedTest @@ -6739,24 +6738,6 @@ void getTraceStats__whenFilterMetadataLessThanNull__thenReturnTracesFiltered() { getStatsAndAssert(projectName, null, filters, apiKey, workspaceName, expectedStats); } - Stream getTraceStats__whenFilterByDuration__thenReturnTracesFiltered() { - return Stream.of( - arguments(Operator.EQUAL, - Duration.ofMillis(1L).toNanos() / 1000, 1.0), - arguments(Operator.GREATER_THAN, - Duration.ofMillis(8L).toNanos() / 1000, 7.0), - arguments(Operator.GREATER_THAN_EQUAL, - Duration.ofMillis(1L).toNanos() / 1000, 1.0), - arguments(Operator.GREATER_THAN_EQUAL, - Duration.ofMillis(1L).plusNanos(1000).toNanos() / 1000, 1.0), - arguments(Operator.LESS_THAN, - Duration.ofMillis(1L).plusNanos(1).toNanos() / 1000, 2.0), - arguments(Operator.LESS_THAN_EQUAL, - Duration.ofMillis(1L).toNanos() / 1000, 1.0), - arguments(Operator.LESS_THAN_EQUAL, - Duration.ofMillis(1L).toNanos() / 1000, 2.0)); - } - @ParameterizedTest @MethodSource void getTraceStats__whenFilterByDuration__thenReturnTracesFiltered(Operator operator, long end, @@ -6834,64 +6815,6 @@ Stream getTraceStats__whenFilterByDuration__thenReturnTracesFiltered( Duration.ofMillis(1L).toNanos() / 1000, 2.0)); } - @ParameterizedTest - @MethodSource - void getTraceStats__whenFilterByDuration__thenReturnTracesFiltered(Operator operator, long end, double duration) { - String workspaceName = UUID.randomUUID().toString(); - String workspaceId = UUID.randomUUID().toString(); - String apiKey = UUID.randomUUID().toString(); - - mockTargetWorkspace(apiKey, workspaceName, workspaceId); - - var projectName = generator.generate().toString(); - var traces = PodamFactoryUtils.manufacturePojoList(factory, Trace.class) - .stream() - .map(trace -> { - Instant now = Instant.now(); - return trace.toBuilder() - .projectId(null) - .usage(null) - .projectName(projectName) - .feedbackScores(null) - .totalEstimatedCost(BigDecimal.ZERO) - .startTime(now) - .endTime(Set.of(Operator.LESS_THAN, Operator.LESS_THAN_EQUAL).contains(operator) - ? Instant.now().plusSeconds(2) - : now.plusNanos(1000)) - .build(); - }) - .collect(Collectors.toCollection(ArrayList::new)); - - var start = Instant.now().truncatedTo(ChronoUnit.MILLIS); - traces.set(0, traces.getFirst().toBuilder() - .startTime(start) - .endTime(start.plus(end, ChronoUnit.MICROS)) - .build()); - - traces.forEach(expectedTrace -> create(expectedTrace, apiKey, workspaceName)); - - var expectedTraces = List.of(traces.getFirst()); - - var unexpectedTraces = PodamFactoryUtils.manufacturePojoList(factory, Trace.class).stream() - .map(span -> span.toBuilder() - .projectId(null) - .build()) - .toList(); - - unexpectedTraces.forEach(expectedTrace -> create(expectedTrace, apiKey, workspaceName)); - - var filters = List.of( - TraceFilter.builder() - .field(TraceField.DURATION) - .operator(operator) - .value(String.valueOf(duration)) - .build()); - - var expectedStats = getProjectTraceStatItems(expectedTraces); - - getStatsAndAssert(projectName, null, filters, apiKey, workspaceName, expectedStats); - } - private void getStatsAndAssert(String projectName, UUID projectId, List filters, String apiKey, String workspaceName, List> expectedStats) { WebTarget webTarget = client.target(URL_TEMPLATE.formatted(baseURI)) @@ -7707,8 +7630,7 @@ static Stream getTraceStats__whenFilterInvalidOperatorForFieldType__then .field(TraceField.DURATION) .operator(Operator.NOT_CONTAINS) .value("1") - .build() - ); + .build()); } @ParameterizedTest @@ -7809,8 +7731,7 @@ static Stream getTraceStats__whenFilterInvalidValueOrKeyForFieldType__th .field(TraceField.DURATION) .operator(Operator.EQUAL) .value(RandomStringUtils.randomAlphanumeric(5)) - .build() - ); + .build()); } @ParameterizedTest From 13456d022d91675d56466276c1bd221cb98de429 Mon Sep 17 00:00:00 2001 From: Thiago Hora Date: Fri, 13 Dec 2024 13:04:36 +0100 Subject: [PATCH 06/12] Fix tests --- .../src/main/java/com/comet/opik/api/Span.java | 9 --------- .../src/main/java/com/comet/opik/api/Trace.java | 8 -------- .../opik/api/resources/v1/priv/SpansResourceTest.java | 3 +++ .../opik/api/resources/v1/priv/TracesResourceTest.java | 4 ++++ .../NumericalFeedbackDetailTypeManufacturer.java | 2 +- 5 files changed, 8 insertions(+), 18 deletions(-) diff --git a/apps/opik-backend/src/main/java/com/comet/opik/api/Span.java b/apps/opik-backend/src/main/java/com/comet/opik/api/Span.java index d75842f042..2ebc3b1cd9 100644 --- a/apps/opik-backend/src/main/java/com/comet/opik/api/Span.java +++ b/apps/opik-backend/src/main/java/com/comet/opik/api/Span.java @@ -1,9 +1,7 @@ package com.comet.opik.api; import com.comet.opik.domain.SpanType; -import com.comet.opik.utils.DurationUtils; import com.fasterxml.jackson.annotation.JsonIgnoreProperties; -import com.fasterxml.jackson.annotation.JsonProperty; import com.fasterxml.jackson.annotation.JsonView; import com.fasterxml.jackson.databind.JsonNode; import com.fasterxml.jackson.databind.PropertyNamingStrategies; @@ -75,11 +73,4 @@ public static class Write { public static class Public { } } - - @JsonProperty - @JsonView({Span.View.Public.class}) - @Schema(accessMode = Schema.AccessMode.READ_ONLY, description = "Duration in milliseconds as a decimal number to support sub-millisecond precision") - public Double duration() { - return DurationUtils.getDurationInMillisWithSubMilliPrecision(startTime, endTime); - } } diff --git a/apps/opik-backend/src/main/java/com/comet/opik/api/Trace.java b/apps/opik-backend/src/main/java/com/comet/opik/api/Trace.java index c42a403644..d149229cb6 100644 --- a/apps/opik-backend/src/main/java/com/comet/opik/api/Trace.java +++ b/apps/opik-backend/src/main/java/com/comet/opik/api/Trace.java @@ -1,8 +1,6 @@ package com.comet.opik.api; -import com.comet.opik.utils.DurationUtils; import com.fasterxml.jackson.annotation.JsonIgnoreProperties; -import com.fasterxml.jackson.annotation.JsonProperty; import com.fasterxml.jackson.annotation.JsonView; import com.fasterxml.jackson.databind.JsonNode; import com.fasterxml.jackson.databind.PropertyNamingStrategies; @@ -71,10 +69,4 @@ public static class Public { } } - @JsonProperty - @JsonView({Span.View.Public.class}) - @Schema(accessMode = Schema.AccessMode.READ_ONLY, description = "Duration in milliseconds as a decimal number to support sub-millisecond precision") - public Double duration() { - return DurationUtils.getDurationInMillisWithSubMilliPrecision(startTime, endTime); - } } diff --git a/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/SpansResourceTest.java b/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/SpansResourceTest.java index 6cb42d1055..1ffa2ff3a6 100644 --- a/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/SpansResourceTest.java +++ b/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/SpansResourceTest.java @@ -38,6 +38,7 @@ import com.comet.opik.domain.cost.ModelPrice; import com.comet.opik.infrastructure.auth.RequestContext; import com.comet.opik.podam.PodamFactoryUtils; +import com.comet.opik.utils.DurationUtils; import com.comet.opik.utils.JsonUtils; import com.fasterxml.jackson.databind.JsonNode; import com.fasterxml.jackson.databind.node.NullNode; @@ -901,6 +902,8 @@ void findWithImageTruncation(JsonNode original, JsonNode expected, boolean trunc .input(expected) .output(expected) .metadata(expected) + .duration(DurationUtils.getDurationInMillisWithSubMilliPrecision(span.startTime(), + span.endTime())) .build()) .toList(); diff --git a/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/TracesResourceTest.java b/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/TracesResourceTest.java index db1b6d743f..162dd2f787 100644 --- a/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/TracesResourceTest.java +++ b/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/TracesResourceTest.java @@ -39,6 +39,7 @@ import com.comet.opik.domain.cost.ModelPrice; import com.comet.opik.infrastructure.auth.RequestContext; import com.comet.opik.podam.PodamFactoryUtils; +import com.comet.opik.utils.DurationUtils; import com.comet.opik.utils.JsonUtils; import com.fasterxml.jackson.databind.JsonNode; import com.fasterxml.uuid.Generators; @@ -53,6 +54,7 @@ import jakarta.ws.rs.core.Response; import org.apache.commons.lang3.RandomStringUtils; import org.apache.http.HttpStatus; +import org.assertj.core.api.SoftAssertions; import org.assertj.core.api.recursive.comparison.RecursiveComparisonConfiguration; import org.jdbi.v3.core.Jdbi; import org.junit.jupiter.api.AfterAll; @@ -884,6 +886,8 @@ void findWithImageTruncation(JsonNode original, JsonNode expected, boolean trunc .input(expected) .output(expected) .metadata(expected) + .duration(DurationUtils.getDurationInMillisWithSubMilliPrecision(trace.startTime(), + trace.endTime())) .build()) .toList(); diff --git a/apps/opik-backend/src/test/java/com/comet/opik/podam/manufacturer/NumericalFeedbackDetailTypeManufacturer.java b/apps/opik-backend/src/test/java/com/comet/opik/podam/manufacturer/NumericalFeedbackDetailTypeManufacturer.java index ef4ff789b3..6789fcf1db 100644 --- a/apps/opik-backend/src/test/java/com/comet/opik/podam/manufacturer/NumericalFeedbackDetailTypeManufacturer.java +++ b/apps/opik-backend/src/test/java/com/comet/opik/podam/manufacturer/NumericalFeedbackDetailTypeManufacturer.java @@ -10,7 +10,7 @@ import java.math.RoundingMode; import static com.comet.opik.api.FeedbackDefinition.NumericalFeedbackDefinition.NumericalFeedbackDetail; -import static com.comet.opik.utils.ValidationUtils.*; +import static com.comet.opik.utils.ValidationUtils.SCALE; public class NumericalFeedbackDetailTypeManufacturer extends AbstractTypeManufacturer { From 2f4c02622e6cebc2e2f64978289a5da59b072f63 Mon Sep 17 00:00:00 2001 From: Thiago Hora Date: Fri, 13 Dec 2024 17:22:44 +0100 Subject: [PATCH 07/12] Fix condition --- .../migrations/000008_add_duration_columns.sql | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/apps/opik-backend/src/main/resources/liquibase/db-app-analytics/migrations/000008_add_duration_columns.sql b/apps/opik-backend/src/main/resources/liquibase/db-app-analytics/migrations/000008_add_duration_columns.sql index a41046ab3f..73ce89b2ff 100644 --- a/apps/opik-backend/src/main/resources/liquibase/db-app-analytics/migrations/000008_add_duration_columns.sql +++ b/apps/opik-backend/src/main/resources/liquibase/db-app-analytics/migrations/000008_add_duration_columns.sql @@ -2,10 +2,10 @@ --changeset thiagohora:add_duration_columns ALTER TABLE ${ANALYTICS_DB_DATABASE_NAME}.spans - ADD COLUMN IF NOT EXISTS duration Nullable(Float64) MATERIALIZED if(end_time IS NOT NULL, (dateDiff('microsecond', start_time, end_time) / 1000.0), NULL); + ADD COLUMN IF NOT EXISTS duration Nullable(Float64) MATERIALIZED if(end_time IS NOT NULL AND start_time IS NOT NULL AND notEquals(start_time, toDateTime64('1970-01-01 00:00:00.000', 9)), (dateDiff('microsecond', start_time, end_time) / 1000.0), NULL); ALTER TABLE ${ANALYTICS_DB_DATABASE_NAME}.traces - ADD COLUMN IF NOT EXISTS duration Nullable(Float64) MATERIALIZED if(end_time IS NOT NULL, (dateDiff('microsecond', start_time, end_time) / 1000.0), NULL); + ADD COLUMN IF NOT EXISTS duration Nullable(Float64) MATERIALIZED if(end_time IS NOT NULL AND start_time IS NOT NULL AND notEquals(start_time, toDateTime64('1970-01-01 00:00:00.000', 9)), (dateDiff('microsecond', start_time, end_time) / 1000.0), NULL); --rollback ALTER TABLE ${ANALYTICS_DB_DATABASE_NAME}.spans DROP COLUMN IF EXISTS duration; --rollback ALTER TABLE ${ANALYTICS_DB_DATABASE_NAME}.traces DROP COLUMN IF EXISTS duration; From ebbfafeb019ffc12492b9942a5885d488171fb7e Mon Sep 17 00:00:00 2001 From: Ido Berkovich Date: Wed, 18 Dec 2024 11:34:49 +0200 Subject: [PATCH 08/12] OPIK-446 resolve logical conflicts after rebase --- .../comet/opik/domain/ProjectMetricsDAO.java | 3 +-- .../com/comet/opik/utils/DurationUtils.java | 23 ------------------- .../000008_add_duration_columns.sql | 11 --------- .../resources/v1/priv/SpansResourceTest.java | 1 - .../resources/v1/priv/TracesResourceTest.java | 2 -- 5 files changed, 1 insertion(+), 39 deletions(-) delete mode 100644 apps/opik-backend/src/main/java/com/comet/opik/utils/DurationUtils.java delete mode 100644 apps/opik-backend/src/main/resources/liquibase/db-app-analytics/migrations/000008_add_duration_columns.sql diff --git a/apps/opik-backend/src/main/java/com/comet/opik/domain/ProjectMetricsDAO.java b/apps/opik-backend/src/main/java/com/comet/opik/domain/ProjectMetricsDAO.java index b384206d2a..06580820f4 100644 --- a/apps/opik-backend/src/main/java/com/comet/opik/domain/ProjectMetricsDAO.java +++ b/apps/opik-backend/src/main/java/com/comet/opik/domain/ProjectMetricsDAO.java @@ -56,7 +56,6 @@ record Entry(String name, Instant time, Number value) { @Singleton @RequiredArgsConstructor(onConstructor_ = @Inject) class ProjectMetricsDAOImpl implements ProjectMetricsDAO { - private final @NonNull TransactionTemplateAsync template; private static final Map INTERVAL_TO_SQL = Map.of( @@ -68,7 +67,7 @@ class ProjectMetricsDAOImpl implements ProjectMetricsDAO { SELECT AS bucket, arrayMap(v -> toDecimal64(if(isNaN(v), 0, v), 9), - quantiles(0.5, 0.9, 0.99)(duration) + quantiles(0.5, 0.9, 0.99)(duration_millis) ) AS duration FROM traces WHERE project_id = :project_id diff --git a/apps/opik-backend/src/main/java/com/comet/opik/utils/DurationUtils.java b/apps/opik-backend/src/main/java/com/comet/opik/utils/DurationUtils.java deleted file mode 100644 index 74ddd85010..0000000000 --- a/apps/opik-backend/src/main/java/com/comet/opik/utils/DurationUtils.java +++ /dev/null @@ -1,23 +0,0 @@ -package com.comet.opik.utils; - -import lombok.NonNull; -import lombok.experimental.UtilityClass; - -import java.time.Duration; -import java.time.Instant; - -@UtilityClass -public class DurationUtils { - - public static final Double TIME_UNIT = 1_000.0; - - public static Double getDurationInMillisWithSubMilliPrecision(@NonNull Instant startTime, Instant endTime) { - if (endTime == null) { - return null; - } - - long micros = Duration.between(startTime, endTime).toNanos() / TIME_UNIT.longValue(); - return micros / TIME_UNIT; - } - -} diff --git a/apps/opik-backend/src/main/resources/liquibase/db-app-analytics/migrations/000008_add_duration_columns.sql b/apps/opik-backend/src/main/resources/liquibase/db-app-analytics/migrations/000008_add_duration_columns.sql deleted file mode 100644 index 73ce89b2ff..0000000000 --- a/apps/opik-backend/src/main/resources/liquibase/db-app-analytics/migrations/000008_add_duration_columns.sql +++ /dev/null @@ -1,11 +0,0 @@ ---liquibase formatted sql ---changeset thiagohora:add_duration_columns - -ALTER TABLE ${ANALYTICS_DB_DATABASE_NAME}.spans - ADD COLUMN IF NOT EXISTS duration Nullable(Float64) MATERIALIZED if(end_time IS NOT NULL AND start_time IS NOT NULL AND notEquals(start_time, toDateTime64('1970-01-01 00:00:00.000', 9)), (dateDiff('microsecond', start_time, end_time) / 1000.0), NULL); - -ALTER TABLE ${ANALYTICS_DB_DATABASE_NAME}.traces - ADD COLUMN IF NOT EXISTS duration Nullable(Float64) MATERIALIZED if(end_time IS NOT NULL AND start_time IS NOT NULL AND notEquals(start_time, toDateTime64('1970-01-01 00:00:00.000', 9)), (dateDiff('microsecond', start_time, end_time) / 1000.0), NULL); - ---rollback ALTER TABLE ${ANALYTICS_DB_DATABASE_NAME}.spans DROP COLUMN IF EXISTS duration; ---rollback ALTER TABLE ${ANALYTICS_DB_DATABASE_NAME}.traces DROP COLUMN IF EXISTS duration; diff --git a/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/SpansResourceTest.java b/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/SpansResourceTest.java index 1ffa2ff3a6..2b5c224280 100644 --- a/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/SpansResourceTest.java +++ b/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/SpansResourceTest.java @@ -38,7 +38,6 @@ import com.comet.opik.domain.cost.ModelPrice; import com.comet.opik.infrastructure.auth.RequestContext; import com.comet.opik.podam.PodamFactoryUtils; -import com.comet.opik.utils.DurationUtils; import com.comet.opik.utils.JsonUtils; import com.fasterxml.jackson.databind.JsonNode; import com.fasterxml.jackson.databind.node.NullNode; diff --git a/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/TracesResourceTest.java b/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/TracesResourceTest.java index 162dd2f787..ae28029721 100644 --- a/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/TracesResourceTest.java +++ b/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/TracesResourceTest.java @@ -39,7 +39,6 @@ import com.comet.opik.domain.cost.ModelPrice; import com.comet.opik.infrastructure.auth.RequestContext; import com.comet.opik.podam.PodamFactoryUtils; -import com.comet.opik.utils.DurationUtils; import com.comet.opik.utils.JsonUtils; import com.fasterxml.jackson.databind.JsonNode; import com.fasterxml.uuid.Generators; @@ -54,7 +53,6 @@ import jakarta.ws.rs.core.Response; import org.apache.commons.lang3.RandomStringUtils; import org.apache.http.HttpStatus; -import org.assertj.core.api.SoftAssertions; import org.assertj.core.api.recursive.comparison.RecursiveComparisonConfiguration; import org.jdbi.v3.core.Jdbi; import org.junit.jupiter.api.AfterAll; From 19374bba49ca17bba60fe831b7bc3555a9b39097 Mon Sep 17 00:00:00 2001 From: Ido Berkovich Date: Wed, 18 Dec 2024 11:40:10 +0200 Subject: [PATCH 09/12] OPIK-446 resolve logical conflicts after rebase --- .../main/java/com/comet/opik/api/Trace.java | 1 - .../opik/domain/ProjectMetricsService.java | 1 - .../v1/priv/ProjectMetricsResourceTest.java | 6 ---- .../resources/v1/priv/TracesResourceTest.java | 36 +++++++++---------- 4 files changed, 18 insertions(+), 26 deletions(-) diff --git a/apps/opik-backend/src/main/java/com/comet/opik/api/Trace.java b/apps/opik-backend/src/main/java/com/comet/opik/api/Trace.java index d149229cb6..b5f04a696a 100644 --- a/apps/opik-backend/src/main/java/com/comet/opik/api/Trace.java +++ b/apps/opik-backend/src/main/java/com/comet/opik/api/Trace.java @@ -68,5 +68,4 @@ public static class Write { public static class Public { } } - } diff --git a/apps/opik-backend/src/main/java/com/comet/opik/domain/ProjectMetricsService.java b/apps/opik-backend/src/main/java/com/comet/opik/domain/ProjectMetricsService.java index 3e5c2de1a2..c9ffffe327 100644 --- a/apps/opik-backend/src/main/java/com/comet/opik/domain/ProjectMetricsService.java +++ b/apps/opik-backend/src/main/java/com/comet/opik/domain/ProjectMetricsService.java @@ -30,7 +30,6 @@ public interface ProjectMetricsService { @Slf4j @Singleton class ProjectMetricsServiceImpl implements ProjectMetricsService { - private final @NonNull Map>>> metricHandler; @Inject diff --git a/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/ProjectMetricsResourceTest.java b/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/ProjectMetricsResourceTest.java index d172456036..a8dfdf8183 100644 --- a/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/ProjectMetricsResourceTest.java +++ b/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/ProjectMetricsResourceTest.java @@ -295,7 +295,6 @@ void getProjectMetrics__whenSessionTokenIsPresent__thenReturnProperResponse( @DisplayName("Number of traces") @TestInstance(TestInstance.Lifecycle.PER_CLASS) class NumberOfTracesTest { - @ParameterizedTest @EnumSource(TimeInterval.class) void happyPath(TimeInterval interval) { @@ -405,7 +404,6 @@ private void createTraces(String projectName, Instant marker, int count) { @DisplayName("Feedback scores") @TestInstance(TestInstance.Lifecycle.PER_CLASS) class FeedbackScoresTest { - @ParameterizedTest @EnumSource(TimeInterval.class) void happyPath(TimeInterval interval) { @@ -496,7 +494,6 @@ private static BigDecimal calcAverage(List scores) { @DisplayName("Token usage") @TestInstance(TestInstance.Lifecycle.PER_CLASS) class TokenUsageTest { - @ParameterizedTest @EnumSource(TimeInterval.class) void happyPath(TimeInterval interval) { @@ -607,7 +604,6 @@ private void getAndAssertEmpty(UUID projectId, TimeInterval interval, Instant ma @DisplayName("Cost") @TestInstance(TestInstance.Lifecycle.PER_CLASS) class CostTest { - @ParameterizedTest @EnumSource(TimeInterval.class) void happyPath(TimeInterval interval) { @@ -689,7 +685,6 @@ private BigDecimal createSpans( @DisplayName("Duration") @TestInstance(TestInstance.Lifecycle.PER_CLASS) class DurationTest { - @ParameterizedTest @EnumSource(TimeInterval.class) void happyPath(TimeInterval interval) { @@ -788,7 +783,6 @@ private List createTraces(String projectName, Instant marker) { .toList(), List.of(0.50, 0.90, 0.99)); } - } private ProjectMetricResponse getProjectMetrics( diff --git a/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/TracesResourceTest.java b/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/TracesResourceTest.java index ae28029721..bf563902d0 100644 --- a/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/TracesResourceTest.java +++ b/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/TracesResourceTest.java @@ -6740,6 +6740,24 @@ void getTraceStats__whenFilterMetadataLessThanNull__thenReturnTracesFiltered() { getStatsAndAssert(projectName, null, filters, apiKey, workspaceName, expectedStats); } + Stream getTraceStats__whenFilterByDuration__thenReturnTracesFiltered() { + return Stream.of( + arguments(Operator.EQUAL, + Duration.ofMillis(1L).toNanos() / 1000, 1.0), + arguments(Operator.GREATER_THAN, + Duration.ofMillis(8L).toNanos() / 1000, 7.0), + arguments(Operator.GREATER_THAN_EQUAL, + Duration.ofMillis(1L).toNanos() / 1000, 1.0), + arguments(Operator.GREATER_THAN_EQUAL, + Duration.ofMillis(1L).plusNanos(1000).toNanos() / 1000, 1.0), + arguments(Operator.LESS_THAN, + Duration.ofMillis(1L).plusNanos(1).toNanos() / 1000, 2.0), + arguments(Operator.LESS_THAN_EQUAL, + Duration.ofMillis(1L).toNanos() / 1000, 1.0), + arguments(Operator.LESS_THAN_EQUAL, + Duration.ofMillis(1L).toNanos() / 1000, 2.0)); + } + @ParameterizedTest @MethodSource void getTraceStats__whenFilterByDuration__thenReturnTracesFiltered(Operator operator, long end, @@ -6799,24 +6817,6 @@ void getTraceStats__whenFilterByDuration__thenReturnTracesFiltered(Operator oper getStatsAndAssert(projectName, null, filters, apiKey, workspaceName, expectedStats); } - Stream getTraceStats__whenFilterByDuration__thenReturnTracesFiltered() { - return Stream.of( - arguments(Operator.EQUAL, - Duration.ofMillis(1L).toNanos() / 1000, 1.0), - arguments(Operator.GREATER_THAN, - Duration.ofMillis(8L).toNanos() / 1000, 7.0), - arguments(Operator.GREATER_THAN_EQUAL, - Duration.ofMillis(1L).toNanos() / 1000, 1.0), - arguments(Operator.GREATER_THAN_EQUAL, - Duration.ofMillis(1L).plusNanos(1000).toNanos() / 1000, 1.0), - arguments(Operator.LESS_THAN, - Duration.ofMillis(1L).plusNanos(1).toNanos() / 1000, 2.0), - arguments(Operator.LESS_THAN_EQUAL, - Duration.ofMillis(1L).toNanos() / 1000, 1.0), - arguments(Operator.LESS_THAN_EQUAL, - Duration.ofMillis(1L).toNanos() / 1000, 2.0)); - } - private void getStatsAndAssert(String projectName, UUID projectId, List filters, String apiKey, String workspaceName, List> expectedStats) { WebTarget webTarget = client.target(URL_TEMPLATE.formatted(baseURI)) From cb763b350d33d05387a617df260a492202702094 Mon Sep 17 00:00:00 2001 From: Ido Berkovich Date: Wed, 18 Dec 2024 13:42:45 +0200 Subject: [PATCH 10/12] OPIK-446 data layer logic encapsulation --- .../src/main/java/com/comet/opik/domain/SpanDAO.java | 7 ++++++- .../src/main/java/com/comet/opik/domain/TraceDAO.java | 7 ++++++- .../com/comet/opik/domain/filter/FilterQueryBuilder.java | 7 +------ .../java/com/comet/opik/domain/filter/FilterStrategy.java | 3 ++- 4 files changed, 15 insertions(+), 9 deletions(-) diff --git a/apps/opik-backend/src/main/java/com/comet/opik/domain/SpanDAO.java b/apps/opik-backend/src/main/java/com/comet/opik/domain/SpanDAO.java index 5fea2d7f0f..9218dd2f2e 100644 --- a/apps/opik-backend/src/main/java/com/comet/opik/domain/SpanDAO.java +++ b/apps/opik-backend/src/main/java/com/comet/opik/domain/SpanDAO.java @@ -554,7 +554,11 @@ AND id in ( FROM ( SELECT - id + id, + if(end_time IS NOT NULL AND start_time IS NOT NULL + AND notEquals(start_time, toDateTime64('1970-01-01 00:00:00.000', 9)), + (dateDiff('microsecond', start_time, end_time) / 1000.0), + NULL) AS duration_millis FROM spans WHERE project_id = :project_id AND workspace_id = :workspace_id @@ -1211,6 +1215,7 @@ private void bindSearchCriteria(Statement statement, SpanSearchCriteria spanSear .ifPresent(filters -> { filterQueryBuilder.bind(statement, filters, FilterStrategy.SPAN); filterQueryBuilder.bind(statement, filters, FilterStrategy.FEEDBACK_SCORES); + filterQueryBuilder.bind(statement, filters, FilterStrategy.DURATION); }); } diff --git a/apps/opik-backend/src/main/java/com/comet/opik/domain/TraceDAO.java b/apps/opik-backend/src/main/java/com/comet/opik/domain/TraceDAO.java index b525679585..93ec17835f 100644 --- a/apps/opik-backend/src/main/java/com/comet/opik/domain/TraceDAO.java +++ b/apps/opik-backend/src/main/java/com/comet/opik/domain/TraceDAO.java @@ -405,7 +405,11 @@ WHERE created_at BETWEEN toStartOfDay(yesterday()) AND toStartOfDay(today()) sum(s.total_estimated_cost) as total_estimated_cost FROM ( SELECT - id + id, + if(end_time IS NOT NULL AND start_time IS NOT NULL + AND notEquals(start_time, toDateTime64('1970-01-01 00:00:00.000', 9)), + (dateDiff('microsecond', start_time, end_time) / 1000.0), + NULL) AS duration_millis FROM traces WHERE project_id = :project_id AND workspace_id = :workspace_id @@ -1052,6 +1056,7 @@ private void bindSearchCriteria(TraceSearchCriteria traceSearchCriteria, Stateme filterQueryBuilder.bind(statement, filters, FilterStrategy.TRACE); filterQueryBuilder.bind(statement, filters, FilterStrategy.TRACE_AGGREGATION); filterQueryBuilder.bind(statement, filters, FilterStrategy.FEEDBACK_SCORES); + filterQueryBuilder.bind(statement, filters, FilterStrategy.DURATION); }); } diff --git a/apps/opik-backend/src/main/java/com/comet/opik/domain/filter/FilterQueryBuilder.java b/apps/opik-backend/src/main/java/com/comet/opik/domain/filter/FilterQueryBuilder.java index b2a3d66aff..8edad0657d 100644 --- a/apps/opik-backend/src/main/java/com/comet/opik/domain/filter/FilterQueryBuilder.java +++ b/apps/opik-backend/src/main/java/com/comet/opik/domain/filter/FilterQueryBuilder.java @@ -42,12 +42,7 @@ public class FilterQueryBuilder { private static final String USAGE_PROMPT_TOKENS_ANALYTICS_DB = "usage['prompt_tokens']"; private static final String USAGE_TOTAL_TOKENS_ANALYTICS_DB = "usage['total_tokens']"; private static final String VALUE_ANALYTICS_DB = "value"; - private static final String DURATION_ANALYTICS_DB = """ - if(end_time IS NOT NULL AND start_time IS NOT NULL - AND notEquals(start_time, toDateTime64('1970-01-01 00:00:00.000', 9)), - (dateDiff('microsecond', start_time, end_time) / 1000.0), - NULL) - """; + private static final String DURATION_ANALYTICS_DB = "duration_millis"; private static final Map> ANALYTICS_DB_OPERATOR_MAP = new EnumMap<>(Map.of( Operator.CONTAINS, new EnumMap<>(Map.of( diff --git a/apps/opik-backend/src/main/java/com/comet/opik/domain/filter/FilterStrategy.java b/apps/opik-backend/src/main/java/com/comet/opik/domain/filter/FilterStrategy.java index 72cd9ed865..ffb9f06a7b 100644 --- a/apps/opik-backend/src/main/java/com/comet/opik/domain/filter/FilterStrategy.java +++ b/apps/opik-backend/src/main/java/com/comet/opik/domain/filter/FilterStrategy.java @@ -10,7 +10,8 @@ public enum FilterStrategy { SPAN, EXPERIMENT_ITEM, DATASET_ITEM, - FEEDBACK_SCORES; + FEEDBACK_SCORES, + DURATION; public static final String DYNAMIC_FIELD = ":dynamicField%1$d"; From ca78abb3317683d941d5e6b1eec87cf20d29a1fa Mon Sep 17 00:00:00 2001 From: Ido Berkovich Date: Wed, 18 Dec 2024 14:36:33 +0200 Subject: [PATCH 11/12] OPIK-446 covered invalid metric type flow properly --- .../opik/domain/ProjectMetricsService.java | 5 +- .../v1/priv/ProjectMetricsResourceTest.java | 48 ++++++++++++++++--- 2 files changed, 43 insertions(+), 10 deletions(-) diff --git a/apps/opik-backend/src/main/java/com/comet/opik/domain/ProjectMetricsService.java b/apps/opik-backend/src/main/java/com/comet/opik/domain/ProjectMetricsService.java index c9ffffe327..dce05d0dc8 100644 --- a/apps/opik-backend/src/main/java/com/comet/opik/domain/ProjectMetricsService.java +++ b/apps/opik-backend/src/main/java/com/comet/opik/domain/ProjectMetricsService.java @@ -7,14 +7,12 @@ import com.google.inject.ImplementedBy; import jakarta.inject.Inject; import jakarta.inject.Singleton; -import jakarta.ws.rs.BadRequestException; import lombok.NonNull; import lombok.extern.slf4j.Slf4j; import reactor.core.publisher.Mono; import java.util.List; import java.util.Map; -import java.util.Optional; import java.util.UUID; import java.util.function.BiFunction; import java.util.stream.Collectors; @@ -79,7 +77,6 @@ private List> entriesToResults(List>> getMetricHandler( MetricType metricType) { - return Optional.ofNullable(metricHandler.get(metricType)) - .orElseThrow(() -> new BadRequestException(ERR_PROJECT_METRIC_NOT_SUPPORTED.formatted(metricType))); + return metricHandler.get(metricType); } } diff --git a/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/ProjectMetricsResourceTest.java b/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/ProjectMetricsResourceTest.java index a8dfdf8183..97a1de319b 100644 --- a/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/ProjectMetricsResourceTest.java +++ b/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/ProjectMetricsResourceTest.java @@ -5,6 +5,7 @@ import com.comet.opik.api.Span; import com.comet.opik.api.TimeInterval; import com.comet.opik.api.Trace; +import com.comet.opik.api.error.ErrorMessage; import com.comet.opik.api.metrics.MetricType; import com.comet.opik.api.metrics.ProjectMetricRequest; import com.comet.opik.api.metrics.ProjectMetricResponse; @@ -25,6 +26,7 @@ import com.comet.opik.domain.cost.ModelPrice; import com.comet.opik.infrastructure.DatabaseAnalyticsFactory; import com.comet.opik.podam.PodamFactoryUtils; +import com.comet.opik.utils.JsonUtils; import com.github.tomakehurst.wiremock.client.WireMock; import com.redis.testcontainers.RedisContainer; import jakarta.ws.rs.client.Entity; @@ -33,6 +35,7 @@ import jakarta.ws.rs.core.MediaType; import org.apache.commons.lang3.RandomStringUtils; import org.apache.commons.lang3.reflect.TypeUtils; +import org.apache.hc.core5.http.ContentType; import org.apache.hc.core5.http.HttpStatus; import org.jdbi.v3.core.Jdbi; import org.junit.jupiter.api.AfterAll; @@ -40,6 +43,7 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Nested; +import org.junit.jupiter.api.Test; import org.junit.jupiter.api.TestInstance; import org.junit.jupiter.api.extension.RegisterExtension; import org.junit.jupiter.params.ParameterizedTest; @@ -325,7 +329,7 @@ void happyPath(TimeInterval interval) { @ParameterizedTest @MethodSource - void invalidParameters(ProjectMetricRequest request, String expectedErr) { + void invalidParameters(Entity request, String expectedErr) { // setup mockTargetWorkspace(); @@ -334,7 +338,7 @@ void invalidParameters(ProjectMetricRequest request, String expectedErr) { .request() .header(HttpHeaders.AUTHORIZATION, API_KEY) .header(WORKSPACE_HEADER, WORKSPACE_NAME) - .post(Entity.json(request))) { + .post(request)) { // assertions assertThat(response.getStatus()).isEqualTo(HttpStatus.SC_BAD_REQUEST); @@ -355,13 +359,45 @@ public static Stream invalidParameters() { .interval(TimeInterval.HOURLY).build(); return Stream.of( - arguments(named("start later than end", validReq.toBuilder() + arguments(named("start later than end", Entity.json(validReq.toBuilder() .intervalEnd(now.minus(2, ChronoUnit.HOURS)) - .build()), ProjectMetricsService.ERR_START_BEFORE_END), - arguments(named("start equal to end", validReq.toBuilder() + .build())), ProjectMetricsService.ERR_START_BEFORE_END), + arguments(named("start equal to end", Entity.json(validReq.toBuilder() .intervalStart(now) .intervalEnd(now) - .build()), ProjectMetricsService.ERR_START_BEFORE_END)); + .build())), ProjectMetricsService.ERR_START_BEFORE_END)); + } + + @Test + void invalidMetricType() { + // setup + mockTargetWorkspace(); + + // SUT + Instant now = Instant.now(); + var request = Entity.entity(JsonUtils.writeValueAsString(ProjectMetricRequest.builder() + .intervalStart(now.minus(1, ChronoUnit.HOURS)) + .intervalEnd(now) + .metricType(MetricType.TRACE_COUNT) + .interval(TimeInterval.HOURLY).build().toBuilder() + .metricType(MetricType.DURATION) + .build()) + .replace(MetricType.DURATION.toString(), "non-existing-metric"), + ContentType.APPLICATION_JSON.toString()); + try (var response = client.target(URL_TEMPLATE.formatted(baseURI, UUID.randomUUID())) + .request() + .header(HttpHeaders.AUTHORIZATION, API_KEY) + .header(WORKSPACE_HEADER, WORKSPACE_NAME) + .post(request)) { + + // assertions + assertThat(response.getStatus()).isEqualTo(HttpStatus.SC_BAD_REQUEST); + assertThat(response.hasEntity()).isTrue(); + + var actualError = response.readEntity(ErrorMessage.class); + + assertThat(actualError).isEqualTo(new ErrorMessage(List.of("Unable to process JSON"))); + } } @ParameterizedTest From e539f56e6eb824baa1b3848fcc81cf8fd66b4144 Mon Sep 17 00:00:00 2001 From: Ido Berkovich Date: Wed, 18 Dec 2024 14:42:17 +0200 Subject: [PATCH 12/12] OPIK-446 fixed metrics query --- .../main/java/com/comet/opik/domain/ProjectMetricsDAO.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/apps/opik-backend/src/main/java/com/comet/opik/domain/ProjectMetricsDAO.java b/apps/opik-backend/src/main/java/com/comet/opik/domain/ProjectMetricsDAO.java index 06580820f4..cbeb50af85 100644 --- a/apps/opik-backend/src/main/java/com/comet/opik/domain/ProjectMetricsDAO.java +++ b/apps/opik-backend/src/main/java/com/comet/opik/domain/ProjectMetricsDAO.java @@ -67,7 +67,10 @@ class ProjectMetricsDAOImpl implements ProjectMetricsDAO { SELECT AS bucket, arrayMap(v -> toDecimal64(if(isNaN(v), 0, v), 9), - quantiles(0.5, 0.9, 0.99)(duration_millis) + quantiles(0.5, 0.9, 0.99)(if(end_time IS NOT NULL AND start_time IS NOT NULL + AND notEquals(start_time, toDateTime64('1970-01-01 00:00:00.000', 9)), + (dateDiff('microsecond', start_time, end_time) / 1000.0), + NULL)) ) AS duration FROM traces WHERE project_id = :project_id