From 755d147387cad5246820382bcf1900a502ce2d16 Mon Sep 17 00:00:00 2001 From: Thiago Hora Date: Thu, 16 Jan 2025 12:57:11 +0100 Subject: [PATCH 01/17] OPIK-795: Create rule logs table --- .../000010_create_evaluation_rule_log_table.sql | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) create mode 100644 apps/opik-backend/src/main/resources/liquibase/db-app-analytics/migrations/000010_create_evaluation_rule_log_table.sql diff --git a/apps/opik-backend/src/main/resources/liquibase/db-app-analytics/migrations/000010_create_evaluation_rule_log_table.sql b/apps/opik-backend/src/main/resources/liquibase/db-app-analytics/migrations/000010_create_evaluation_rule_log_table.sql new file mode 100644 index 0000000000..b52039280f --- /dev/null +++ b/apps/opik-backend/src/main/resources/liquibase/db-app-analytics/migrations/000010_create_evaluation_rule_log_table.sql @@ -0,0 +1,17 @@ +--liquibase formatted sql +--changeset thiagohora:000010_create_automation_rule_log_table + +CREATE TABLE IF NOT EXISTS ${ANALYTICS_DB_DATABASE_NAME}.automation_rule_evaluator_logs ( + timestamp DateTime64(9, 'UTC') DEFAULT now64(9), + workspace_id String, + rule_id FixedString(36), + level Enum8('INFO'=0, 'ERROR'=1), + message String, + extra Map(String, String), + INDEX idx_workspace_rule_id (workspace_id, rule_id) TYPE bloom_filter(0.01) +) +ENGINE = MergeTree() +ORDER BY (timestamp, workspace_id, rule_id, level) +TTL toDateTime(timestamp + INTERVAL 6 MONTH); + +--rollback DROP TABLE IF EXISTS ${ANALYTICS_DB_DATABASE_NAME}.automation_rule_evaluator_logs; From 9fa8f0b17539d21320f78c4a55fe2c6c86eb839c Mon Sep 17 00:00:00 2001 From: Thiago Hora Date: Thu, 16 Jan 2025 22:38:53 +0100 Subject: [PATCH 02/17] OPIK-796: Implement clickhouse user facing logs --- apps/opik-backend/config.yml | 10 ++ .../v1/events/OnlineScoringEventListener.java | 111 +++++++++--- .../ClickHouseLogAppenderConfig.java | 18 ++ .../infrastructure/OpikConfiguration.java | 3 + .../db/DatabaseAnalyticsModule.java | 14 +- .../log/ClickHouseAppender.java | 159 ++++++++++++++++++ .../log/UserFacingRuleLoggingFactory.java | 42 +++++ ...00010_create_evaluation_rule_log_table.sql | 2 +- .../src/test/resources/config-test.yml | 10 ++ 9 files changed, 339 insertions(+), 30 deletions(-) create mode 100644 apps/opik-backend/src/main/java/com/comet/opik/infrastructure/ClickHouseLogAppenderConfig.java create mode 100644 apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/ClickHouseAppender.java create mode 100644 apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/UserFacingRuleLoggingFactory.java diff --git a/apps/opik-backend/config.yml b/apps/opik-backend/config.yml index 3a452fb41c..dd94e35181 100644 --- a/apps/opik-backend/config.yml +++ b/apps/opik-backend/config.yml @@ -256,3 +256,13 @@ cacheManager: # Default: {} # Description: Dynamically created caches with their respective time to live in seconds automationRules: ${CACHE_MANAGER_AUTOMATION_RULES_DURATION:-PT1S} + +# Configuration for clickhouse log appender +clickHouseLogAppender: + # Default: 1000 + # Description: Number of log messages to be batched before sending to ClickHouse + batchSize: ${CLICKHOUSE_LOG_APPENDER_BATCH_SIZE:-1000} + + # Default: PT0.500S or 500ms + # Description: Time interval after which the log messages are sent to ClickHouse if the batch size is not reached + flushIntervalDuration: ${CLICKHOUSE_LOG_APPENDER_FLUSH_INTERVAL_DURATION:-PT0.500S} \ No newline at end of file diff --git a/apps/opik-backend/src/main/java/com/comet/opik/api/resources/v1/events/OnlineScoringEventListener.java b/apps/opik-backend/src/main/java/com/comet/opik/api/resources/v1/events/OnlineScoringEventListener.java index 5a15f71ea8..27813c52c6 100644 --- a/apps/opik-backend/src/main/java/com/comet/opik/api/resources/v1/events/OnlineScoringEventListener.java +++ b/apps/opik-backend/src/main/java/com/comet/opik/api/resources/v1/events/OnlineScoringEventListener.java @@ -2,26 +2,30 @@ import com.comet.opik.api.AutomationRuleEvaluatorLlmAsJudge; import com.comet.opik.api.AutomationRuleEvaluatorType; +import com.comet.opik.api.FeedbackScoreBatchItem; import com.comet.opik.api.Trace; import com.comet.opik.api.events.TracesCreated; import com.comet.opik.domain.AutomationRuleEvaluatorService; import com.comet.opik.domain.ChatCompletionService; import com.comet.opik.domain.FeedbackScoreService; import com.comet.opik.infrastructure.auth.RequestContext; +import com.comet.opik.infrastructure.log.UserFacingRuleLoggingFactory; import com.google.common.eventbus.EventBus; import com.google.common.eventbus.Subscribe; +import dev.langchain4j.model.chat.response.ChatResponse; import jakarta.inject.Inject; import lombok.extern.slf4j.Slf4j; +import org.slf4j.Logger; +import org.slf4j.MDC; import ru.vyarus.dropwizard.guice.module.installer.feature.eager.EagerSingleton; +import java.math.BigDecimal; import java.util.List; import java.util.Map; import java.util.Random; import java.util.UUID; import java.util.stream.Collectors; -import static com.comet.opik.api.AutomationRuleEvaluatorLlmAsJudge.LlmAsJudgeCode; - @EagerSingleton @Slf4j public class OnlineScoringEventListener { @@ -29,6 +33,7 @@ public class OnlineScoringEventListener { private final AutomationRuleEvaluatorService ruleEvaluatorService; private final ChatCompletionService aiProxyService; private final FeedbackScoreService feedbackScoreService; + private final Logger userFacingLogger; @Inject public OnlineScoringEventListener(EventBus eventBus, @@ -39,6 +44,7 @@ public OnlineScoringEventListener(EventBus eventBus, this.aiProxyService = aiProxyService; this.feedbackScoreService = feedbackScoreService; eventBus.register(this); + userFacingLogger = UserFacingRuleLoggingFactory.getLogger(OnlineScoringEventListener.class); } /** @@ -72,11 +78,29 @@ public void onTracesCreated(TracesCreated tracesBatch) { log.info("Found {} evaluators for project '{}' on workspace '{}'", evaluators.size(), projectId, tracesBatch.workspaceId()); - // for each rule, sample traces and score them - evaluators.forEach(evaluator -> traces.stream() - .filter(e -> random.nextFloat() < evaluator.getSamplingRate()) - .forEach(trace -> score(trace, evaluator.getCode(), tracesBatch.workspaceId(), - tracesBatch.userName()))); + // Important to set the workspaceId for logging purposes + try (MDC.MDCCloseable scope = MDC.putCloseable("workspace_id", tracesBatch.workspaceId())) { + + // for each rule, sample traces and score them + evaluators.forEach(evaluator -> traces.stream() + .filter(trace -> { + boolean sampled = random.nextFloat() < evaluator.getSamplingRate(); + + if (!sampled) { + MDC.put("rule_id", evaluator.getId().toString()); + MDC.put("trace_id", trace.id().toString()); + + userFacingLogger.info( + "The traceId '{}' was skipped for rule: '{}' and per the sampling rate '{}'", + trace.id(), evaluator.getName(), evaluator.getSamplingRate()); + } + + return sampled; + }) + .forEach(trace -> score(trace, evaluator, tracesBatch.workspaceId(), + tracesBatch.userName()))); + } + }); } @@ -85,31 +109,66 @@ public void onTracesCreated(TracesCreated tracesBatch) { * If the evaluator has multiple score definitions, it calls the LLM once per score definition. * * @param trace the trace to score - * @param evaluatorCode the automation rule to score the trace + * @param evaluator the automation rule to score the trace * @param workspaceId the workspace the trace belongs */ - private void score(Trace trace, LlmAsJudgeCode evaluatorCode, String workspaceId, + private void score(Trace trace, AutomationRuleEvaluatorLlmAsJudge evaluator, String workspaceId, String userName) { - var scoreRequest = OnlineScoringEngine.prepareLlmRequest(evaluatorCode, trace); - - var chatResponse = aiProxyService.scoreTrace(scoreRequest, evaluatorCode.model(), workspaceId); - - var scores = OnlineScoringEngine.toFeedbackScores(chatResponse).stream() - .map(item -> item.toBuilder() - .id(trace.id()) - .projectId(trace.projectId()) - .projectName(trace.projectName()) - .build()) - .toList(); + //This is crucial for logging purposes to identify the rule and trace + try (MDC.MDCCloseable ruleScope = MDC.putCloseable("rule_id", evaluator.getId().toString()); + MDC.MDCCloseable traceScope = MDC.putCloseable("trace_id", trace.id().toString())) { - log.info("Received {} scores for traceId '{}' in workspace '{}'. Storing them.", scores.size(), trace.id(), - workspaceId); + processScores(trace, evaluator, workspaceId, userName); + } + } - feedbackScoreService.scoreBatchOfTraces(scores) - .contextWrite(ctx -> ctx.put(RequestContext.USER_NAME, userName) - .put(RequestContext.WORKSPACE_ID, workspaceId)) - .block(); + private void processScores(Trace trace, AutomationRuleEvaluatorLlmAsJudge evaluator, String workspaceId, + String userName) { + userFacingLogger.info("Evaluating traceId '{}' sampled by rule '{}'", trace.id(), evaluator.getName()); + + var scoreRequest = OnlineScoringEngine.prepareLlmRequest(evaluator.getCode(), trace); + + userFacingLogger.info("Sending traceId '{}' to LLM using the following input:\n\n{}", trace.id(), scoreRequest); + + final ChatResponse chatResponse; + + try { + chatResponse = aiProxyService.scoreTrace(scoreRequest, evaluator.getCode().model(), workspaceId); + userFacingLogger.info("Received response for traceId '{}':\n\n{}", trace.id(), chatResponse); + } catch (Exception e) { + userFacingLogger.error("Unexpected while scoring traceId '{}' with rule '{}'", trace.id(), + evaluator.getName()); + throw e; + } + + try { + var scores = OnlineScoringEngine.toFeedbackScores(chatResponse).stream() + .map(item -> item.toBuilder() + .id(trace.id()) + .projectId(trace.projectId()) + .projectName(trace.projectName()) + .build()) + .toList(); + + log.info("Received {} scores for traceId '{}' in workspace '{}'. Storing them.", scores.size(), trace.id(), + workspaceId); + + feedbackScoreService.scoreBatchOfTraces(scores) + .contextWrite(ctx -> ctx.put(RequestContext.USER_NAME, userName) + .put(RequestContext.WORKSPACE_ID, workspaceId)) + .block(); + + Map> loggedScores = scores + .stream() + .collect(Collectors.groupingBy(FeedbackScoreBatchItem::name, + Collectors.mapping(FeedbackScoreBatchItem::value, Collectors.toList()))); + + userFacingLogger.info("Scores for traceId '{}' stored successfully:\n\n{}", trace.id(), loggedScores); + } catch (Exception e) { + userFacingLogger.error("Unexpected while storing scores for traceId '{}'", trace.id()); + throw e; + } } } diff --git a/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/ClickHouseLogAppenderConfig.java b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/ClickHouseLogAppenderConfig.java new file mode 100644 index 0000000000..c4d234dfb4 --- /dev/null +++ b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/ClickHouseLogAppenderConfig.java @@ -0,0 +1,18 @@ +package com.comet.opik.infrastructure; + +import com.fasterxml.jackson.annotation.JsonProperty; +import jakarta.validation.Valid; +import jakarta.validation.constraints.NotNull; +import lombok.Data; + +import java.time.Duration; + +@Data +public class ClickHouseLogAppenderConfig { + + @Valid @JsonProperty + private int batchSize = 500; + + @Valid @JsonProperty + @NotNull private Duration flushIntervalDuration; +} diff --git a/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/OpikConfiguration.java b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/OpikConfiguration.java index d7ec4d7427..e98ee0c2c7 100644 --- a/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/OpikConfiguration.java +++ b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/OpikConfiguration.java @@ -53,4 +53,7 @@ public class OpikConfiguration extends JobConfiguration { @Valid @NotNull @JsonProperty private CacheConfiguration cacheManager = new CacheConfiguration(); + + @Valid @NotNull @JsonProperty + private ClickHouseLogAppenderConfig clickHouseLogAppender = new ClickHouseLogAppenderConfig(); } diff --git a/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/db/DatabaseAnalyticsModule.java b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/db/DatabaseAnalyticsModule.java index 5afa853170..63001278c7 100644 --- a/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/db/DatabaseAnalyticsModule.java +++ b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/db/DatabaseAnalyticsModule.java @@ -1,7 +1,9 @@ package com.comet.opik.infrastructure.db; +import com.comet.opik.infrastructure.ClickHouseLogAppenderConfig; import com.comet.opik.infrastructure.DatabaseAnalyticsFactory; import com.comet.opik.infrastructure.OpikConfiguration; +import com.comet.opik.infrastructure.log.UserFacingRuleLoggingFactory; import com.google.inject.Provides; import io.opentelemetry.api.GlobalOpenTelemetry; import io.opentelemetry.instrumentation.r2dbc.v1_0.R2dbcTelemetry; @@ -19,14 +21,20 @@ public class DatabaseAnalyticsModule extends DropwizardAwareModule { + + private static final String INSERT_STATEMENT = """ + INSERT INTO automation_rule_evaluator_logs (timestamp, level, workspace_id, rule_id, message, extra) + VALUES , 9), + :level, + :workspace_id, + :rule_id, + :message, + mapFromArrays(:extra_keys, :extra_values) + ) + , + }> + ; + """; + + public static synchronized void init(@NonNull ConnectionFactory connectionFactory, int batchSize, + @NonNull Duration flushIntervalDuration) { + if (INSTANCE == null) { + INSTANCE = new ClickHouseAppender(connectionFactory, batchSize, flushIntervalDuration); + INSTANCE.start(); + } + } + + public static ClickHouseAppender getInstance() { + if (INSTANCE == null) { + throw new IllegalStateException("ClickHouseAppender is not initialized"); + } + return INSTANCE; + } + + private static ClickHouseAppender INSTANCE; + + private final ConnectionFactory connectionFactory; + private final int batchSize; + private final Duration flushIntervalDuration; + private volatile boolean running = true; + + private BlockingQueue logQueue; + private ScheduledExecutorService scheduler; + + @Override + public void start() { + if (connectionFactory == null) { + log.error("ClickHouse connection factory is not set"); + return; + } + + logQueue = new LinkedBlockingQueue<>(batchSize * 100); + scheduler = Executors.newSingleThreadScheduledExecutor(); + + // Background flush thread + scheduler.scheduleAtFixedRate(this::flushLogs, flushIntervalDuration.toMillis(), + flushIntervalDuration.toMillis(), TimeUnit.MILLISECONDS); + + super.start(); + } + + private void flushLogs() { + if (logQueue.isEmpty()) return; + + List batch = new ArrayList<>(logQueue.size()); + logQueue.drainTo(batch, logQueue.size()); + + if (batch.isEmpty()) return; + + Mono.from(connectionFactory.create()) + .flatMapMany(conn -> { + + var template = new ST(INSERT_STATEMENT); + List queryItems = getQueryItemPlaceHolder(batch.size()); + + template.add("items", queryItems); + + Statement statement = conn.createStatement(template.render()); + + for (int i = 0; i < batch.size(); i++) { + ILoggingEvent event = batch.get(i); + String logLevel = event.getLevel().toString(); + String workspaceId = Optional.ofNullable(event.getMDCPropertyMap().get("workspace_id")) + .orElseThrow(() -> failWithMessage("workspace_id is not set")); + String traceId = Optional.ofNullable(event.getMDCPropertyMap().get("trace_id")) + .orElseThrow(() -> failWithMessage("trace_id is not set")); + String ruleId = Optional.ofNullable(event.getMDCPropertyMap().get("rule_id")) + .orElseThrow(() -> failWithMessage("rule_id is not set")); + statement + .bind("timestamp" + i, event.getInstant().toString()) + .bind("level" + i, logLevel) + .bind("workspace_id" + i, workspaceId) + .bind("rule_id" + i, ruleId) + .bind("message" + i, event.getFormattedMessage()) + .bind("extra_keys" + i, new String[]{"trace_id"}) + .bind("extra_values" + i, new String[]{traceId}); + } + + return statement.execute(); + }) + .subscribe( + noop -> { + }, + e -> log.error("Failed to insert logs", e)); + } + + private IllegalStateException failWithMessage(String message) { + addError(message); + return new IllegalStateException(message); + } + + @Override + protected void append(ILoggingEvent event) { + if (!running) return; + + boolean added = logQueue.offer(event); + if (!added) { + log.warn("Log queue is full, dropping log: " + event.getFormattedMessage()); + } + + if (logQueue.size() >= batchSize) { + scheduler.execute(this::flushLogs); + } + } + + @Override + public void stop() { + running = false; + super.stop(); + flushLogs(); + scheduler.shutdown(); + } +} diff --git a/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/UserFacingRuleLoggingFactory.java b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/UserFacingRuleLoggingFactory.java new file mode 100644 index 0000000000..192572c29a --- /dev/null +++ b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/UserFacingRuleLoggingFactory.java @@ -0,0 +1,42 @@ +package com.comet.opik.infrastructure.log; + +import ch.qos.logback.classic.AsyncAppender; +import ch.qos.logback.classic.Logger; +import ch.qos.logback.classic.LoggerContext; +import io.r2dbc.spi.ConnectionFactory; +import lombok.NonNull; +import lombok.experimental.UtilityClass; +import org.slf4j.LoggerFactory; + +import java.time.Duration; + +@UtilityClass +public class UserFacingRuleLoggingFactory { + + static final LoggerContext CONTEXT = (LoggerContext) LoggerFactory.getILoggerFactory(); + static final AsyncAppender ASYNC_APPENDER = new AsyncAppender(); + + public static void init(@NonNull ConnectionFactory connectionFactory, int batchSize, + @NonNull Duration flushIntervalSeconds) { + ClickHouseAppender.init(connectionFactory, batchSize, flushIntervalSeconds); + + ClickHouseAppender clickHouseAppender = ClickHouseAppender.getInstance(); + clickHouseAppender.setContext(CONTEXT); + + ASYNC_APPENDER.setContext(CONTEXT); + ASYNC_APPENDER.setNeverBlock(true); + ASYNC_APPENDER.setIncludeCallerData(true); + ASYNC_APPENDER.addAppender(clickHouseAppender); + ASYNC_APPENDER.start(); + + Runtime.getRuntime().addShutdownHook(new Thread(CONTEXT::stop)); + } + + public static org.slf4j.Logger getLogger(Class clazz) { + Logger logger = CONTEXT.getLogger("%s.UserFacingLog".formatted(clazz.getName())); + logger.addAppender(ASYNC_APPENDER); + logger.setAdditive(false); + return logger; + } + +} diff --git a/apps/opik-backend/src/main/resources/liquibase/db-app-analytics/migrations/000010_create_evaluation_rule_log_table.sql b/apps/opik-backend/src/main/resources/liquibase/db-app-analytics/migrations/000010_create_evaluation_rule_log_table.sql index b52039280f..0717cfc2d3 100644 --- a/apps/opik-backend/src/main/resources/liquibase/db-app-analytics/migrations/000010_create_evaluation_rule_log_table.sql +++ b/apps/opik-backend/src/main/resources/liquibase/db-app-analytics/migrations/000010_create_evaluation_rule_log_table.sql @@ -5,7 +5,7 @@ CREATE TABLE IF NOT EXISTS ${ANALYTICS_DB_DATABASE_NAME}.automation_rule_evaluat timestamp DateTime64(9, 'UTC') DEFAULT now64(9), workspace_id String, rule_id FixedString(36), - level Enum8('INFO'=0, 'ERROR'=1), + level Enum8('INFO'=0, 'ERROR'=1, 'WARN'=2, 'DEBUG'=3, 'TRACE'=4), message String, extra Map(String, String), INDEX idx_workspace_rule_id (workspace_id, rule_id) TYPE bloom_filter(0.01) diff --git a/apps/opik-backend/src/test/resources/config-test.yml b/apps/opik-backend/src/test/resources/config-test.yml index e13a6540ea..76a00f89b0 100644 --- a/apps/opik-backend/src/test/resources/config-test.yml +++ b/apps/opik-backend/src/test/resources/config-test.yml @@ -218,3 +218,13 @@ cacheManager: # Default: {} # Description: Dynamically created caches with their respective time to live in seconds testCache: PT1S + +# Configuration for clickhouse log appender +clickHouseLogAppender: + # Default: 1000 + # Description: Number of log messages to be batched before sending to ClickHouse + batchSize: 1000 + + # Default: PT0.500S or 500ms + # Description: Time interval after which the log messages are sent to ClickHouse if the batch size is not reached + flushIntervalDuration: PT0.500S From e28974efb7e64827678e99892cd8b4b880b44e94 Mon Sep 17 00:00:00 2001 From: Thiago Hora Date: Fri, 17 Jan 2025 09:38:54 +0100 Subject: [PATCH 03/17] Address PR review --- apps/opik-backend/config.yml | 2 +- .../v1/events/OnlineScoringEventListener.java | 22 +++++++++++-------- .../log/ClickHouseAppender.java | 9 +++++--- 3 files changed, 20 insertions(+), 13 deletions(-) diff --git a/apps/opik-backend/config.yml b/apps/opik-backend/config.yml index dd94e35181..d408df583b 100644 --- a/apps/opik-backend/config.yml +++ b/apps/opik-backend/config.yml @@ -265,4 +265,4 @@ clickHouseLogAppender: # Default: PT0.500S or 500ms # Description: Time interval after which the log messages are sent to ClickHouse if the batch size is not reached - flushIntervalDuration: ${CLICKHOUSE_LOG_APPENDER_FLUSH_INTERVAL_DURATION:-PT0.500S} \ No newline at end of file + flushIntervalDuration: ${CLICKHOUSE_LOG_APPENDER_FLUSH_INTERVAL_DURATION:-PT0.500S} diff --git a/apps/opik-backend/src/main/java/com/comet/opik/api/resources/v1/events/OnlineScoringEventListener.java b/apps/opik-backend/src/main/java/com/comet/opik/api/resources/v1/events/OnlineScoringEventListener.java index 27813c52c6..ed7cf7ae2a 100644 --- a/apps/opik-backend/src/main/java/com/comet/opik/api/resources/v1/events/OnlineScoringEventListener.java +++ b/apps/opik-backend/src/main/java/com/comet/opik/api/resources/v1/events/OnlineScoringEventListener.java @@ -24,7 +24,11 @@ import java.util.Map; import java.util.Random; import java.util.UUID; -import java.util.stream.Collectors; + +import static java.util.stream.Collectors.groupingBy; +import static java.util.stream.Collectors.mapping; +import static java.util.stream.Collectors.toList; +import static java.util.stream.Collectors.toMap; @EagerSingleton @Slf4j @@ -59,10 +63,10 @@ public void onTracesCreated(TracesCreated tracesBatch) { log.debug(tracesBatch.traces().toString()); Map> tracesByProject = tracesBatch.traces().stream() - .collect(Collectors.groupingBy(Trace::projectId)); + .collect(groupingBy(Trace::projectId)); Map countMap = tracesByProject.entrySet().stream() - .collect(Collectors.toMap(entry -> "projectId: " + entry.getKey(), + .collect(toMap(entry -> "projectId: " + entry.getKey(), entry -> entry.getValue().size())); log.debug("Received traces for workspace '{}': {}", tracesBatch.workspaceId(), countMap); @@ -116,8 +120,8 @@ private void score(Trace trace, AutomationRuleEvaluatorLlmAsJudge evaluator, Str String userName) { //This is crucial for logging purposes to identify the rule and trace - try (MDC.MDCCloseable ruleScope = MDC.putCloseable("rule_id", evaluator.getId().toString()); - MDC.MDCCloseable traceScope = MDC.putCloseable("trace_id", trace.id().toString())) { + try (var ruleScope = MDC.putCloseable("rule_id", evaluator.getId().toString()); + var traceScope = MDC.putCloseable("trace_id", trace.id().toString())) { processScores(trace, evaluator, workspaceId, userName); } @@ -137,7 +141,7 @@ private void processScores(Trace trace, AutomationRuleEvaluatorLlmAsJudge evalua chatResponse = aiProxyService.scoreTrace(scoreRequest, evaluator.getCode().model(), workspaceId); userFacingLogger.info("Received response for traceId '{}':\n\n{}", trace.id(), chatResponse); } catch (Exception e) { - userFacingLogger.error("Unexpected while scoring traceId '{}' with rule '{}'", trace.id(), + userFacingLogger.error("Unexpected error while scoring traceId '{}' with rule '{}'", trace.id(), evaluator.getName()); throw e; } @@ -161,12 +165,12 @@ private void processScores(Trace trace, AutomationRuleEvaluatorLlmAsJudge evalua Map> loggedScores = scores .stream() - .collect(Collectors.groupingBy(FeedbackScoreBatchItem::name, - Collectors.mapping(FeedbackScoreBatchItem::value, Collectors.toList()))); + .collect( + groupingBy(FeedbackScoreBatchItem::name, mapping(FeedbackScoreBatchItem::value, toList()))); userFacingLogger.info("Scores for traceId '{}' stored successfully:\n\n{}", trace.id(), loggedScores); } catch (Exception e) { - userFacingLogger.error("Unexpected while storing scores for traceId '{}'", trace.id()); + userFacingLogger.error("Unexpected error while storing scores for traceId '{}'", trace.id()); throw e; } } diff --git a/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/ClickHouseAppender.java b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/ClickHouseAppender.java index 68e8b436ee..c34fd1e037 100644 --- a/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/ClickHouseAppender.java +++ b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/ClickHouseAppender.java @@ -45,6 +45,7 @@ INSERT INTO automation_rule_evaluator_logs (timestamp, level, workspace_id, rule public static synchronized void init(@NonNull ConnectionFactory connectionFactory, int batchSize, @NonNull Duration flushIntervalDuration) { + if (INSTANCE == null) { INSTANCE = new ClickHouseAppender(connectionFactory, batchSize, flushIntervalDuration); INSTANCE.start(); @@ -75,7 +76,7 @@ public void start() { return; } - logQueue = new LinkedBlockingQueue<>(batchSize * 100); + logQueue = new LinkedBlockingQueue<>(); scheduler = Executors.newSingleThreadScheduledExecutor(); // Background flush thread @@ -105,6 +106,7 @@ private void flushLogs() { for (int i = 0; i < batch.size(); i++) { ILoggingEvent event = batch.get(i); + String logLevel = event.getLevel().toString(); String workspaceId = Optional.ofNullable(event.getMDCPropertyMap().get("workspace_id")) .orElseThrow(() -> failWithMessage("workspace_id is not set")); @@ -112,6 +114,7 @@ private void flushLogs() { .orElseThrow(() -> failWithMessage("trace_id is not set")); String ruleId = Optional.ofNullable(event.getMDCPropertyMap().get("rule_id")) .orElseThrow(() -> failWithMessage("rule_id is not set")); + statement .bind("timestamp" + i, event.getInstant().toString()) .bind("level" + i, logLevel) @@ -131,7 +134,7 @@ private void flushLogs() { } private IllegalStateException failWithMessage(String message) { - addError(message); + log.error(message); return new IllegalStateException(message); } @@ -141,7 +144,7 @@ protected void append(ILoggingEvent event) { boolean added = logQueue.offer(event); if (!added) { - log.warn("Log queue is full, dropping log: " + event.getFormattedMessage()); + log.warn("Log queue is full, dropping log: {}", event.getFormattedMessage()); } if (logQueue.size() >= batchSize) { From e6863eb92b6d6fdc7e3264a4d4c4398e02192bc0 Mon Sep 17 00:00:00 2001 From: Thiago Hora Date: Thu, 16 Jan 2025 12:57:11 +0100 Subject: [PATCH 04/17] OPIK-795: Create rule logs table --- .../000010_create_evaluation_rule_log_table.sql | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) create mode 100644 apps/opik-backend/src/main/resources/liquibase/db-app-analytics/migrations/000010_create_evaluation_rule_log_table.sql diff --git a/apps/opik-backend/src/main/resources/liquibase/db-app-analytics/migrations/000010_create_evaluation_rule_log_table.sql b/apps/opik-backend/src/main/resources/liquibase/db-app-analytics/migrations/000010_create_evaluation_rule_log_table.sql new file mode 100644 index 0000000000..0717cfc2d3 --- /dev/null +++ b/apps/opik-backend/src/main/resources/liquibase/db-app-analytics/migrations/000010_create_evaluation_rule_log_table.sql @@ -0,0 +1,17 @@ +--liquibase formatted sql +--changeset thiagohora:000010_create_automation_rule_log_table + +CREATE TABLE IF NOT EXISTS ${ANALYTICS_DB_DATABASE_NAME}.automation_rule_evaluator_logs ( + timestamp DateTime64(9, 'UTC') DEFAULT now64(9), + workspace_id String, + rule_id FixedString(36), + level Enum8('INFO'=0, 'ERROR'=1, 'WARN'=2, 'DEBUG'=3, 'TRACE'=4), + message String, + extra Map(String, String), + INDEX idx_workspace_rule_id (workspace_id, rule_id) TYPE bloom_filter(0.01) +) +ENGINE = MergeTree() +ORDER BY (timestamp, workspace_id, rule_id, level) +TTL toDateTime(timestamp + INTERVAL 6 MONTH); + +--rollback DROP TABLE IF EXISTS ${ANALYTICS_DB_DATABASE_NAME}.automation_rule_evaluator_logs; From 6ab6a1488b31a610f0259c3586aef2fac56eca53 Mon Sep 17 00:00:00 2001 From: Thiago Hora Date: Fri, 17 Jan 2025 12:14:50 +0100 Subject: [PATCH 05/17] Fix file name and column name --- ... 000010_create_automation_rule_evaluator_logs_table.sql} | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) rename apps/opik-backend/src/main/resources/liquibase/db-app-analytics/migrations/{000010_create_evaluation_rule_log_table.sql => 000010_create_automation_rule_evaluator_logs_table.sql} (76%) diff --git a/apps/opik-backend/src/main/resources/liquibase/db-app-analytics/migrations/000010_create_evaluation_rule_log_table.sql b/apps/opik-backend/src/main/resources/liquibase/db-app-analytics/migrations/000010_create_automation_rule_evaluator_logs_table.sql similarity index 76% rename from apps/opik-backend/src/main/resources/liquibase/db-app-analytics/migrations/000010_create_evaluation_rule_log_table.sql rename to apps/opik-backend/src/main/resources/liquibase/db-app-analytics/migrations/000010_create_automation_rule_evaluator_logs_table.sql index 0717cfc2d3..0253fb537b 100644 --- a/apps/opik-backend/src/main/resources/liquibase/db-app-analytics/migrations/000010_create_evaluation_rule_log_table.sql +++ b/apps/opik-backend/src/main/resources/liquibase/db-app-analytics/migrations/000010_create_automation_rule_evaluator_logs_table.sql @@ -1,13 +1,13 @@ --liquibase formatted sql ---changeset thiagohora:000010_create_automation_rule_log_table +--changeset thiagohora:create_automation_rule_evaluator_logs_table CREATE TABLE IF NOT EXISTS ${ANALYTICS_DB_DATABASE_NAME}.automation_rule_evaluator_logs ( timestamp DateTime64(9, 'UTC') DEFAULT now64(9), workspace_id String, rule_id FixedString(36), - level Enum8('INFO'=0, 'ERROR'=1, 'WARN'=2, 'DEBUG'=3, 'TRACE'=4), + level Enum8('TRACE'=0, 'DEBUG'=1, 'INFO'=2, 'WARM'=3, 'ERROR'=4), message String, - extra Map(String, String), + markers Map(String, String), INDEX idx_workspace_rule_id (workspace_id, rule_id) TYPE bloom_filter(0.01) ) ENGINE = MergeTree() From 249d38410d74db8f89dd92095d9586207a3debb3 Mon Sep 17 00:00:00 2001 From: Thiago Hora Date: Fri, 17 Jan 2025 10:15:41 +0100 Subject: [PATCH 06/17] Fix tests --- .../ClickHouseLogAppenderConfig.java | 2 +- .../log/ClickHouseAppender.java | 17 ++++++++----- .../log/UserFacingRuleLoggingFactory.java | 25 +++++++++++-------- 3 files changed, 27 insertions(+), 17 deletions(-) diff --git a/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/ClickHouseLogAppenderConfig.java b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/ClickHouseLogAppenderConfig.java index c4d234dfb4..9f571f38ea 100644 --- a/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/ClickHouseLogAppenderConfig.java +++ b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/ClickHouseLogAppenderConfig.java @@ -11,7 +11,7 @@ public class ClickHouseLogAppenderConfig { @Valid @JsonProperty - private int batchSize = 500; + private int batchSize = 1000; @Valid @JsonProperty @NotNull private Duration flushIntervalDuration; diff --git a/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/ClickHouseAppender.java b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/ClickHouseAppender.java index c34fd1e037..ce4df2a993 100644 --- a/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/ClickHouseAppender.java +++ b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/ClickHouseAppender.java @@ -43,23 +43,27 @@ INSERT INTO automation_rule_evaluator_logs (timestamp, level, workspace_id, rule ; """; + private static ClickHouseAppender instance; + public static synchronized void init(@NonNull ConnectionFactory connectionFactory, int batchSize, @NonNull Duration flushIntervalDuration) { - if (INSTANCE == null) { - INSTANCE = new ClickHouseAppender(connectionFactory, batchSize, flushIntervalDuration); - INSTANCE.start(); + if (instance == null) { + setInstance(new ClickHouseAppender(connectionFactory, batchSize, flushIntervalDuration)); + instance.start(); } } public static ClickHouseAppender getInstance() { - if (INSTANCE == null) { + if (instance == null) { throw new IllegalStateException("ClickHouseAppender is not initialized"); } - return INSTANCE; + return instance; } - private static ClickHouseAppender INSTANCE; + private static synchronized void setInstance(ClickHouseAppender instance) { + ClickHouseAppender.instance = instance; + } private final ConnectionFactory connectionFactory; private final int batchSize; @@ -157,6 +161,7 @@ public void stop() { running = false; super.stop(); flushLogs(); + setInstance(null); scheduler.shutdown(); } } diff --git a/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/UserFacingRuleLoggingFactory.java b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/UserFacingRuleLoggingFactory.java index 192572c29a..60798bf7fe 100644 --- a/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/UserFacingRuleLoggingFactory.java +++ b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/UserFacingRuleLoggingFactory.java @@ -13,28 +13,33 @@ @UtilityClass public class UserFacingRuleLoggingFactory { - static final LoggerContext CONTEXT = (LoggerContext) LoggerFactory.getILoggerFactory(); - static final AsyncAppender ASYNC_APPENDER = new AsyncAppender(); + private static final LoggerContext CONTEXT = (LoggerContext) LoggerFactory.getILoggerFactory(); + private static AsyncAppender asyncAppender; - public static void init(@NonNull ConnectionFactory connectionFactory, int batchSize, + public static synchronized void init(@NonNull ConnectionFactory connectionFactory, int batchSize, @NonNull Duration flushIntervalSeconds) { ClickHouseAppender.init(connectionFactory, batchSize, flushIntervalSeconds); ClickHouseAppender clickHouseAppender = ClickHouseAppender.getInstance(); clickHouseAppender.setContext(CONTEXT); - ASYNC_APPENDER.setContext(CONTEXT); - ASYNC_APPENDER.setNeverBlock(true); - ASYNC_APPENDER.setIncludeCallerData(true); - ASYNC_APPENDER.addAppender(clickHouseAppender); - ASYNC_APPENDER.start(); + asyncAppender = new AsyncAppender(); + asyncAppender.setContext(CONTEXT); + asyncAppender.setNeverBlock(true); + asyncAppender.setIncludeCallerData(true); + asyncAppender.addAppender(clickHouseAppender); + asyncAppender.start(); + addShutdownHook(); + } + + private static void addShutdownHook() { Runtime.getRuntime().addShutdownHook(new Thread(CONTEXT::stop)); } - public static org.slf4j.Logger getLogger(Class clazz) { + public static org.slf4j.Logger getLogger(@NonNull Class clazz) { Logger logger = CONTEXT.getLogger("%s.UserFacingLog".formatted(clazz.getName())); - logger.addAppender(ASYNC_APPENDER); + logger.addAppender(asyncAppender); logger.setAdditive(false); return logger; } From 69fc3e3c42d49215dfeb6987f181934dda54513f Mon Sep 17 00:00:00 2001 From: Thiago Hora Date: Fri, 17 Jan 2025 16:22:41 +0100 Subject: [PATCH 07/17] Change table sortable key --- .../000010_create_automation_rule_evaluator_logs_table.sql | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/apps/opik-backend/src/main/resources/liquibase/db-app-analytics/migrations/000010_create_automation_rule_evaluator_logs_table.sql b/apps/opik-backend/src/main/resources/liquibase/db-app-analytics/migrations/000010_create_automation_rule_evaluator_logs_table.sql index 0253fb537b..7c3fa82058 100644 --- a/apps/opik-backend/src/main/resources/liquibase/db-app-analytics/migrations/000010_create_automation_rule_evaluator_logs_table.sql +++ b/apps/opik-backend/src/main/resources/liquibase/db-app-analytics/migrations/000010_create_automation_rule_evaluator_logs_table.sql @@ -7,11 +7,10 @@ CREATE TABLE IF NOT EXISTS ${ANALYTICS_DB_DATABASE_NAME}.automation_rule_evaluat rule_id FixedString(36), level Enum8('TRACE'=0, 'DEBUG'=1, 'INFO'=2, 'WARM'=3, 'ERROR'=4), message String, - markers Map(String, String), - INDEX idx_workspace_rule_id (workspace_id, rule_id) TYPE bloom_filter(0.01) + markers Map(String, String) ) ENGINE = MergeTree() -ORDER BY (timestamp, workspace_id, rule_id, level) +ORDER BY (workspace_id, rule_id, timestamp) TTL toDateTime(timestamp + INTERVAL 6 MONTH); --rollback DROP TABLE IF EXISTS ${ANALYTICS_DB_DATABASE_NAME}.automation_rule_evaluator_logs; From 22feff1fcc45120db7de6d81bade9c773c781e4d Mon Sep 17 00:00:00 2001 From: Thiago Hora Date: Fri, 17 Jan 2025 16:37:34 +0100 Subject: [PATCH 08/17] Fix marker field ingestion --- .../log/ClickHouseAppender.java | 26 +++++++++---------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/ClickHouseAppender.java b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/ClickHouseAppender.java index ce4df2a993..178f9f63e0 100644 --- a/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/ClickHouseAppender.java +++ b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/ClickHouseAppender.java @@ -28,18 +28,18 @@ class ClickHouseAppender extends AppenderBase { private static final String INSERT_STATEMENT = """ - INSERT INTO automation_rule_evaluator_logs (timestamp, level, workspace_id, rule_id, message, extra) + INSERT INTO automation_rule_evaluator_logs (timestamp, level, workspace_id, rule_id, message, markers) VALUES , 9), - :level, - :workspace_id, - :rule_id, - :message, - mapFromArrays(:extra_keys, :extra_values) - ) - , - }> + ( + parseDateTime64BestEffort(:timestamp, 9), + :level, + :workspace_id, + :rule_id, + :message, + mapFromArrays(:marker_keys, :marker_values) + ) + , + }> ; """; @@ -125,8 +125,8 @@ private void flushLogs() { .bind("workspace_id" + i, workspaceId) .bind("rule_id" + i, ruleId) .bind("message" + i, event.getFormattedMessage()) - .bind("extra_keys" + i, new String[]{"trace_id"}) - .bind("extra_values" + i, new String[]{traceId}); + .bind("marker_keys" + i, new String[]{"trace_id"}) + .bind("marker_values" + i, new String[]{traceId}); } return statement.execute(); From a400485fce5d25ccbec61e940ed5bb4d78023e86 Mon Sep 17 00:00:00 2001 From: Thiago Hora Date: Mon, 20 Jan 2025 12:11:20 +0100 Subject: [PATCH 09/17] [OPIK-796] Implement get log API --- .../java/com/comet/opik/api/LogCriteria.java | 19 ++ .../main/java/com/comet/opik/api/LogItem.java | 43 ++++ .../AutomationRuleEvaluatorsResource.java | 20 ++ .../AutomationRuleEvaluatorLogsDAO.java | 102 ++++++++++ .../AutomationRuleEvaluatorService.java | 11 + .../log/ClickHouseAppender.java | 3 +- ...AutomationRuleEvaluatorResourceClient.java | 16 ++ .../v1/priv/AuthenticationResourceTest.java | 8 +- .../AutomationRuleEvaluatorsResourceTest.java | 191 +++++++++++++++++- 9 files changed, 401 insertions(+), 12 deletions(-) create mode 100644 apps/opik-backend/src/main/java/com/comet/opik/api/LogCriteria.java create mode 100644 apps/opik-backend/src/main/java/com/comet/opik/api/LogItem.java create mode 100644 apps/opik-backend/src/main/java/com/comet/opik/domain/AutomationRuleEvaluatorLogsDAO.java diff --git a/apps/opik-backend/src/main/java/com/comet/opik/api/LogCriteria.java b/apps/opik-backend/src/main/java/com/comet/opik/api/LogCriteria.java new file mode 100644 index 0000000000..a8fe507f1e --- /dev/null +++ b/apps/opik-backend/src/main/java/com/comet/opik/api/LogCriteria.java @@ -0,0 +1,19 @@ +package com.comet.opik.api; + +import lombok.Builder; +import lombok.NonNull; + +import java.util.Map; +import java.util.UUID; + +import static com.comet.opik.api.LogItem.LogLevel; + +@Builder +public record LogCriteria( + @NonNull String workspaceId, + UUID entityId, + LogLevel level, + Integer size, + Map markers) { + +} diff --git a/apps/opik-backend/src/main/java/com/comet/opik/api/LogItem.java b/apps/opik-backend/src/main/java/com/comet/opik/api/LogItem.java new file mode 100644 index 0000000000..ec9ed59a88 --- /dev/null +++ b/apps/opik-backend/src/main/java/com/comet/opik/api/LogItem.java @@ -0,0 +1,43 @@ +package com.comet.opik.api; + +import com.fasterxml.jackson.annotation.JsonIgnore; +import com.fasterxml.jackson.annotation.JsonIgnoreProperties; +import com.fasterxml.jackson.databind.PropertyNamingStrategies; +import com.fasterxml.jackson.databind.annotation.JsonNaming; +import io.swagger.v3.oas.annotations.media.Schema; +import lombok.Builder; + +import java.time.Instant; +import java.util.List; +import java.util.Map; +import java.util.UUID; + +@Builder +@JsonIgnoreProperties(ignoreUnknown = true) +@JsonNaming(PropertyNamingStrategies.SnakeCaseStrategy.class) +public record LogItem( + @Schema(accessMode = Schema.AccessMode.READ_ONLY) Instant timestamp, + @JsonIgnore String workspaceId, + @Schema(accessMode = Schema.AccessMode.READ_ONLY) UUID ruleId, + @Schema(accessMode = Schema.AccessMode.READ_ONLY) LogLevel level, + @Schema(accessMode = Schema.AccessMode.READ_ONLY) String message, + @Schema(accessMode = Schema.AccessMode.READ_ONLY) Map markers) { + + public enum LogLevel { + INFO, + WARN, + ERROR, + DEBUG, + TRACE + } + + @Builder + @JsonIgnoreProperties(ignoreUnknown = true) + @JsonNaming(PropertyNamingStrategies.SnakeCaseStrategy.class) + public record LogPage(List content, int page, int size, long total) implements Page { + + public static LogPage empty(int page) { + return new LogPage(List.of(), page, 0, 0); + } + } +} diff --git a/apps/opik-backend/src/main/java/com/comet/opik/api/resources/v1/priv/AutomationRuleEvaluatorsResource.java b/apps/opik-backend/src/main/java/com/comet/opik/api/resources/v1/priv/AutomationRuleEvaluatorsResource.java index 146694326a..47ee51e81c 100644 --- a/apps/opik-backend/src/main/java/com/comet/opik/api/resources/v1/priv/AutomationRuleEvaluatorsResource.java +++ b/apps/opik-backend/src/main/java/com/comet/opik/api/resources/v1/priv/AutomationRuleEvaluatorsResource.java @@ -4,6 +4,7 @@ import com.comet.opik.api.AutomationRuleEvaluator; import com.comet.opik.api.AutomationRuleEvaluatorUpdate; import com.comet.opik.api.BatchDelete; +import com.comet.opik.api.LogCriteria; import com.comet.opik.api.Page; import com.comet.opik.domain.AutomationRuleEvaluatorService; import com.comet.opik.infrastructure.auth.RequestContext; @@ -43,6 +44,7 @@ import static com.comet.opik.api.AutomationRuleEvaluator.AutomationRuleEvaluatorPage; import static com.comet.opik.api.AutomationRuleEvaluator.View; +import static com.comet.opik.api.LogItem.LogPage; @Path("/v1/private/automations/projects/{projectId}/evaluators/") @Produces(MediaType.APPLICATION_JSON) @@ -164,4 +166,22 @@ public Response deleteEvaluators( return Response.noContent().build(); } + @GET + @Path("/{id}/logs") + @Operation(operationId = "getEvaluatorLogsById", summary = "Get automation rule evaluator logs by id", description = "Get automation rule evaluator logs by id", responses = { + @ApiResponse(responseCode = "200", description = "Automation rule evaluator logs resource", content = @Content(schema = @Schema(implementation = LogPage.class))) + }) + public Response getLogs(@PathParam("projectId") UUID projectId, @PathParam("id") UUID evaluatorId, + @QueryParam("size") @Min(1) @DefaultValue("1000") int size) { + String workspaceId = requestContext.get().getWorkspaceId(); + + log.info("Looking for logs for automated evaluator: id '{}' on project_id '{}' and workspace_id '{}'", + evaluatorId, projectId, workspaceId); + var criteria = LogCriteria.builder().workspaceId(workspaceId).entityId(evaluatorId).size(size).build(); + LogPage logs = service.getLogs(criteria).block(); + log.info("Found {} logs for automated evaluator: id '{}' on project_id '{}'and workspace_id '{}'", logs.size(), + evaluatorId, projectId, workspaceId); + + return Response.ok().entity(logs).build(); + } } diff --git a/apps/opik-backend/src/main/java/com/comet/opik/domain/AutomationRuleEvaluatorLogsDAO.java b/apps/opik-backend/src/main/java/com/comet/opik/domain/AutomationRuleEvaluatorLogsDAO.java new file mode 100644 index 0000000000..2dcca620ae --- /dev/null +++ b/apps/opik-backend/src/main/java/com/comet/opik/domain/AutomationRuleEvaluatorLogsDAO.java @@ -0,0 +1,102 @@ +package com.comet.opik.domain; + +import com.comet.opik.api.LogCriteria; +import com.comet.opik.api.LogItem; +import com.google.inject.ImplementedBy; +import io.r2dbc.spi.ConnectionFactory; +import io.r2dbc.spi.Row; +import io.r2dbc.spi.Statement; +import jakarta.inject.Inject; +import jakarta.inject.Singleton; +import lombok.NonNull; +import lombok.RequiredArgsConstructor; +import lombok.extern.slf4j.Slf4j; +import org.stringtemplate.v4.ST; +import reactor.core.publisher.Mono; + +import java.time.Instant; +import java.util.List; +import java.util.Map; +import java.util.Optional; +import java.util.UUID; + +import static com.comet.opik.api.LogItem.LogLevel; +import static com.comet.opik.api.LogItem.LogPage; + +@ImplementedBy(AutomationRuleEvaluatorLogsDAOImpl.class) +public interface AutomationRuleEvaluatorLogsDAO { + + Mono findLogs(LogCriteria criteria); + +} + +@Slf4j +@Singleton +@RequiredArgsConstructor(onConstructor_ = @Inject) +class AutomationRuleEvaluatorLogsDAOImpl implements AutomationRuleEvaluatorLogsDAO { + + public static final String FIND_ALL = """ + SELECT * FROM automation_rule_evaluator_logs + WHERE workspace_id = :workspaceId + AND level = :level + AND rule_id = :ruleId + ORDER BY timestamp DESC + LIMIT :limit OFFSET :offset + """; + + private final @NonNull ConnectionFactory connectionFactory; + + public Mono findLogs(@NonNull LogCriteria criteria) { + return Mono.from(connectionFactory.create()) + .flatMapMany(connection -> { + log.info("Finding logs with criteria: {}", criteria); + + var template = new ST(FIND_ALL); + + bindTemplateParameters(criteria, template); + + Statement statement = connection.createStatement(template.render()); + + bindParameters(criteria, statement); + + return statement.execute(); + }) + .flatMap(result -> result.map((row, rowMetadata) -> mapRow(row))) + .collectList() + .map(this::mapPage); + } + + private LogPage mapPage(List logs) { + return LogPage.builder() + .content(logs) + .page(1) + .total(logs.size()) + .size(logs.size()) + .build(); + } + + private LogItem mapRow(Row row) { + return LogItem.builder() + .timestamp(row.get("timestamp", Instant.class)) + .level(LogLevel.valueOf(row.get("level", String.class))) + .workspaceId(row.get("workspace_id", String.class)) + .ruleId(row.get("rule_id", UUID.class)) + .message(row.get("message", String.class)) + .markers(row.get("markers", Map.class)) + .build(); + } + + private void bindTemplateParameters(LogCriteria criteria, ST template) { + Optional.ofNullable(criteria.level()).ifPresent(level -> template.add("level", level)); + Optional.ofNullable(criteria.entityId()).ifPresent(ruleId -> template.add("ruleId", ruleId)); + Optional.ofNullable(criteria.size()).ifPresent(limit -> template.add("limit", limit)); + } + + private void bindParameters(LogCriteria criteria, Statement statement) { + statement.bind("workspaceId", criteria.workspaceId()); + Optional.ofNullable(criteria.level()).ifPresent(level -> statement.bind("level", level)); + Optional.ofNullable(criteria.entityId()).ifPresent(ruleId -> statement.bind("ruleId", ruleId)); + Optional.ofNullable(criteria.size()).ifPresent(limit -> statement.bind("limit", limit)); + } + +} \ No newline at end of file diff --git a/apps/opik-backend/src/main/java/com/comet/opik/domain/AutomationRuleEvaluatorService.java b/apps/opik-backend/src/main/java/com/comet/opik/domain/AutomationRuleEvaluatorService.java index 3335750068..b658e2084c 100644 --- a/apps/opik-backend/src/main/java/com/comet/opik/domain/AutomationRuleEvaluatorService.java +++ b/apps/opik-backend/src/main/java/com/comet/opik/domain/AutomationRuleEvaluatorService.java @@ -6,6 +6,7 @@ import com.comet.opik.api.AutomationRuleEvaluatorLlmAsJudge; import com.comet.opik.api.AutomationRuleEvaluatorType; import com.comet.opik.api.AutomationRuleEvaluatorUpdate; +import com.comet.opik.api.LogCriteria; import com.comet.opik.api.error.EntityAlreadyExistsException; import com.comet.opik.api.error.ErrorMessage; import com.comet.opik.infrastructure.cache.CacheEvict; @@ -19,6 +20,7 @@ import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; import org.jdbi.v3.core.statement.UnableToExecuteStatementException; +import reactor.core.publisher.Mono; import ru.vyarus.guicey.jdbi3.tx.TransactionTemplate; import java.sql.SQLIntegrityConstraintViolationException; @@ -28,6 +30,7 @@ import java.util.UUID; import static com.comet.opik.api.AutomationRuleEvaluator.AutomationRuleEvaluatorPage; +import static com.comet.opik.api.LogItem.LogPage; import static com.comet.opik.infrastructure.db.TransactionTemplateAsync.READ_ONLY; import static com.comet.opik.infrastructure.db.TransactionTemplateAsync.WRITE; @@ -50,6 +53,8 @@ AutomationRuleEvaluatorPage find(@NonNull UUID projectId, @NonNull String worksp List findAll(@NonNull UUID projectId, @NonNull String workspaceId, AutomationRuleEvaluatorType automationRuleEvaluatorType); + + Mono getLogs(LogCriteria criteria); } @Singleton @@ -61,6 +66,7 @@ class AutomationRuleEvaluatorServiceImpl implements AutomationRuleEvaluatorServi private final @NonNull IdGenerator idGenerator; private final @NonNull TransactionTemplate template; + private final @NonNull AutomationRuleEvaluatorLogsDAO logsDAO; @Override @CacheEvict(name = "automation_rule_evaluators_find_by_type", key = "$projectId +'-'+ $workspaceId +'-'+ $inputRuleEvaluator.type") @@ -250,4 +256,9 @@ public List findAll(@NonNull UUID projectId, }); } + @Override + public Mono getLogs(@NonNull LogCriteria criteria) { + return logsDAO.findLogs(criteria); + } + } diff --git a/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/ClickHouseAppender.java b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/ClickHouseAppender.java index 178f9f63e0..13154685ad 100644 --- a/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/ClickHouseAppender.java +++ b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/ClickHouseAppender.java @@ -118,13 +118,14 @@ private void flushLogs() { .orElseThrow(() -> failWithMessage("trace_id is not set")); String ruleId = Optional.ofNullable(event.getMDCPropertyMap().get("rule_id")) .orElseThrow(() -> failWithMessage("rule_id is not set")); + String message = event.getFormattedMessage(); statement .bind("timestamp" + i, event.getInstant().toString()) .bind("level" + i, logLevel) .bind("workspace_id" + i, workspaceId) .bind("rule_id" + i, ruleId) - .bind("message" + i, event.getFormattedMessage()) + .bind("message" + i, message) .bind("marker_keys" + i, new String[]{"trace_id"}) .bind("marker_values" + i, new String[]{traceId}); } diff --git a/apps/opik-backend/src/test/java/com/comet/opik/api/resources/utils/resources/AutomationRuleEvaluatorResourceClient.java b/apps/opik-backend/src/test/java/com/comet/opik/api/resources/utils/resources/AutomationRuleEvaluatorResourceClient.java index abe0f9339d..bb3b181695 100644 --- a/apps/opik-backend/src/test/java/com/comet/opik/api/resources/utils/resources/AutomationRuleEvaluatorResourceClient.java +++ b/apps/opik-backend/src/test/java/com/comet/opik/api/resources/utils/resources/AutomationRuleEvaluatorResourceClient.java @@ -70,4 +70,20 @@ public void updateEvaluator(UUID evaluatorId, UUID projectId, String workspaceNa } } } + + public AutomationRuleEvaluator getEvaluator(UUID evaluatorId, UUID projectId, String workspaceName, + String apiKey) { + try (var actualResponse = client.target(RESOURCE_PATH.formatted(baseURI, projectId)) + .path(evaluatorId.toString()) + .request() + .header(HttpHeaders.AUTHORIZATION, apiKey) + .accept(MediaType.APPLICATION_JSON_TYPE) + .header(WORKSPACE_HEADER, workspaceName) + .get()) { + + assertThat(actualResponse.getStatusInfo().getStatusCode()).isEqualTo(200); + + return actualResponse.readEntity(AutomationRuleEvaluator.class); + } + } } diff --git a/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/AuthenticationResourceTest.java b/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/AuthenticationResourceTest.java index dabc9eae97..710e490f7f 100644 --- a/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/AuthenticationResourceTest.java +++ b/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/AuthenticationResourceTest.java @@ -10,7 +10,6 @@ import com.comet.opik.api.resources.utils.TestDropwizardAppExtensionUtils; import com.comet.opik.api.resources.utils.WireMockUtils; import com.comet.opik.infrastructure.DatabaseAnalyticsFactory; -import com.comet.opik.podam.PodamFactoryUtils; import com.github.tomakehurst.wiremock.client.WireMock; import com.redis.testcontainers.RedisContainer; import jakarta.ws.rs.client.Entity; @@ -32,7 +31,6 @@ import org.testcontainers.lifecycle.Startables; import ru.vyarus.dropwizard.guice.test.ClientSupport; import ru.vyarus.dropwizard.guice.test.jupiter.ext.TestDropwizardAppExtension; -import uk.co.jemos.podam.api.PodamFactory; import java.sql.SQLException; import java.util.UUID; @@ -65,7 +63,7 @@ class AuthenticationResourceTest { private static final String UNAUTHORISED_WORKSPACE_NAME = UUID.randomUUID().toString(); @RegisterExtension - private static final TestDropwizardAppExtension app; + private static final TestDropwizardAppExtension APP; private static final WireMockUtils.WireMockRuntime wireMock; @@ -77,12 +75,10 @@ class AuthenticationResourceTest { DatabaseAnalyticsFactory databaseAnalyticsFactory = ClickHouseContainerUtils .newDatabaseAnalyticsFactory(CLICKHOUSE_CONTAINER, DATABASE_NAME); - app = TestDropwizardAppExtensionUtils.newTestDropwizardAppExtension( + APP = TestDropwizardAppExtensionUtils.newTestDropwizardAppExtension( MYSQL.getJdbcUrl(), databaseAnalyticsFactory, wireMock.runtimeInfo(), REDIS.getRedisURI()); } - private final PodamFactory factory = PodamFactoryUtils.newPodamFactory(); - private String baseURI; private ClientSupport client; diff --git a/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/AutomationRuleEvaluatorsResourceTest.java b/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/AutomationRuleEvaluatorsResourceTest.java index c95b1664ec..5aefa8d8ee 100644 --- a/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/AutomationRuleEvaluatorsResourceTest.java +++ b/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/AutomationRuleEvaluatorsResourceTest.java @@ -4,21 +4,34 @@ import com.comet.opik.api.AutomationRuleEvaluatorLlmAsJudge; import com.comet.opik.api.AutomationRuleEvaluatorUpdate; import com.comet.opik.api.BatchDelete; +import com.comet.opik.api.LogItem; +import com.comet.opik.api.Trace; import com.comet.opik.api.resources.utils.AuthTestUtils; +import com.comet.opik.api.resources.utils.ClickHouseContainerUtils; import com.comet.opik.api.resources.utils.ClientSupportUtils; import com.comet.opik.api.resources.utils.MigrationUtils; import com.comet.opik.api.resources.utils.MySQLContainerUtils; import com.comet.opik.api.resources.utils.RedisContainerUtils; import com.comet.opik.api.resources.utils.TestDropwizardAppExtensionUtils; +import com.comet.opik.api.resources.utils.TestDropwizardAppExtensionUtils.AppContextConfig; import com.comet.opik.api.resources.utils.WireMockUtils; import com.comet.opik.api.resources.utils.resources.AutomationRuleEvaluatorResourceClient; +import com.comet.opik.api.resources.utils.resources.ProjectResourceClient; +import com.comet.opik.api.resources.utils.resources.TraceResourceClient; +import com.comet.opik.domain.ChatCompletionService; import com.comet.opik.podam.PodamFactoryUtils; +import com.fasterxml.jackson.core.JsonProcessingException; +import com.fasterxml.jackson.databind.ObjectMapper; import com.github.tomakehurst.wiremock.client.WireMock; +import com.google.inject.AbstractModule; import com.redis.testcontainers.RedisContainer; +import dev.langchain4j.data.message.AiMessage; +import dev.langchain4j.model.chat.response.ChatResponse; import jakarta.ws.rs.HttpMethod; import jakarta.ws.rs.client.Entity; import jakarta.ws.rs.core.HttpHeaders; import jakarta.ws.rs.core.MediaType; +import org.apache.http.HttpStatus; import org.jdbi.v3.core.Jdbi; import org.junit.jupiter.api.AfterAll; import org.junit.jupiter.api.BeforeAll; @@ -31,19 +44,24 @@ import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.MethodSource; +import org.testcontainers.clickhouse.ClickHouseContainer; import org.testcontainers.containers.MySQLContainer; import org.testcontainers.lifecycle.Startables; +import org.testcontainers.shaded.org.awaitility.Awaitility; import ru.vyarus.dropwizard.guice.test.ClientSupport; import ru.vyarus.dropwizard.guice.test.jupiter.ext.TestDropwizardAppExtension; import uk.co.jemos.podam.api.PodamFactory; +import java.time.Instant; import java.util.Collections; import java.util.List; +import java.util.Map; import java.util.Set; import java.util.UUID; import java.util.stream.IntStream; import java.util.stream.Stream; +import static com.comet.opik.api.resources.utils.ClickHouseContainerUtils.DATABASE_NAME; import static com.comet.opik.api.resources.utils.TestHttpClientUtils.UNAUTHORIZED_RESPONSE; import static com.comet.opik.infrastructure.auth.RequestContext.SESSION_COOKIE; import static com.comet.opik.infrastructure.auth.RequestContext.WORKSPACE_HEADER; @@ -55,6 +73,9 @@ import static com.github.tomakehurst.wiremock.client.WireMock.urlPathEqualTo; import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.params.provider.Arguments.arguments; +import static org.mockito.Mockito.any; +import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.mock; @TestInstance(TestInstance.Lifecycle.PER_CLASS) @DisplayName("Automation Rule Evaluators Resource Test") @@ -62,6 +83,53 @@ class AutomationRuleEvaluatorsResourceTest { private static final String URL_TEMPLATE = "%s/v1/private/automations/projects/%s/evaluators/"; + private static final String messageToTest = "Summary: {{summary}}\\nInstruction: {{instruction}}\\n\\n"; + private static final String testEvaluator = """ + { + "model": { "name": "gpt-4o", "temperature": 0.3 }, + "messages": [ + { "role": "USER", "content": "%s" }, + { "role": "SYSTEM", "content": "You're a helpful AI, be cordial." } + ], + "variables": { + "summary": "input.questions.question1", + "instruction": "output.output", + "nonUsed": "input.questions.question2", + "toFail1": "metadata.nonexistent.path" + }, + "schema": [ + { "name": "Relevance", "type": "INTEGER", "description": "Relevance of the summary" }, + { "name": "Conciseness", "type": "DOUBLE", "description": "Conciseness of the summary" }, + { "name": "Technical Accuracy", "type": "BOOLEAN", "description": "Technical accuracy of the summary" } + ] + } + """ + .formatted(messageToTest).trim(); + + private static final String summaryStr = "What was the approach to experimenting with different data mixtures?"; + private static final String outputStr = "The study employed a systematic approach to experiment with varying data mixtures by manipulating the proportions and sources of datasets used for model training."; + private static final String input = """ + { + "questions": { + "question1": "%s", + "question2": "Whatever, we wont use it anyway" + }, + "pdf_url": "https://arxiv.org/pdf/2406.04744", + "title": "CRAG -- Comprehensive RAG Benchmark" + } + """.formatted(summaryStr).trim(); + private static final String output = """ + { + "output": "%s" + } + """.formatted(outputStr).trim(); + + private static final String validAiMsgTxt = "{\"Relevance\":{\"score\":5,\"reason\":\"The summary directly addresses the approach taken in the study by mentioning the systematic experimentation with varying data mixtures and the manipulation of proportions and sources.\"}," + + + "\"Conciseness\":{\"score\":4,\"reason\":\"The summary is mostly concise but could be slightly more streamlined by removing redundant phrases.\"}," + + + "\"Technical Accuracy\":{\"score\":0,\"reason\":\"The summary accurately describes the experimental approach involving data mixtures, proportions, and sources, reflecting the technical details of the study.\"}}"; + private static final String USER = UUID.randomUUID().toString(); private static final String API_KEY = UUID.randomUUID().toString(); private static final String WORKSPACE_ID = UUID.randomUUID().toString(); @@ -71,18 +139,37 @@ class AutomationRuleEvaluatorsResourceTest { private static final MySQLContainer MYSQL = MySQLContainerUtils.newMySQLContainer(); + private static final ClickHouseContainer CLICKHOUSE = ClickHouseContainerUtils.newClickHouseContainer(); + @RegisterExtension - private static final TestDropwizardAppExtension app; + private static final TestDropwizardAppExtension APP; private static final WireMockUtils.WireMockRuntime wireMock; + public static final ChatCompletionService COMPLETION_SERVICE = mock(ChatCompletionService.class); + static { - Startables.deepStart(REDIS, MYSQL).join(); + Startables.deepStart(REDIS, MYSQL, CLICKHOUSE).join(); wireMock = WireMockUtils.startWireMock(); - app = TestDropwizardAppExtensionUtils.newTestDropwizardAppExtension(MYSQL.getJdbcUrl(), null, - wireMock.runtimeInfo(), REDIS.getRedisURI()); + var databaseAnalyticsFactory = ClickHouseContainerUtils.newDatabaseAnalyticsFactory(CLICKHOUSE, DATABASE_NAME); + + APP = TestDropwizardAppExtensionUtils.newTestDropwizardAppExtension( + AppContextConfig.builder() + .jdbcUrl(MYSQL.getJdbcUrl()) + .databaseAnalyticsFactory(databaseAnalyticsFactory) + .redisUrl(REDIS.getRedisURI()) + .runtimeInfo(wireMock.runtimeInfo()) + .modules(List.of(new AbstractModule() { + + @Override + protected void configure() { + bind(ChatCompletionService.class).toInstance(COMPLETION_SERVICE); + } + + })) + .build()); } private final PodamFactory factory = PodamFactoryUtils.newPodamFactory(); @@ -90,6 +177,8 @@ class AutomationRuleEvaluatorsResourceTest { private String baseURI; private ClientSupport client; private AutomationRuleEvaluatorResourceClient evaluatorsResourceClient; + private TraceResourceClient traceResourceClient; + private ProjectResourceClient projectResourceClient; @BeforeAll void setUpAll(ClientSupport client, Jdbi jdbi) { @@ -104,6 +193,8 @@ void setUpAll(ClientSupport client, Jdbi jdbi) { mockTargetWorkspace(API_KEY, WORKSPACE_NAME, WORKSPACE_ID); this.evaluatorsResourceClient = new AutomationRuleEvaluatorResourceClient(this.client, baseURI); + this.traceResourceClient = new TraceResourceClient(this.client, baseURI); + this.projectResourceClient = new ProjectResourceClient(this.client, baseURI, factory); } private static void mockTargetWorkspace(String apiKey, String workspaceName, String workspaceId) { @@ -710,6 +801,96 @@ void deleteProjectAutomationRuleEvaluators__whenSessionTokenIsPresent__thenRetur } } } - } + @ParameterizedTest + @MethodSource("credentials") + @DisplayName("get logs per rule evaluators: when api key is present, then return proper response") + void getLogsPerRuleEvaluators__whenSessionTokenIsPresent__thenReturnProperResponse( + String sessionToken, + boolean isAuthorized, + String workspaceName) throws JsonProcessingException { + + String projectName = UUID.randomUUID().toString(); + + ObjectMapper mapper = new ObjectMapper(); + + var projectId = projectResourceClient.createProject(projectName, API_KEY, WORKSPACE_NAME); + + var evaluator = factory.manufacturePojo(AutomationRuleEvaluatorLlmAsJudge.class).toBuilder() + .id(null) + .code(mapper.readValue(testEvaluator, AutomationRuleEvaluatorLlmAsJudge.LlmAsJudgeCode.class)) + .samplingRate(1f) + .build(); + + var trace = factory.manufacturePojo(Trace.class).toBuilder() + .projectName(projectName) + .input(mapper.readTree(input)) + .output(mapper.readTree(output)) + .build(); + + var id = evaluatorsResourceClient.createEvaluator(evaluator, projectId, WORKSPACE_NAME, API_KEY); + + var chatResponse = ChatResponse.builder().aiMessage(AiMessage.from(validAiMsgTxt)).build(); + + doReturn(chatResponse) + .when(COMPLETION_SERVICE) + .scoreTrace(any(), any(), any()); + + Instant startTime = Instant.now(); + traceResourceClient.createTrace(trace, API_KEY, WORKSPACE_NAME); + + Awaitility.await().untilAsserted(() -> { + + try (var actualResponse = client.target(URL_TEMPLATE.formatted(baseURI, projectId)) + .path(id.toString()) + .path("logs") + .request() + .cookie(SESSION_COOKIE, sessionToken) + .accept(MediaType.APPLICATION_JSON_TYPE) + .header(WORKSPACE_HEADER, workspaceName) + .get()) { + + if (isAuthorized) { + assertThat(actualResponse.getStatusInfo().getStatusCode()).isEqualTo(HttpStatus.SC_OK); + assertThat(actualResponse.hasEntity()).isTrue(); + + var actualEntity = actualResponse.readEntity(LogItem.LogPage.class); + + assertThat(actualEntity.content()).hasSize(4); + assertThat(actualEntity.total()).isEqualTo(4); + assertThat(actualEntity.size()).isEqualTo(4); + assertThat(actualEntity.page()).isEqualTo(1); + + assertThat(actualEntity.content()) + .allSatisfy(log -> { + assertThat(log.timestamp()).isBetween(startTime, Instant.now()); + assertThat(log.ruleId()).isEqualTo(id); + assertThat(log.markers()).isEqualTo(Map.of("trace_id", trace.id().toString())); + assertThat(log.level()).isEqualTo(LogItem.LogLevel.INFO); + }); + + assertThat(actualEntity.content()) + .anyMatch(log -> log.message() + .matches("Scores for traceId '.*' stored successfully:\\n\\n.*")); + + assertThat(actualEntity.content()) + .anyMatch(log -> log.message().matches("Received response for traceId '.*':\\n\\n.*")); + + assertThat(actualEntity.content()) + .anyMatch(log -> log.message().matches( + "(?s)Sending traceId '([^']*)' to LLM using the following input:\\n\\n.*")); + + assertThat(actualEntity.content()) + .anyMatch(log -> log.message().matches("Evaluating traceId '.*' sampled by rule '.*'")); + + } else { + assertThat(actualResponse.getStatusInfo().getStatusCode()) + .isEqualTo(HttpStatus.SC_UNAUTHORIZED); + assertThat(actualResponse.readEntity(io.dropwizard.jersey.errors.ErrorMessage.class)) + .isEqualTo(UNAUTHORIZED_RESPONSE); + } + } + }); + } + } } From 357be9795af675a3e1cd8f266677cdac71e13772 Mon Sep 17 00:00:00 2001 From: Thiago Hora Date: Mon, 20 Jan 2025 13:48:32 +0100 Subject: [PATCH 10/17] Address PR comments --- .../v1/events/OnlineScoringEventListener.java | 15 +++++----- .../db/DatabaseAnalyticsModule.java | 4 +-- .../log/ClickHouseAppender.java | 28 ++++++++++++++----- ...ory.java => UserFacingLoggingFactory.java} | 2 +- 4 files changed, 32 insertions(+), 17 deletions(-) rename apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/{UserFacingRuleLoggingFactory.java => UserFacingLoggingFactory.java} (97%) diff --git a/apps/opik-backend/src/main/java/com/comet/opik/api/resources/v1/events/OnlineScoringEventListener.java b/apps/opik-backend/src/main/java/com/comet/opik/api/resources/v1/events/OnlineScoringEventListener.java index ed7cf7ae2a..7e898e6c3a 100644 --- a/apps/opik-backend/src/main/java/com/comet/opik/api/resources/v1/events/OnlineScoringEventListener.java +++ b/apps/opik-backend/src/main/java/com/comet/opik/api/resources/v1/events/OnlineScoringEventListener.java @@ -9,11 +9,12 @@ import com.comet.opik.domain.ChatCompletionService; import com.comet.opik.domain.FeedbackScoreService; import com.comet.opik.infrastructure.auth.RequestContext; -import com.comet.opik.infrastructure.log.UserFacingRuleLoggingFactory; +import com.comet.opik.infrastructure.log.UserFacingLoggingFactory; import com.google.common.eventbus.EventBus; import com.google.common.eventbus.Subscribe; import dev.langchain4j.model.chat.response.ChatResponse; import jakarta.inject.Inject; +import lombok.NonNull; import lombok.extern.slf4j.Slf4j; import org.slf4j.Logger; import org.slf4j.MDC; @@ -37,18 +38,18 @@ public class OnlineScoringEventListener { private final AutomationRuleEvaluatorService ruleEvaluatorService; private final ChatCompletionService aiProxyService; private final FeedbackScoreService feedbackScoreService; - private final Logger userFacingLogger; + private final @NonNull Logger userFacingLogger; @Inject - public OnlineScoringEventListener(EventBus eventBus, - AutomationRuleEvaluatorService ruleEvaluatorService, - ChatCompletionService aiProxyService, - FeedbackScoreService feedbackScoreService) { + public OnlineScoringEventListener(@NonNull EventBus eventBus, + @NonNull AutomationRuleEvaluatorService ruleEvaluatorService, + @NonNull ChatCompletionService aiProxyService, + @NonNull FeedbackScoreService feedbackScoreService) { this.ruleEvaluatorService = ruleEvaluatorService; this.aiProxyService = aiProxyService; this.feedbackScoreService = feedbackScoreService; eventBus.register(this); - userFacingLogger = UserFacingRuleLoggingFactory.getLogger(OnlineScoringEventListener.class); + userFacingLogger = UserFacingLoggingFactory.getLogger(OnlineScoringEventListener.class); } /** diff --git a/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/db/DatabaseAnalyticsModule.java b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/db/DatabaseAnalyticsModule.java index 63001278c7..3a4bccf61b 100644 --- a/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/db/DatabaseAnalyticsModule.java +++ b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/db/DatabaseAnalyticsModule.java @@ -3,7 +3,7 @@ import com.comet.opik.infrastructure.ClickHouseLogAppenderConfig; import com.comet.opik.infrastructure.DatabaseAnalyticsFactory; import com.comet.opik.infrastructure.OpikConfiguration; -import com.comet.opik.infrastructure.log.UserFacingRuleLoggingFactory; +import com.comet.opik.infrastructure.log.UserFacingLoggingFactory; import com.google.inject.Provides; import io.opentelemetry.api.GlobalOpenTelemetry; import io.opentelemetry.instrumentation.r2dbc.v1_0.R2dbcTelemetry; @@ -27,7 +27,7 @@ protected void configure() { ClickHouseLogAppenderConfig clickHouseLogAppenderConfig = configuration(ClickHouseLogAppenderConfig.class); // Initialize the UserFacingRuleLollingFactory - UserFacingRuleLoggingFactory.init(connectionFactory, clickHouseLogAppenderConfig.getBatchSize(), + UserFacingLoggingFactory.init(connectionFactory, clickHouseLogAppenderConfig.getBatchSize(), clickHouseLogAppenderConfig.getFlushIntervalDuration()); } diff --git a/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/ClickHouseAppender.java b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/ClickHouseAppender.java index 178f9f63e0..bce05f7c01 100644 --- a/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/ClickHouseAppender.java +++ b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/ClickHouseAppender.java @@ -65,9 +65,9 @@ private static synchronized void setInstance(ClickHouseAppender instance) { ClickHouseAppender.instance = instance; } - private final ConnectionFactory connectionFactory; + private final @NonNull ConnectionFactory connectionFactory; + private final @NonNull Duration flushIntervalDuration; private final int batchSize; - private final Duration flushIntervalDuration; private volatile boolean running = true; private BlockingQueue logQueue; @@ -75,10 +75,6 @@ private static synchronized void setInstance(ClickHouseAppender instance) { @Override public void start() { - if (connectionFactory == null) { - log.error("ClickHouse connection factory is not set"); - return; - } logQueue = new LinkedBlockingQueue<>(); scheduler = Executors.newSingleThreadScheduledExecutor(); @@ -144,7 +140,10 @@ private IllegalStateException failWithMessage(String message) { @Override protected void append(ILoggingEvent event) { - if (!running) return; + if (!running) { + log.debug("ClickHouseAppender is stopped, dropping log: {}", event.getFormattedMessage()); + return; + } boolean added = logQueue.offer(event); if (!added) { @@ -163,5 +162,20 @@ public void stop() { flushLogs(); setInstance(null); scheduler.shutdown(); + awaitTermination(); + } + + private void awaitTermination() { + try { + if (!scheduler.awaitTermination(5, TimeUnit.SECONDS)) { + scheduler.shutdownNow(); + if (!scheduler.awaitTermination(5, TimeUnit.SECONDS)) { // Final attempt + log.error("ClickHouseAppender did not terminate"); + } + } + } catch (InterruptedException ie) { + scheduler.shutdownNow(); + log.warn("ClickHouseAppender interrupted while waiting for termination", ie); + } } } diff --git a/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/UserFacingRuleLoggingFactory.java b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/UserFacingLoggingFactory.java similarity index 97% rename from apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/UserFacingRuleLoggingFactory.java rename to apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/UserFacingLoggingFactory.java index 60798bf7fe..04c3732dd2 100644 --- a/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/UserFacingRuleLoggingFactory.java +++ b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/UserFacingLoggingFactory.java @@ -11,7 +11,7 @@ import java.time.Duration; @UtilityClass -public class UserFacingRuleLoggingFactory { +public class UserFacingLoggingFactory { private static final LoggerContext CONTEXT = (LoggerContext) LoggerFactory.getILoggerFactory(); private static AsyncAppender asyncAppender; From 5a2e109c953c6d33ca21d50588bb43d9cabe35b9 Mon Sep 17 00:00:00 2001 From: Thiago Hora Date: Mon, 20 Jan 2025 13:55:49 +0100 Subject: [PATCH 11/17] Fix tests --- .../com/comet/opik/infrastructure/log/ClickHouseAppender.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/ClickHouseAppender.java b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/ClickHouseAppender.java index bce05f7c01..fa1046e4da 100644 --- a/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/ClickHouseAppender.java +++ b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/ClickHouseAppender.java @@ -49,7 +49,7 @@ public static synchronized void init(@NonNull ConnectionFactory connectionFactor @NonNull Duration flushIntervalDuration) { if (instance == null) { - setInstance(new ClickHouseAppender(connectionFactory, batchSize, flushIntervalDuration)); + setInstance(new ClickHouseAppender(connectionFactory, flushIntervalDuration, batchSize)); instance.start(); } } From f9245b1d36e74beb783117bd9b70d02756e28fe4 Mon Sep 17 00:00:00 2001 From: Thiago Hora Date: Mon, 20 Jan 2025 14:50:18 +0100 Subject: [PATCH 12/17] Extract DAO from log appender --- .../v1/events/OnlineScoringEventListener.java | 4 +- .../java/com/comet/opik/domain/UserLog.java | 9 ++ .../log/ClickHouseAppender.java | 102 ++++++------------ .../log/UserFacingLoggingFactory.java | 5 +- .../tables/AutomationRuleEvaluatorLogDAO.java | 83 ++++++++++++++ .../log/tables/UserLogTableFactory.java | 37 +++++++ 6 files changed, 168 insertions(+), 72 deletions(-) create mode 100644 apps/opik-backend/src/main/java/com/comet/opik/domain/UserLog.java create mode 100644 apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/tables/AutomationRuleEvaluatorLogDAO.java create mode 100644 apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/tables/UserLogTableFactory.java diff --git a/apps/opik-backend/src/main/java/com/comet/opik/api/resources/v1/events/OnlineScoringEventListener.java b/apps/opik-backend/src/main/java/com/comet/opik/api/resources/v1/events/OnlineScoringEventListener.java index 7e898e6c3a..f9409b2632 100644 --- a/apps/opik-backend/src/main/java/com/comet/opik/api/resources/v1/events/OnlineScoringEventListener.java +++ b/apps/opik-backend/src/main/java/com/comet/opik/api/resources/v1/events/OnlineScoringEventListener.java @@ -8,6 +8,7 @@ import com.comet.opik.domain.AutomationRuleEvaluatorService; import com.comet.opik.domain.ChatCompletionService; import com.comet.opik.domain.FeedbackScoreService; +import com.comet.opik.domain.UserLog; import com.comet.opik.infrastructure.auth.RequestContext; import com.comet.opik.infrastructure.log.UserFacingLoggingFactory; import com.google.common.eventbus.EventBus; @@ -84,7 +85,8 @@ public void onTracesCreated(TracesCreated tracesBatch) { projectId, tracesBatch.workspaceId()); // Important to set the workspaceId for logging purposes - try (MDC.MDCCloseable scope = MDC.putCloseable("workspace_id", tracesBatch.workspaceId())) { + try (MDC.MDCCloseable logScope = MDC.putCloseable(UserLog.MARKER, UserLog.AUTOMATION_RULE_EVALUATOR.name()); + MDC.MDCCloseable scope = MDC.putCloseable("workspace_id", tracesBatch.workspaceId())) { // for each rule, sample traces and score them evaluators.forEach(evaluator -> traces.stream() diff --git a/apps/opik-backend/src/main/java/com/comet/opik/domain/UserLog.java b/apps/opik-backend/src/main/java/com/comet/opik/domain/UserLog.java new file mode 100644 index 0000000000..f4c7c9878d --- /dev/null +++ b/apps/opik-backend/src/main/java/com/comet/opik/domain/UserLog.java @@ -0,0 +1,9 @@ +package com.comet.opik.domain; + +public enum UserLog { + AUTOMATION_RULE_EVALUATOR + + ; + + public static final String MARKER = "user_log"; +} diff --git a/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/ClickHouseAppender.java b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/ClickHouseAppender.java index fa1046e4da..b24f3b4114 100644 --- a/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/ClickHouseAppender.java +++ b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/ClickHouseAppender.java @@ -2,59 +2,41 @@ import ch.qos.logback.classic.spi.ILoggingEvent; import ch.qos.logback.core.AppenderBase; -import com.comet.opik.utils.TemplateUtils; -import io.r2dbc.spi.ConnectionFactory; -import io.r2dbc.spi.Statement; +import com.comet.opik.domain.UserLog; +import com.comet.opik.infrastructure.log.tables.UserLogTableFactory; import lombok.NonNull; import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; -import org.stringtemplate.v4.ST; -import reactor.core.publisher.Mono; import java.time.Duration; import java.util.ArrayList; import java.util.List; -import java.util.Optional; +import java.util.Map; import java.util.concurrent.BlockingQueue; import java.util.concurrent.Executors; import java.util.concurrent.LinkedBlockingQueue; import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.TimeUnit; +import java.util.stream.Collectors; -import static com.comet.opik.utils.TemplateUtils.getQueryItemPlaceHolder; +import static java.util.stream.Collectors.groupingBy; @RequiredArgsConstructor(access = lombok.AccessLevel.PRIVATE) @Slf4j class ClickHouseAppender extends AppenderBase { - private static final String INSERT_STATEMENT = """ - INSERT INTO automation_rule_evaluator_logs (timestamp, level, workspace_id, rule_id, message, markers) - VALUES , 9), - :level, - :workspace_id, - :rule_id, - :message, - mapFromArrays(:marker_keys, :marker_values) - ) - , - }> - ; - """; - private static ClickHouseAppender instance; - public static synchronized void init(@NonNull ConnectionFactory connectionFactory, int batchSize, + public static synchronized void init(@NonNull UserLogTableFactory userLogTableFactory, int batchSize, @NonNull Duration flushIntervalDuration) { if (instance == null) { - setInstance(new ClickHouseAppender(connectionFactory, flushIntervalDuration, batchSize)); + setInstance(new ClickHouseAppender(userLogTableFactory, flushIntervalDuration, batchSize)); instance.start(); } } - public static ClickHouseAppender getInstance() { + public static synchronized ClickHouseAppender getInstance() { if (instance == null) { throw new IllegalStateException("ClickHouseAppender is not initialized"); } @@ -65,7 +47,7 @@ private static synchronized void setInstance(ClickHouseAppender instance) { ClickHouseAppender.instance = instance; } - private final @NonNull ConnectionFactory connectionFactory; + private final @NonNull UserLogTableFactory userLogTableFactory; private final @NonNull Duration flushIntervalDuration; private final int batchSize; private volatile boolean running = true; @@ -94,48 +76,28 @@ private void flushLogs() { if (batch.isEmpty()) return; - Mono.from(connectionFactory.create()) - .flatMapMany(conn -> { - - var template = new ST(INSERT_STATEMENT); - List queryItems = getQueryItemPlaceHolder(batch.size()); - - template.add("items", queryItems); - - Statement statement = conn.createStatement(template.render()); - - for (int i = 0; i < batch.size(); i++) { - ILoggingEvent event = batch.get(i); - - String logLevel = event.getLevel().toString(); - String workspaceId = Optional.ofNullable(event.getMDCPropertyMap().get("workspace_id")) - .orElseThrow(() -> failWithMessage("workspace_id is not set")); - String traceId = Optional.ofNullable(event.getMDCPropertyMap().get("trace_id")) - .orElseThrow(() -> failWithMessage("trace_id is not set")); - String ruleId = Optional.ofNullable(event.getMDCPropertyMap().get("rule_id")) - .orElseThrow(() -> failWithMessage("rule_id is not set")); - - statement - .bind("timestamp" + i, event.getInstant().toString()) - .bind("level" + i, logLevel) - .bind("workspace_id" + i, workspaceId) - .bind("rule_id" + i, ruleId) - .bind("message" + i, event.getFormattedMessage()) - .bind("marker_keys" + i, new String[]{"trace_id"}) - .bind("marker_values" + i, new String[]{traceId}); + Map> eventsPerTable = batch.stream() + .collect(groupingBy(event -> event.getMDCPropertyMap().getOrDefault(UserLog.MARKER, ""))); + + eventsPerTable + .forEach((userLog, events) -> { + + if (userLog.isBlank()) { + log.error("UserLog marker is not set for events: {}", events.stream() + .map(ILoggingEvent::getFormattedMessage) + .collect(Collectors.joining(", "))); + } else { + UserLogTableFactory.UserLogTableDAO tableDAO = userLogTableFactory + .getDAO(UserLog.valueOf(userLog)); + + tableDAO + .saveAll(events) + .subscribe( + noop -> { + }, + e -> log.error("Failed to insert logs", e)); } - - return statement.execute(); - }) - .subscribe( - noop -> { - }, - e -> log.error("Failed to insert logs", e)); - } - - private IllegalStateException failWithMessage(String message) { - log.error(message); - return new IllegalStateException(message); + }); } @Override @@ -173,9 +135,9 @@ private void awaitTermination() { log.error("ClickHouseAppender did not terminate"); } } - } catch (InterruptedException ie) { + } catch (InterruptedException ex) { scheduler.shutdownNow(); - log.warn("ClickHouseAppender interrupted while waiting for termination", ie); + log.warn("ClickHouseAppender interrupted while waiting for termination", ex); } } } diff --git a/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/UserFacingLoggingFactory.java b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/UserFacingLoggingFactory.java index 04c3732dd2..57c23e0f93 100644 --- a/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/UserFacingLoggingFactory.java +++ b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/UserFacingLoggingFactory.java @@ -3,6 +3,7 @@ import ch.qos.logback.classic.AsyncAppender; import ch.qos.logback.classic.Logger; import ch.qos.logback.classic.LoggerContext; +import com.comet.opik.infrastructure.log.tables.UserLogTableFactory; import io.r2dbc.spi.ConnectionFactory; import lombok.NonNull; import lombok.experimental.UtilityClass; @@ -18,7 +19,9 @@ public class UserFacingLoggingFactory { public static synchronized void init(@NonNull ConnectionFactory connectionFactory, int batchSize, @NonNull Duration flushIntervalSeconds) { - ClickHouseAppender.init(connectionFactory, batchSize, flushIntervalSeconds); + + UserLogTableFactory tableFactory = UserLogTableFactory.getInstance(connectionFactory); + ClickHouseAppender.init(tableFactory, batchSize, flushIntervalSeconds); ClickHouseAppender clickHouseAppender = ClickHouseAppender.getInstance(); clickHouseAppender.setContext(CONTEXT); diff --git a/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/tables/AutomationRuleEvaluatorLogDAO.java b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/tables/AutomationRuleEvaluatorLogDAO.java new file mode 100644 index 0000000000..83f2a33307 --- /dev/null +++ b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/tables/AutomationRuleEvaluatorLogDAO.java @@ -0,0 +1,83 @@ +package com.comet.opik.infrastructure.log.tables; + +import ch.qos.logback.classic.spi.ILoggingEvent; +import com.comet.opik.utils.TemplateUtils; +import io.r2dbc.spi.ConnectionFactory; +import io.r2dbc.spi.Statement; +import lombok.RequiredArgsConstructor; +import lombok.extern.slf4j.Slf4j; +import org.stringtemplate.v4.ST; +import reactor.core.publisher.Mono; + +import java.util.List; +import java.util.Optional; + +import static com.comet.opik.infrastructure.log.tables.UserLogTableFactory.UserLogTableDAO; +import static com.comet.opik.utils.TemplateUtils.getQueryItemPlaceHolder; + +@RequiredArgsConstructor +@Slf4j +class AutomationRuleEvaluatorLogDAO implements UserLogTableDAO { + + private final ConnectionFactory factory; + + private static final String INSERT_STATEMENT = """ + INSERT INTO automation_rule_evaluator_logs (timestamp, level, workspace_id, rule_id, message, markers) + VALUES , 9), + :level, + :workspace_id, + :rule_id, + :message, + mapFromArrays(:marker_keys, :marker_values) + ) + , + }> + ; + """; + + @Override + public Mono saveAll(List events) { + return Mono.from(factory.create()) + .flatMapMany(connection -> { + var template = new ST(INSERT_STATEMENT); + List queryItems = getQueryItemPlaceHolder(events.size()); + + template.add("items", queryItems); + + Statement statement = connection.createStatement(template.render()); + + for (int i = 0; i < events.size(); i++) { + ILoggingEvent event = events.get(i); + + String logLevel = event.getLevel().toString(); + String workspaceId = Optional.ofNullable(event.getMDCPropertyMap().get("workspace_id")) + .orElseThrow(() -> failWithMessage("workspace_id is not set")); + String traceId = Optional.ofNullable(event.getMDCPropertyMap().get("trace_id")) + .orElseThrow(() -> failWithMessage("trace_id is not set")); + String ruleId = Optional.ofNullable(event.getMDCPropertyMap().get("rule_id")) + .orElseThrow(() -> failWithMessage("rule_id is not set")); + + statement + .bind("timestamp" + i, event.getInstant().toString()) + .bind("level" + i, logLevel) + .bind("workspace_id" + i, workspaceId) + .bind("rule_id" + i, ruleId) + .bind("message" + i, event.getFormattedMessage()) + .bind("marker_keys" + i, new String[]{"trace_id"}) + .bind("marker_values" + i, new String[]{traceId}); + } + + return statement.execute(); + }) + .collectList() + .then(); + } + + private IllegalStateException failWithMessage(String message) { + log.error(message); + return new IllegalStateException(message); + } + +} diff --git a/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/tables/UserLogTableFactory.java b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/tables/UserLogTableFactory.java new file mode 100644 index 0000000000..9bce54ce50 --- /dev/null +++ b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/tables/UserLogTableFactory.java @@ -0,0 +1,37 @@ +package com.comet.opik.infrastructure.log.tables; + +import ch.qos.logback.classic.spi.ILoggingEvent; +import com.comet.opik.domain.UserLog; +import io.r2dbc.spi.ConnectionFactory; +import lombok.NonNull; +import lombok.RequiredArgsConstructor; +import reactor.core.publisher.Mono; + +import java.util.List; + +public interface UserLogTableFactory { + + static UserLogTableFactory getInstance(ConnectionFactory factory) { + return new UserLogTableFactoryImpl(factory); + } + + interface UserLogTableDAO { + Mono saveAll(List events); + } + + UserLogTableDAO getDAO(UserLog userLog); + +} + +@RequiredArgsConstructor +class UserLogTableFactoryImpl implements UserLogTableFactory { + + private final ConnectionFactory factory; + + @Override + public UserLogTableDAO getDAO(@NonNull UserLog userLog) { + return switch (userLog) { + case AUTOMATION_RULE_EVALUATOR -> new AutomationRuleEvaluatorLogDAO(factory); + }; + } +} From 49d1ae4de64fb2f1d75fda49436a5c407f8bf9d6 Mon Sep 17 00:00:00 2001 From: Thiago Hora Date: Mon, 20 Jan 2025 17:55:26 +0100 Subject: [PATCH 13/17] Address PR review --- .../v1/events/OnlineScoringEventListener.java | 2 +- .../log/ClickHouseAppender.java | 23 ++++++++----------- .../log/UserFacingLoggingFactory.java | 6 ++--- .../tables/AutomationRuleEvaluatorLogDAO.java | 3 ++- .../log/tables/UserLogTableFactory.java | 14 ++++++----- 5 files changed, 23 insertions(+), 25 deletions(-) diff --git a/apps/opik-backend/src/main/java/com/comet/opik/api/resources/v1/events/OnlineScoringEventListener.java b/apps/opik-backend/src/main/java/com/comet/opik/api/resources/v1/events/OnlineScoringEventListener.java index f9409b2632..f3e17021ee 100644 --- a/apps/opik-backend/src/main/java/com/comet/opik/api/resources/v1/events/OnlineScoringEventListener.java +++ b/apps/opik-backend/src/main/java/com/comet/opik/api/resources/v1/events/OnlineScoringEventListener.java @@ -39,7 +39,7 @@ public class OnlineScoringEventListener { private final AutomationRuleEvaluatorService ruleEvaluatorService; private final ChatCompletionService aiProxyService; private final FeedbackScoreService feedbackScoreService; - private final @NonNull Logger userFacingLogger; + private final Logger userFacingLogger; @Inject public OnlineScoringEventListener(@NonNull EventBus eventBus, diff --git a/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/ClickHouseAppender.java b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/ClickHouseAppender.java index b24f3b4114..52e54dc025 100644 --- a/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/ClickHouseAppender.java +++ b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/ClickHouseAppender.java @@ -1,5 +1,6 @@ package com.comet.opik.infrastructure.log; +import ch.qos.logback.classic.LoggerContext; import ch.qos.logback.classic.spi.ILoggingEvent; import ch.qos.logback.core.AppenderBase; import com.comet.opik.domain.UserLog; @@ -27,19 +28,16 @@ class ClickHouseAppender extends AppenderBase { private static ClickHouseAppender instance; - public static synchronized void init(@NonNull UserLogTableFactory userLogTableFactory, int batchSize, - @NonNull Duration flushIntervalDuration) { + public static synchronized ClickHouseAppender init(@NonNull UserLogTableFactory userLogTableFactory, int batchSize, + @NonNull Duration flushIntervalDuration, @NonNull LoggerContext context) { if (instance == null) { - setInstance(new ClickHouseAppender(userLogTableFactory, flushIntervalDuration, batchSize)); + ClickHouseAppender appender = new ClickHouseAppender(userLogTableFactory, flushIntervalDuration, batchSize); + setInstance(appender); + appender.setContext(context); instance.start(); } - } - public static synchronized ClickHouseAppender getInstance() { - if (instance == null) { - throw new IllegalStateException("ClickHouseAppender is not initialized"); - } return instance; } @@ -52,15 +50,12 @@ private static synchronized void setInstance(ClickHouseAppender instance) { private final int batchSize; private volatile boolean running = true; - private BlockingQueue logQueue; - private ScheduledExecutorService scheduler; + private final BlockingQueue logQueue = new LinkedBlockingQueue<>(); + private ScheduledExecutorService scheduler = Executors.newSingleThreadScheduledExecutor(); @Override public void start() { - logQueue = new LinkedBlockingQueue<>(); - scheduler = Executors.newSingleThreadScheduledExecutor(); - // Background flush thread scheduler.scheduleAtFixedRate(this::flushLogs, flushIntervalDuration.toMillis(), flushIntervalDuration.toMillis(), TimeUnit.MILLISECONDS); @@ -125,6 +120,8 @@ public void stop() { setInstance(null); scheduler.shutdown(); awaitTermination(); + logQueue.clear(); + scheduler = Executors.newSingleThreadScheduledExecutor(); } private void awaitTermination() { diff --git a/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/UserFacingLoggingFactory.java b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/UserFacingLoggingFactory.java index 57c23e0f93..9d62dd2a47 100644 --- a/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/UserFacingLoggingFactory.java +++ b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/UserFacingLoggingFactory.java @@ -21,10 +21,8 @@ public static synchronized void init(@NonNull ConnectionFactory connectionFactor @NonNull Duration flushIntervalSeconds) { UserLogTableFactory tableFactory = UserLogTableFactory.getInstance(connectionFactory); - ClickHouseAppender.init(tableFactory, batchSize, flushIntervalSeconds); - - ClickHouseAppender clickHouseAppender = ClickHouseAppender.getInstance(); - clickHouseAppender.setContext(CONTEXT); + ClickHouseAppender clickHouseAppender = ClickHouseAppender.init(tableFactory, batchSize, flushIntervalSeconds, + CONTEXT); asyncAppender = new AsyncAppender(); asyncAppender.setContext(CONTEXT); diff --git a/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/tables/AutomationRuleEvaluatorLogDAO.java b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/tables/AutomationRuleEvaluatorLogDAO.java index 83f2a33307..a0b976c816 100644 --- a/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/tables/AutomationRuleEvaluatorLogDAO.java +++ b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/tables/AutomationRuleEvaluatorLogDAO.java @@ -4,6 +4,7 @@ import com.comet.opik.utils.TemplateUtils; import io.r2dbc.spi.ConnectionFactory; import io.r2dbc.spi.Statement; +import lombok.NonNull; import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; import org.stringtemplate.v4.ST; @@ -38,7 +39,7 @@ INSERT INTO automation_rule_evaluator_logs (timestamp, level, workspace_id, rule """; @Override - public Mono saveAll(List events) { + public Mono saveAll(@NonNull List events) { return Mono.from(factory.create()) .flatMapMany(connection -> { var template = new ST(INSERT_STATEMENT); diff --git a/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/tables/UserLogTableFactory.java b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/tables/UserLogTableFactory.java index 9bce54ce50..def849d8c0 100644 --- a/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/tables/UserLogTableFactory.java +++ b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/tables/UserLogTableFactory.java @@ -4,10 +4,10 @@ import com.comet.opik.domain.UserLog; import io.r2dbc.spi.ConnectionFactory; import lombok.NonNull; -import lombok.RequiredArgsConstructor; import reactor.core.publisher.Mono; import java.util.List; +import java.util.Map; public interface UserLogTableFactory { @@ -23,15 +23,17 @@ interface UserLogTableDAO { } -@RequiredArgsConstructor class UserLogTableFactoryImpl implements UserLogTableFactory { - private final ConnectionFactory factory; + private final Map daoMap; + + UserLogTableFactoryImpl(@NonNull ConnectionFactory factory) { + daoMap = Map.of( + UserLog.AUTOMATION_RULE_EVALUATOR, new AutomationRuleEvaluatorLogDAO(factory)); + } @Override public UserLogTableDAO getDAO(@NonNull UserLog userLog) { - return switch (userLog) { - case AUTOMATION_RULE_EVALUATOR -> new AutomationRuleEvaluatorLogDAO(factory); - }; + return daoMap.get(userLog); } } From b9476b66cf8b90f81c60c7ad544a9989859a4386 Mon Sep 17 00:00:00 2001 From: Thiago Hora Date: Tue, 21 Jan 2025 20:17:37 +0100 Subject: [PATCH 14/17] Make LLM Providers configurable via tests and add judge scorer to application lifecycle --- .../java/com/comet/opik/OpikApplication.java | 8 +- .../ChunkedResponseHandler.java | 2 +- .../events/OnlineScoringLlmAsJudgeScorer.java | 80 ++++++-- .../v1/priv/ChatCompletionsResource.java | 2 +- .../{ => llm}/ChatCompletionService.java | 4 +- .../opik/domain/llm/LlmProviderFactory.java | 19 ++ .../LlmProviderService.java | 2 +- .../llmproviders/LlmProviderClientModule.java | 19 -- .../opik/infrastructure/llm/LlmModule.java | 20 ++ .../llm/LlmProviderClientGenerator.java | 12 ++ .../llm/LlmProviderFactoryImpl.java} | 45 +++-- .../llm/LlmProviderUnsupportedException.java | 15 ++ .../llm/LlmServiceProvider.java | 13 ++ .../antropic/AnthropicClientGenerator.java | 70 +++++++ .../antropic/AnthropicLlmServiceProvider.java | 34 ++++ .../llm/antropic}/AnthropicModelName.java | 2 +- .../llm/antropic/AnthropicModule.java | 30 +++ .../llm/antropic}/LlmProviderAnthropic.java | 18 +- .../antropic}/LlmProviderAnthropicMapper.java | 4 +- .../llm/gemini/GeminiClientGenerator.java | 55 ++++++ .../llm/gemini/GeminiLlmServiceProvider.java | 29 +++ .../llm/gemini}/GeminiModelName.java | 2 +- .../llm/gemini/GeminiModule.java | 28 +++ .../llm/gemini}/LlmProviderGemini.java | 6 +- .../llm/gemini}/LlmProviderGeminiMapper.java | 4 +- .../llm/openai}/LlmProviderOpenAi.java | 3 +- .../llm/openai/OpenAIClientGenerator.java} | 59 ++---- .../llm/openai/OpenAILlmServiceProvider.java | 29 +++ .../llm/openai/OpenAIModule.java | 28 +++ .../llm/openai}/OpenaiModelName.java | 2 +- .../TestDropwizardAppExtensionUtils.java | 9 +- ...AutomationRuleEvaluatorResourceClient.java | 6 +- .../v1/events/OnlineScoringEngineTest.java | 2 +- .../AutomationRuleEvaluatorsResourceTest.java | 172 +++++++++++++----- .../v1/priv/ChatCompletionsResourceTest.java | 15 +- .../llm}/LlmProviderFactoryTest.java | 38 +++- .../llm/antropic/AnthropicMappersTest.java | 73 ++++++++ .../llm/gemini/GeminiMappersTest.java} | 57 +----- 38 files changed, 777 insertions(+), 239 deletions(-) rename apps/opik-backend/src/main/java/com/comet/opik/{domain/llmproviders => api}/ChunkedResponseHandler.java (98%) rename apps/opik-backend/src/main/java/com/comet/opik/domain/{ => llm}/ChatCompletionService.java (97%) create mode 100644 apps/opik-backend/src/main/java/com/comet/opik/domain/llm/LlmProviderFactory.java rename apps/opik-backend/src/main/java/com/comet/opik/domain/{llmproviders => llm}/LlmProviderService.java (95%) delete mode 100644 apps/opik-backend/src/main/java/com/comet/opik/domain/llmproviders/LlmProviderClientModule.java create mode 100644 apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/LlmModule.java create mode 100644 apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/LlmProviderClientGenerator.java rename apps/opik-backend/src/main/java/com/comet/opik/{domain/llmproviders/LlmProviderFactory.java => infrastructure/llm/LlmProviderFactoryImpl.java} (64%) create mode 100644 apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/LlmProviderUnsupportedException.java create mode 100644 apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/LlmServiceProvider.java create mode 100644 apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/antropic/AnthropicClientGenerator.java create mode 100644 apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/antropic/AnthropicLlmServiceProvider.java rename apps/opik-backend/src/main/java/com/comet/opik/{domain/llmproviders => infrastructure/llm/antropic}/AnthropicModelName.java (94%) create mode 100644 apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/antropic/AnthropicModule.java rename apps/opik-backend/src/main/java/com/comet/opik/{domain/llmproviders => infrastructure/llm/antropic}/LlmProviderAnthropic.java (77%) rename apps/opik-backend/src/main/java/com/comet/opik/{domain/llmproviders => infrastructure/llm/antropic}/LlmProviderAnthropicMapper.java (98%) create mode 100644 apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/gemini/GeminiClientGenerator.java create mode 100644 apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/gemini/GeminiLlmServiceProvider.java rename apps/opik-backend/src/main/java/com/comet/opik/{domain/llmproviders => infrastructure/llm/gemini}/GeminiModelName.java (92%) create mode 100644 apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/gemini/GeminiModule.java rename apps/opik-backend/src/main/java/com/comet/opik/{domain/llmproviders => infrastructure/llm/gemini}/LlmProviderGemini.java (87%) rename apps/opik-backend/src/main/java/com/comet/opik/{domain/llmproviders => infrastructure/llm/gemini}/LlmProviderGeminiMapper.java (98%) rename apps/opik-backend/src/main/java/com/comet/opik/{domain/llmproviders => infrastructure/llm/openai}/LlmProviderOpenAi.java (94%) rename apps/opik-backend/src/main/java/com/comet/opik/{domain/llmproviders/LlmProviderClientGenerator.java => infrastructure/llm/openai/OpenAIClientGenerator.java} (50%) create mode 100644 apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/openai/OpenAILlmServiceProvider.java create mode 100644 apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/openai/OpenAIModule.java rename apps/opik-backend/src/main/java/com/comet/opik/{domain/llmproviders => infrastructure/llm/openai}/OpenaiModelName.java (96%) rename apps/opik-backend/src/test/java/com/comet/opik/{domain/llmproviders => infrastructure/llm}/LlmProviderFactoryTest.java (65%) create mode 100644 apps/opik-backend/src/test/java/com/comet/opik/infrastructure/llm/antropic/AnthropicMappersTest.java rename apps/opik-backend/src/test/java/com/comet/opik/{domain/llmproviders/LlmProviderClientsMappersTest.java => infrastructure/llm/gemini/GeminiMappersTest.java} (54%) diff --git a/apps/opik-backend/src/main/java/com/comet/opik/OpikApplication.java b/apps/opik-backend/src/main/java/com/comet/opik/OpikApplication.java index c09f300c87..e5319711c2 100644 --- a/apps/opik-backend/src/main/java/com/comet/opik/OpikApplication.java +++ b/apps/opik-backend/src/main/java/com/comet/opik/OpikApplication.java @@ -1,7 +1,6 @@ package com.comet.opik; import com.comet.opik.api.error.JsonInvalidFormatExceptionMapper; -import com.comet.opik.domain.llmproviders.LlmProviderClientModule; import com.comet.opik.infrastructure.ConfigurationModule; import com.comet.opik.infrastructure.EncryptionUtils; import com.comet.opik.infrastructure.OpikConfiguration; @@ -16,6 +15,10 @@ import com.comet.opik.infrastructure.events.EventModule; import com.comet.opik.infrastructure.http.HttpModule; import com.comet.opik.infrastructure.job.JobGuiceyInstaller; +import com.comet.opik.infrastructure.llm.LlmModule; +import com.comet.opik.infrastructure.llm.antropic.AnthropicModule; +import com.comet.opik.infrastructure.llm.gemini.GeminiModule; +import com.comet.opik.infrastructure.llm.openai.OpenAIModule; import com.comet.opik.infrastructure.ratelimit.RateLimitModule; import com.comet.opik.infrastructure.redis.RedisModule; import com.comet.opik.utils.JsonBigDecimalDeserializer; @@ -74,7 +77,8 @@ public void initialize(Bootstrap bootstrap) { .withPlugins(new SqlObjectPlugin(), new Jackson2Plugin())) .modules(new DatabaseAnalyticsModule(), new IdGeneratorModule(), new AuthModule(), new RedisModule(), new RateLimitModule(), new NameGeneratorModule(), new HttpModule(), new EventModule(), - new ConfigurationModule(), new BiModule(), new CacheModule(), new LlmProviderClientModule()) + new ConfigurationModule(), new BiModule(), new CacheModule(), new AnthropicModule(), + new GeminiModule(), new OpenAIModule(), new LlmModule()) .installers(JobGuiceyInstaller.class) .listen(new OpikGuiceyLifecycleEventListener()) .enableAutoConfig() diff --git a/apps/opik-backend/src/main/java/com/comet/opik/domain/llmproviders/ChunkedResponseHandler.java b/apps/opik-backend/src/main/java/com/comet/opik/api/ChunkedResponseHandler.java similarity index 98% rename from apps/opik-backend/src/main/java/com/comet/opik/domain/llmproviders/ChunkedResponseHandler.java rename to apps/opik-backend/src/main/java/com/comet/opik/api/ChunkedResponseHandler.java index d5f89a59a7..254d74fe46 100644 --- a/apps/opik-backend/src/main/java/com/comet/opik/domain/llmproviders/ChunkedResponseHandler.java +++ b/apps/opik-backend/src/main/java/com/comet/opik/api/ChunkedResponseHandler.java @@ -1,4 +1,4 @@ -package com.comet.opik.domain.llmproviders; +package com.comet.opik.api; import dev.ai4j.openai4j.chat.ChatCompletionChoice; import dev.ai4j.openai4j.chat.ChatCompletionResponse; diff --git a/apps/opik-backend/src/main/java/com/comet/opik/api/resources/v1/events/OnlineScoringLlmAsJudgeScorer.java b/apps/opik-backend/src/main/java/com/comet/opik/api/resources/v1/events/OnlineScoringLlmAsJudgeScorer.java index 47dd27fbff..2a973b2bc2 100644 --- a/apps/opik-backend/src/main/java/com/comet/opik/api/resources/v1/events/OnlineScoringLlmAsJudgeScorer.java +++ b/apps/opik-backend/src/main/java/com/comet/opik/api/resources/v1/events/OnlineScoringLlmAsJudgeScorer.java @@ -2,15 +2,16 @@ import com.comet.opik.api.FeedbackScoreBatchItem; import com.comet.opik.api.events.TraceToScoreLlmAsJudge; -import com.comet.opik.domain.ChatCompletionService; import com.comet.opik.domain.FeedbackScoreService; import com.comet.opik.domain.UserLog; +import com.comet.opik.domain.llm.ChatCompletionService; import com.comet.opik.infrastructure.OnlineScoringConfig; import com.comet.opik.infrastructure.OnlineScoringConfig.StreamConfiguration; import com.comet.opik.infrastructure.auth.RequestContext; import com.comet.opik.infrastructure.log.UserFacingLoggingFactory; import dev.langchain4j.model.chat.request.ChatRequest; import dev.langchain4j.model.chat.response.ChatResponse; +import io.dropwizard.lifecycle.Managed; import jakarta.inject.Inject; import lombok.NonNull; import lombok.extern.slf4j.Slf4j; @@ -22,6 +23,7 @@ import org.redisson.client.codec.Codec; import org.slf4j.Logger; import org.slf4j.MDC; +import reactor.core.Disposable; import reactor.core.publisher.Flux; import reactor.core.publisher.Mono; import reactor.core.scheduler.Schedulers; @@ -29,8 +31,10 @@ import ru.vyarus.dropwizard.guice.module.yaml.bind.Config; import java.math.BigDecimal; +import java.time.Duration; import java.util.List; import java.util.Map; +import java.util.Optional; import java.util.UUID; import static com.comet.opik.api.AutomationRuleEvaluatorType.LLM_AS_JUDGE; @@ -41,10 +45,12 @@ /** * This service listens a Redis stream for Traces to be scored in a LLM provider. It will prepare the LLM request * by rendering message templates using values from the Trace and prepare the schema for the return (structured output). + * + * The service has to implement the Managed interface to be able to start and stop the stream connected to the application lifecycle. */ @EagerSingleton @Slf4j -public class OnlineScoringLlmAsJudgeScorer { +public class OnlineScoringLlmAsJudgeScorer implements Managed { private final OnlineScoringConfig config; private final ChatCompletionService aiProxyService; @@ -53,6 +59,10 @@ public class OnlineScoringLlmAsJudgeScorer { private final String consumerId; private final StreamReadGroupArgs redisReadConfig; private final Logger userFacingLogger; + private final RedissonReactiveClient redisson; + + private RStreamReactive stream; + private Disposable streamSubscription; // Store the subscription reference @Inject public OnlineScoringLlmAsJudgeScorer(@NonNull @Config("onlineScoring") OnlineScoringConfig config, @@ -62,29 +72,75 @@ public OnlineScoringLlmAsJudgeScorer(@NonNull @Config("onlineScoring") OnlineSco this.config = config; this.aiProxyService = aiProxyService; this.feedbackScoreService = feedbackScoreService; + this.redisson = redisson; this.redisReadConfig = StreamReadGroupArgs.neverDelivered().count(config.getConsumerBatchSize()); this.consumerId = "consumer-" + config.getConsumerGroupName() + "-" + UUID.randomUUID(); userFacingLogger = UserFacingLoggingFactory.getLogger(OnlineScoringLlmAsJudgeScorer.class); + } + @Override + public void start() { + log.info("OnlineScoringLlmAsJudgeScorer started."); // as we are a LLM consumer, lets check only LLM stream - initStream(config, redisson); + stream = initStream(config, redisson); } - private void initStream(OnlineScoringConfig config, RedissonReactiveClient redisson) { - config.getStreams().stream() + @Override + public void stop() { + log.info("Shutting down OnlineScoringLlmAsJudgeScorer and closing stream."); + if (stream != null) { + if (streamSubscription != null && !streamSubscription.isDisposed()) { + log.info("Waiting for last messages to be processed before shutdown..."); + + try { + // Read any remaining messages before stopping + stream.readGroup(config.getConsumerGroupName(), consumerId, redisReadConfig) + .flatMap(messages -> { + if (!messages.isEmpty()) { + log.info("Processing last {} messages before shutdown.", messages.size()); + + return Flux.fromIterable(messages.entrySet()) + .publishOn(Schedulers.boundedElastic()) + .doOnNext(entry -> processReceivedMessages(stream, entry)) + .collectList() + .then(Mono.fromRunnable(() -> streamSubscription.dispose())); + } + + return Mono.fromRunnable(() -> streamSubscription.dispose()); + }) + .block(Duration.ofSeconds(2)); + } catch (Exception e) { + log.error("Error processing last messages before shutdown: {}", e.getMessage(), e); + } + } else { + log.info("No active subscription, deleting Redis stream."); + } + + stream.delete().doOnTerminate(() -> log.info("Redis Stream deleted")).subscribe(); + } + } + + private RStreamReactive initStream(OnlineScoringConfig config, + RedissonReactiveClient redisson) { + Optional configuration = config.getStreams().stream() .filter(this::isLlmAsJudge) - .findFirst() - .ifPresentOrElse( - llmConfig -> setupListener(redisson, llmConfig), - this::logIfEmpty); + .findFirst(); + + if (configuration.isEmpty()) { + this.logIfEmpty(); + return null; + } + + return setupListener(redisson, configuration.get()); } private void logIfEmpty() { log.warn("No '{}' redis stream config found. Online Scoring consumer won't start.", LLM_AS_JUDGE.name()); } - private void setupListener(RedissonReactiveClient redisson, StreamConfiguration llmConfig) { + private RStreamReactive setupListener(RedissonReactiveClient redisson, + StreamConfiguration llmConfig) { var scoringCodecs = OnlineScoringCodecs.fromString(llmConfig.getCodec()); String streamName = llmConfig.getStreamName(); Codec codec = scoringCodecs.getCodec(); @@ -95,6 +151,8 @@ private void setupListener(RedissonReactiveClient redisson, StreamConfiguration enforceConsumerGroup(stream); setupStreamListener(stream); + + return stream; } private boolean isLlmAsJudge(StreamConfiguration streamConfiguration) { @@ -118,7 +176,7 @@ private void enforceConsumerGroup(RStreamReactive stream) { // Listen for messages - Flux.interval(config.getPoolingInterval().toJavaDuration()) + this.streamSubscription = Flux.interval(config.getPoolingInterval().toJavaDuration()) .flatMap(i -> stream.readGroup(config.getConsumerGroupName(), consumerId, redisReadConfig)) .flatMap(messages -> Flux.fromIterable(messages.entrySet())) .publishOn(Schedulers.boundedElastic()) diff --git a/apps/opik-backend/src/main/java/com/comet/opik/api/resources/v1/priv/ChatCompletionsResource.java b/apps/opik-backend/src/main/java/com/comet/opik/api/resources/v1/priv/ChatCompletionsResource.java index e9721a8ce2..0fa32da42d 100644 --- a/apps/opik-backend/src/main/java/com/comet/opik/api/resources/v1/priv/ChatCompletionsResource.java +++ b/apps/opik-backend/src/main/java/com/comet/opik/api/resources/v1/priv/ChatCompletionsResource.java @@ -1,7 +1,7 @@ package com.comet.opik.api.resources.v1.priv; import com.codahale.metrics.annotation.Timed; -import com.comet.opik.domain.ChatCompletionService; +import com.comet.opik.domain.llm.ChatCompletionService; import com.comet.opik.infrastructure.auth.RequestContext; import com.comet.opik.utils.ChunkedOutputHandlers; import dev.ai4j.openai4j.chat.ChatCompletionRequest; diff --git a/apps/opik-backend/src/main/java/com/comet/opik/domain/ChatCompletionService.java b/apps/opik-backend/src/main/java/com/comet/opik/domain/llm/ChatCompletionService.java similarity index 97% rename from apps/opik-backend/src/main/java/com/comet/opik/domain/ChatCompletionService.java rename to apps/opik-backend/src/main/java/com/comet/opik/domain/llm/ChatCompletionService.java index 8bda68aacc..4bc1912a82 100644 --- a/apps/opik-backend/src/main/java/com/comet/opik/domain/ChatCompletionService.java +++ b/apps/opik-backend/src/main/java/com/comet/opik/domain/llm/ChatCompletionService.java @@ -1,8 +1,6 @@ -package com.comet.opik.domain; +package com.comet.opik.domain.llm; import com.comet.opik.api.AutomationRuleEvaluatorLlmAsJudge; -import com.comet.opik.domain.llmproviders.LlmProviderFactory; -import com.comet.opik.domain.llmproviders.LlmProviderService; import com.comet.opik.infrastructure.LlmProviderClientConfig; import com.comet.opik.utils.ChunkedOutputHandlers; import dev.ai4j.openai4j.chat.ChatCompletionRequest; diff --git a/apps/opik-backend/src/main/java/com/comet/opik/domain/llm/LlmProviderFactory.java b/apps/opik-backend/src/main/java/com/comet/opik/domain/llm/LlmProviderFactory.java new file mode 100644 index 0000000000..f70aaab932 --- /dev/null +++ b/apps/opik-backend/src/main/java/com/comet/opik/domain/llm/LlmProviderFactory.java @@ -0,0 +1,19 @@ +package com.comet.opik.domain.llm; + +import com.comet.opik.api.LlmProvider; +import com.comet.opik.infrastructure.llm.LlmServiceProvider; +import dev.langchain4j.model.chat.ChatLanguageModel; + +import static com.comet.opik.api.AutomationRuleEvaluatorLlmAsJudge.LlmAsJudgeModelParameters; + +public interface LlmProviderFactory { + + String ERROR_MODEL_NOT_SUPPORTED = "model not supported %s"; + + void register(LlmProvider llmProvider, LlmServiceProvider service); + + LlmProviderService getService(String workspaceId, String model); + + ChatLanguageModel getLanguageModel(String workspaceId, LlmAsJudgeModelParameters modelParameters); + +} diff --git a/apps/opik-backend/src/main/java/com/comet/opik/domain/llmproviders/LlmProviderService.java b/apps/opik-backend/src/main/java/com/comet/opik/domain/llm/LlmProviderService.java similarity index 95% rename from apps/opik-backend/src/main/java/com/comet/opik/domain/llmproviders/LlmProviderService.java rename to apps/opik-backend/src/main/java/com/comet/opik/domain/llm/LlmProviderService.java index 83d9140f1d..3ef78b8c5a 100644 --- a/apps/opik-backend/src/main/java/com/comet/opik/domain/llmproviders/LlmProviderService.java +++ b/apps/opik-backend/src/main/java/com/comet/opik/domain/llm/LlmProviderService.java @@ -1,4 +1,4 @@ -package com.comet.opik.domain.llmproviders; +package com.comet.opik.domain.llm; import dev.ai4j.openai4j.chat.ChatCompletionRequest; import dev.ai4j.openai4j.chat.ChatCompletionResponse; diff --git a/apps/opik-backend/src/main/java/com/comet/opik/domain/llmproviders/LlmProviderClientModule.java b/apps/opik-backend/src/main/java/com/comet/opik/domain/llmproviders/LlmProviderClientModule.java deleted file mode 100644 index f6ea5a4cb8..0000000000 --- a/apps/opik-backend/src/main/java/com/comet/opik/domain/llmproviders/LlmProviderClientModule.java +++ /dev/null @@ -1,19 +0,0 @@ -package com.comet.opik.domain.llmproviders; - -import com.comet.opik.infrastructure.LlmProviderClientConfig; -import com.comet.opik.infrastructure.OpikConfiguration; -import com.google.inject.Provides; -import jakarta.inject.Singleton; -import lombok.NonNull; -import ru.vyarus.dropwizard.guice.module.support.DropwizardAwareModule; -import ru.vyarus.dropwizard.guice.module.yaml.bind.Config; - -public class LlmProviderClientModule extends DropwizardAwareModule { - - @Provides - @Singleton - public LlmProviderClientGenerator clientGenerator( - @NonNull @Config("llmProviderClient") LlmProviderClientConfig config) { - return new LlmProviderClientGenerator(config); - } -} diff --git a/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/LlmModule.java b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/LlmModule.java new file mode 100644 index 0000000000..4b769d93f8 --- /dev/null +++ b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/LlmModule.java @@ -0,0 +1,20 @@ +package com.comet.opik.infrastructure.llm; + +import com.comet.opik.domain.LlmProviderApiKeyService; +import com.comet.opik.domain.llm.LlmProviderFactory; +import com.google.inject.AbstractModule; +import com.google.inject.Provides; +import jakarta.inject.Singleton; + +public class LlmModule extends AbstractModule { + + @Provides + @Singleton + public LlmProviderFactory llmProviderFactory(LlmProviderApiKeyService llmProviderApiKeyService) { + return createInstance(llmProviderApiKeyService); + } + + public LlmProviderFactory createInstance(LlmProviderApiKeyService llmProviderApiKeyService) { + return new LlmProviderFactoryImpl(llmProviderApiKeyService); + } +} diff --git a/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/LlmProviderClientGenerator.java b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/LlmProviderClientGenerator.java new file mode 100644 index 0000000000..45a9e13a77 --- /dev/null +++ b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/LlmProviderClientGenerator.java @@ -0,0 +1,12 @@ +package com.comet.opik.infrastructure.llm; + +import dev.langchain4j.model.chat.ChatLanguageModel; + +import static com.comet.opik.api.AutomationRuleEvaluatorLlmAsJudge.LlmAsJudgeModelParameters; + +public interface LlmProviderClientGenerator { + + T generate(String apiKey, Object... args); + + ChatLanguageModel generateChat(String apiKey, LlmAsJudgeModelParameters modelParameters); +} diff --git a/apps/opik-backend/src/main/java/com/comet/opik/domain/llmproviders/LlmProviderFactory.java b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/LlmProviderFactoryImpl.java similarity index 64% rename from apps/opik-backend/src/main/java/com/comet/opik/domain/llmproviders/LlmProviderFactory.java rename to apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/LlmProviderFactoryImpl.java index 3b1d6f5d2f..99bfb3f633 100644 --- a/apps/opik-backend/src/main/java/com/comet/opik/domain/llmproviders/LlmProviderFactory.java +++ b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/LlmProviderFactoryImpl.java @@ -1,49 +1,58 @@ -package com.comet.opik.domain.llmproviders; +package com.comet.opik.infrastructure.llm; -import com.comet.opik.api.AutomationRuleEvaluatorLlmAsJudge; import com.comet.opik.api.LlmProvider; import com.comet.opik.domain.LlmProviderApiKeyService; +import com.comet.opik.domain.llm.LlmProviderFactory; +import com.comet.opik.domain.llm.LlmProviderService; import com.comet.opik.infrastructure.EncryptionUtils; +import com.comet.opik.infrastructure.llm.antropic.AnthropicModelName; +import com.comet.opik.infrastructure.llm.gemini.GeminiModelName; +import com.comet.opik.infrastructure.llm.openai.OpenaiModelName; import dev.langchain4j.model.chat.ChatLanguageModel; import jakarta.inject.Inject; -import jakarta.inject.Singleton; import jakarta.ws.rs.BadRequestException; import lombok.NonNull; import lombok.RequiredArgsConstructor; import org.apache.commons.lang3.EnumUtils; +import java.util.EnumMap; +import java.util.Map; +import java.util.Optional; import java.util.function.Function; -@Singleton +import static com.comet.opik.api.AutomationRuleEvaluatorLlmAsJudge.LlmAsJudgeModelParameters; + @RequiredArgsConstructor(onConstructor_ = @Inject) -public class LlmProviderFactory { - public static final String ERROR_MODEL_NOT_SUPPORTED = "model not supported %s"; +class LlmProviderFactoryImpl implements LlmProviderFactory { private final @NonNull LlmProviderApiKeyService llmProviderApiKeyService; - private final @NonNull LlmProviderClientGenerator llmProviderClientGenerator; + private final Map services = new EnumMap<>(LlmProvider.class); + + public void register(LlmProvider llmProvider, LlmServiceProvider service) { + services.put(llmProvider, service); + } public LlmProviderService getService(@NonNull String workspaceId, @NonNull String model) { var llmProvider = getLlmProvider(model); var apiKey = EncryptionUtils.decrypt(getEncryptedApiKey(workspaceId, llmProvider)); - return switch (llmProvider) { - case LlmProvider.OPEN_AI -> new LlmProviderOpenAi(llmProviderClientGenerator.newOpenAiClient(apiKey)); - case LlmProvider.ANTHROPIC -> - new LlmProviderAnthropic(llmProviderClientGenerator.newAnthropicClient(apiKey)); - case LlmProvider.GEMINI -> new LlmProviderGemini(llmProviderClientGenerator, apiKey); - }; + return Optional.ofNullable(services.get(llmProvider)) + .map(provider -> provider.getService(apiKey)) + .orElseThrow(() -> new LlmProviderUnsupportedException( + "LLM provider not supported: %s".formatted(llmProvider))); } public ChatLanguageModel getLanguageModel(@NonNull String workspaceId, - @NonNull AutomationRuleEvaluatorLlmAsJudge.LlmAsJudgeModelParameters modelParameters) { + @NonNull LlmAsJudgeModelParameters modelParameters) { var llmProvider = getLlmProvider(modelParameters.name()); var apiKey = EncryptionUtils.decrypt(getEncryptedApiKey(workspaceId, llmProvider)); - return switch (llmProvider) { - case LlmProvider.OPEN_AI -> llmProviderClientGenerator.newOpenAiChatLanguageModel(apiKey, modelParameters); - default -> throw new BadRequestException(String.format(ERROR_MODEL_NOT_SUPPORTED, modelParameters.name())); - }; + return Optional.ofNullable(services.get(llmProvider)) + .map(provider -> provider.getLanguageModel(apiKey, modelParameters)) + .orElseThrow(() -> new BadRequestException( + String.format(ERROR_MODEL_NOT_SUPPORTED, modelParameters.name()))); } + /** * The agreed requirement is to resolve the LLM provider and its API key based on the model. */ diff --git a/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/LlmProviderUnsupportedException.java b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/LlmProviderUnsupportedException.java new file mode 100644 index 0000000000..f83d32c443 --- /dev/null +++ b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/LlmProviderUnsupportedException.java @@ -0,0 +1,15 @@ +package com.comet.opik.infrastructure.llm; + +import io.dropwizard.jersey.errors.ErrorMessage; +import jakarta.ws.rs.ClientErrorException; +import jakarta.ws.rs.core.Response; + +public class LlmProviderUnsupportedException extends ClientErrorException { + + public LlmProviderUnsupportedException(String message) { + super( + Response.status(Response.Status.CONFLICT) + .entity(new ErrorMessage(Response.Status.CONFLICT.getStatusCode(), message)) + .build()); + } +} diff --git a/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/LlmServiceProvider.java b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/LlmServiceProvider.java new file mode 100644 index 0000000000..085ec5ee8a --- /dev/null +++ b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/LlmServiceProvider.java @@ -0,0 +1,13 @@ +package com.comet.opik.infrastructure.llm; + +import com.comet.opik.domain.llm.LlmProviderService; +import dev.langchain4j.model.chat.ChatLanguageModel; + +import static com.comet.opik.api.AutomationRuleEvaluatorLlmAsJudge.LlmAsJudgeModelParameters; + +public interface LlmServiceProvider { + + LlmProviderService getService(String apiKey); + + ChatLanguageModel getLanguageModel(String apiKey, LlmAsJudgeModelParameters modelParameters); +} diff --git a/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/antropic/AnthropicClientGenerator.java b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/antropic/AnthropicClientGenerator.java new file mode 100644 index 0000000000..b29dc2ba4b --- /dev/null +++ b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/antropic/AnthropicClientGenerator.java @@ -0,0 +1,70 @@ +package com.comet.opik.infrastructure.llm.antropic; + +import com.comet.opik.infrastructure.LlmProviderClientConfig; +import com.comet.opik.infrastructure.llm.LlmProviderClientGenerator; +import dev.langchain4j.model.anthropic.AnthropicChatModel; +import dev.langchain4j.model.anthropic.internal.client.AnthropicClient; +import dev.langchain4j.model.chat.ChatLanguageModel; +import lombok.NonNull; +import lombok.RequiredArgsConstructor; +import org.apache.commons.lang3.StringUtils; + +import java.util.Optional; + +import static com.comet.opik.api.AutomationRuleEvaluatorLlmAsJudge.LlmAsJudgeModelParameters; + +@RequiredArgsConstructor +public class AnthropicClientGenerator implements LlmProviderClientGenerator { + + private final @NonNull LlmProviderClientConfig llmProviderClientConfig; + + private AnthropicClient newAnthropicClient(@NonNull String apiKey) { + var anthropicClientBuilder = AnthropicClient.builder(); + Optional.ofNullable(llmProviderClientConfig.getAnthropicClient()) + .map(LlmProviderClientConfig.AnthropicClientConfig::url) + .filter(StringUtils::isNotEmpty) + .ifPresent(anthropicClientBuilder::baseUrl); + Optional.ofNullable(llmProviderClientConfig.getAnthropicClient()) + .map(LlmProviderClientConfig.AnthropicClientConfig::version) + .filter(StringUtils::isNotBlank) + .ifPresent(anthropicClientBuilder::version); + Optional.ofNullable(llmProviderClientConfig.getLogRequests()) + .ifPresent(anthropicClientBuilder::logRequests); + Optional.ofNullable(llmProviderClientConfig.getLogResponses()) + .ifPresent(anthropicClientBuilder::logResponses); + // anthropic client builder only receives one timeout variant + Optional.ofNullable(llmProviderClientConfig.getCallTimeout()) + .ifPresent(callTimeout -> anthropicClientBuilder.timeout(callTimeout.toJavaDuration())); + return anthropicClientBuilder + .apiKey(apiKey) + .build(); + } + + private ChatLanguageModel newChatLanguageModel(String apiKey, LlmAsJudgeModelParameters modelParameters) { + var builder = AnthropicChatModel.builder() + .apiKey(apiKey) + .modelName(modelParameters.name()); + + Optional.ofNullable(llmProviderClientConfig.getConnectTimeout()) + .ifPresent(connectTimeout -> builder.timeout(connectTimeout.toJavaDuration())); + + Optional.ofNullable(llmProviderClientConfig.getOpenAiClient()) + .map(LlmProviderClientConfig.OpenAiClientConfig::url) + .filter(StringUtils::isNotBlank) + .ifPresent(builder::baseUrl); + + Optional.ofNullable(modelParameters.temperature()).ifPresent(builder::temperature); + + return builder.build(); + } + + @Override + public AnthropicClient generate(@NonNull String apiKey, Object... args) { + return newAnthropicClient(apiKey); + } + + @Override + public ChatLanguageModel generateChat(@NonNull String apiKey, @NonNull LlmAsJudgeModelParameters modelParameters) { + return newChatLanguageModel(apiKey, modelParameters); + } +} diff --git a/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/antropic/AnthropicLlmServiceProvider.java b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/antropic/AnthropicLlmServiceProvider.java new file mode 100644 index 0000000000..0e9b9d53c2 --- /dev/null +++ b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/antropic/AnthropicLlmServiceProvider.java @@ -0,0 +1,34 @@ +package com.comet.opik.infrastructure.llm.antropic; + +import com.comet.opik.api.AutomationRuleEvaluatorLlmAsJudge.LlmAsJudgeModelParameters; +import com.comet.opik.api.LlmProvider; +import com.comet.opik.domain.llm.LlmProviderFactory; +import com.comet.opik.domain.llm.LlmProviderService; +import com.comet.opik.infrastructure.llm.LlmServiceProvider; +import dev.langchain4j.model.chat.ChatLanguageModel; +import lombok.NonNull; +import lombok.RequiredArgsConstructor; + +@RequiredArgsConstructor +class AnthropicLlmServiceProvider implements LlmServiceProvider { + + private final AnthropicClientGenerator clientGenerator; + + AnthropicLlmServiceProvider(@NonNull AnthropicClientGenerator clientGenerator, + @NonNull LlmProviderFactory factory) { + this.clientGenerator = clientGenerator; + factory.register(LlmProvider.ANTHROPIC, this); + } + + @Override + public LlmProviderService getService(String apiKey) { + return new LlmProviderAnthropic(clientGenerator.generate(apiKey)); + } + + @Override + public ChatLanguageModel getLanguageModel(@NonNull String apiKey, + @NonNull LlmAsJudgeModelParameters modelParameters) { + return clientGenerator.generateChat(apiKey, modelParameters); + } + +} diff --git a/apps/opik-backend/src/main/java/com/comet/opik/domain/llmproviders/AnthropicModelName.java b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/antropic/AnthropicModelName.java similarity index 94% rename from apps/opik-backend/src/main/java/com/comet/opik/domain/llmproviders/AnthropicModelName.java rename to apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/antropic/AnthropicModelName.java index 25759662bc..5ef415c3e8 100644 --- a/apps/opik-backend/src/main/java/com/comet/opik/domain/llmproviders/AnthropicModelName.java +++ b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/antropic/AnthropicModelName.java @@ -1,4 +1,4 @@ -package com.comet.opik.domain.llmproviders; +package com.comet.opik.infrastructure.llm.antropic; import lombok.RequiredArgsConstructor; diff --git a/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/antropic/AnthropicModule.java b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/antropic/AnthropicModule.java new file mode 100644 index 0000000000..0c87397db6 --- /dev/null +++ b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/antropic/AnthropicModule.java @@ -0,0 +1,30 @@ +package com.comet.opik.infrastructure.llm.antropic; + +import com.comet.opik.domain.llm.LlmProviderFactory; +import com.comet.opik.infrastructure.LlmProviderClientConfig; +import com.comet.opik.infrastructure.llm.LlmServiceProvider; +import com.google.inject.AbstractModule; +import com.google.inject.Provides; +import com.google.inject.Singleton; +import jakarta.inject.Named; +import lombok.NonNull; +import ru.vyarus.dropwizard.guice.module.yaml.bind.Config; + +public class AnthropicModule extends AbstractModule { + + @Provides + @Singleton + public AnthropicClientGenerator clientGenerator( + @NonNull @Config("llmProviderClient") LlmProviderClientConfig config) { + return new AnthropicClientGenerator(config); + } + + @Provides + @Singleton + @Named("anthropic") + public LlmServiceProvider llmServiceProvider(@NonNull LlmProviderFactory llmProviderFactory, + @NonNull AnthropicClientGenerator clientGenerator) { + return new AnthropicLlmServiceProvider(clientGenerator, llmProviderFactory); + } + +} diff --git a/apps/opik-backend/src/main/java/com/comet/opik/domain/llmproviders/LlmProviderAnthropic.java b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/antropic/LlmProviderAnthropic.java similarity index 77% rename from apps/opik-backend/src/main/java/com/comet/opik/domain/llmproviders/LlmProviderAnthropic.java rename to apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/antropic/LlmProviderAnthropic.java index 511cd250a9..80ea278948 100644 --- a/apps/opik-backend/src/main/java/com/comet/opik/domain/llmproviders/LlmProviderAnthropic.java +++ b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/antropic/LlmProviderAnthropic.java @@ -1,5 +1,7 @@ -package com.comet.opik.domain.llmproviders; +package com.comet.opik.infrastructure.llm.antropic; +import com.comet.opik.api.ChunkedResponseHandler; +import com.comet.opik.domain.llm.LlmProviderService; import dev.ai4j.openai4j.chat.ChatCompletionRequest; import dev.ai4j.openai4j.chat.ChatCompletionResponse; import dev.langchain4j.model.anthropic.internal.client.AnthropicClient; @@ -14,20 +16,21 @@ import java.util.Optional; import java.util.function.Consumer; -import static com.comet.opik.domain.ChatCompletionService.ERROR_EMPTY_MESSAGES; -import static com.comet.opik.domain.ChatCompletionService.ERROR_NO_COMPLETION_TOKENS; +import static com.comet.opik.domain.llm.ChatCompletionService.ERROR_EMPTY_MESSAGES; +import static com.comet.opik.domain.llm.ChatCompletionService.ERROR_NO_COMPLETION_TOKENS; @RequiredArgsConstructor @Slf4j class LlmProviderAnthropic implements LlmProviderService { + private final @NonNull AnthropicClient anthropicClient; @Override public ChatCompletionResponse generate(@NonNull ChatCompletionRequest request, @NonNull String workspaceId) { - var mapper = LlmProviderAnthropicMapper.INSTANCE; - var response = anthropicClient.createMessage(mapper.toCreateMessageRequest(request)); + var response = anthropicClient + .createMessage(LlmProviderAnthropicMapper.INSTANCE.toCreateMessageRequest(request)); - return mapper.toResponse(response); + return LlmProviderAnthropicMapper.INSTANCE.toResponse(response); } @Override @@ -35,7 +38,8 @@ public void generateStream( @NonNull ChatCompletionRequest request, @NonNull String workspaceId, @NonNull Consumer handleMessage, - @NonNull Runnable handleClose, @NonNull Consumer handleError) { + @NonNull Runnable handleClose, + @NonNull Consumer handleError) { validateRequest(request); anthropicClient.createMessage(LlmProviderAnthropicMapper.INSTANCE.toCreateMessageRequest(request), new ChunkedResponseHandler(handleMessage, handleClose, handleError, request.model())); diff --git a/apps/opik-backend/src/main/java/com/comet/opik/domain/llmproviders/LlmProviderAnthropicMapper.java b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/antropic/LlmProviderAnthropicMapper.java similarity index 98% rename from apps/opik-backend/src/main/java/com/comet/opik/domain/llmproviders/LlmProviderAnthropicMapper.java rename to apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/antropic/LlmProviderAnthropicMapper.java index dcce42e8a0..7f3fad6770 100644 --- a/apps/opik-backend/src/main/java/com/comet/opik/domain/llmproviders/LlmProviderAnthropicMapper.java +++ b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/antropic/LlmProviderAnthropicMapper.java @@ -1,4 +1,4 @@ -package com.comet.opik.domain.llmproviders; +package com.comet.opik.infrastructure.llm.antropic; import dev.ai4j.openai4j.chat.AssistantMessage; import dev.ai4j.openai4j.chat.ChatCompletionChoice; @@ -27,7 +27,7 @@ import java.util.List; @Mapper -public interface LlmProviderAnthropicMapper { +interface LlmProviderAnthropicMapper { LlmProviderAnthropicMapper INSTANCE = Mappers.getMapper(LlmProviderAnthropicMapper.class); @Mapping(source = "response", target = "choices", qualifiedByName = "mapToChoices") diff --git a/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/gemini/GeminiClientGenerator.java b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/gemini/GeminiClientGenerator.java new file mode 100644 index 0000000000..497a8e825c --- /dev/null +++ b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/gemini/GeminiClientGenerator.java @@ -0,0 +1,55 @@ +package com.comet.opik.infrastructure.llm.gemini; + +import com.comet.opik.infrastructure.LlmProviderClientConfig; +import com.comet.opik.infrastructure.llm.LlmProviderClientGenerator; +import dev.ai4j.openai4j.chat.ChatCompletionRequest; +import dev.langchain4j.model.chat.ChatLanguageModel; +import dev.langchain4j.model.googleai.GoogleAiGeminiChatModel; +import dev.langchain4j.model.googleai.GoogleAiGeminiStreamingChatModel; +import lombok.NonNull; +import lombok.RequiredArgsConstructor; + +import java.util.Objects; +import java.util.Optional; + +import static com.comet.opik.api.AutomationRuleEvaluatorLlmAsJudge.LlmAsJudgeModelParameters; +import static dev.langchain4j.model.googleai.GoogleAiGeminiChatModel.GoogleAiGeminiChatModelBuilder; + +@RequiredArgsConstructor +public class GeminiClientGenerator implements LlmProviderClientGenerator { + + private static final int MAX_RETRIES = 1; + private final @NonNull LlmProviderClientConfig llmProviderClientConfig; + + public GoogleAiGeminiChatModel newGeminiClient(@NonNull String apiKey, @NonNull ChatCompletionRequest request) { + return LlmProviderGeminiMapper.INSTANCE.toGeminiChatModel(apiKey, request, + llmProviderClientConfig.getCallTimeout().toJavaDuration(), MAX_RETRIES); + } + + public GoogleAiGeminiStreamingChatModel newGeminiStreamingClient( + @NonNull String apiKey, @NonNull ChatCompletionRequest request) { + return LlmProviderGeminiMapper.INSTANCE.toGeminiStreamingChatModel(apiKey, request, + llmProviderClientConfig.getCallTimeout().toJavaDuration(), MAX_RETRIES); + } + + @Override + public GoogleAiGeminiChatModel generate(String apiKey, Object... args) { + ChatCompletionRequest request = (ChatCompletionRequest) Objects.requireNonNull(args[0], + "ChatCompletionRequest is required"); + return newGeminiClient(apiKey, request); + } + + @Override + public ChatLanguageModel generateChat(String apiKey, LlmAsJudgeModelParameters modelParameters) { + GoogleAiGeminiChatModelBuilder modelBuilder = GoogleAiGeminiChatModel.builder() + .modelName(modelParameters.name()) + .apiKey(apiKey); + + Optional.ofNullable(llmProviderClientConfig.getConnectTimeout()) + .ifPresent(connectTimeout -> modelBuilder.timeout(connectTimeout.toJavaDuration())); + + Optional.ofNullable(modelParameters.temperature()).ifPresent(modelBuilder::temperature); + + return modelBuilder.build(); + } +} diff --git a/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/gemini/GeminiLlmServiceProvider.java b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/gemini/GeminiLlmServiceProvider.java new file mode 100644 index 0000000000..6dba7bb4ec --- /dev/null +++ b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/gemini/GeminiLlmServiceProvider.java @@ -0,0 +1,29 @@ +package com.comet.opik.infrastructure.llm.gemini; + +import com.comet.opik.api.AutomationRuleEvaluatorLlmAsJudge; +import com.comet.opik.api.LlmProvider; +import com.comet.opik.domain.llm.LlmProviderFactory; +import com.comet.opik.domain.llm.LlmProviderService; +import com.comet.opik.infrastructure.llm.LlmServiceProvider; +import dev.langchain4j.model.chat.ChatLanguageModel; + +public class GeminiLlmServiceProvider implements LlmServiceProvider { + + private final GeminiClientGenerator clientGenerator; + + GeminiLlmServiceProvider(GeminiClientGenerator clientGenerator, LlmProviderFactory factory) { + this.clientGenerator = clientGenerator; + factory.register(LlmProvider.GEMINI, this); + } + + @Override + public LlmProviderService getService(String apiKey) { + return new LlmProviderGemini(clientGenerator, apiKey); + } + + @Override + public ChatLanguageModel getLanguageModel(String apiKey, + AutomationRuleEvaluatorLlmAsJudge.LlmAsJudgeModelParameters modelParameters) { + return clientGenerator.generateChat(apiKey, modelParameters); + } +} diff --git a/apps/opik-backend/src/main/java/com/comet/opik/domain/llmproviders/GeminiModelName.java b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/gemini/GeminiModelName.java similarity index 92% rename from apps/opik-backend/src/main/java/com/comet/opik/domain/llmproviders/GeminiModelName.java rename to apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/gemini/GeminiModelName.java index a7a83d160f..48ccf36fdc 100644 --- a/apps/opik-backend/src/main/java/com/comet/opik/domain/llmproviders/GeminiModelName.java +++ b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/gemini/GeminiModelName.java @@ -1,4 +1,4 @@ -package com.comet.opik.domain.llmproviders; +package com.comet.opik.infrastructure.llm.gemini; import lombok.RequiredArgsConstructor; diff --git a/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/gemini/GeminiModule.java b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/gemini/GeminiModule.java new file mode 100644 index 0000000000..89ced9add7 --- /dev/null +++ b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/gemini/GeminiModule.java @@ -0,0 +1,28 @@ +package com.comet.opik.infrastructure.llm.gemini; + +import com.comet.opik.domain.llm.LlmProviderFactory; +import com.comet.opik.infrastructure.LlmProviderClientConfig; +import com.comet.opik.infrastructure.llm.LlmServiceProvider; +import com.google.inject.AbstractModule; +import com.google.inject.Provides; +import com.google.inject.Singleton; +import jakarta.inject.Named; +import lombok.NonNull; +import ru.vyarus.dropwizard.guice.module.yaml.bind.Config; + +public class GeminiModule extends AbstractModule { + + @Provides + @Singleton + public GeminiClientGenerator clientGenerator(@NonNull @Config("llmProviderClient") LlmProviderClientConfig config) { + return new GeminiClientGenerator(config); + } + + @Provides + @Singleton + @Named("gemini") + public LlmServiceProvider llmServiceProvider(@NonNull LlmProviderFactory llmProviderFactory, + @NonNull GeminiClientGenerator clientGenerator) { + return new GeminiLlmServiceProvider(clientGenerator, llmProviderFactory); + } +} diff --git a/apps/opik-backend/src/main/java/com/comet/opik/domain/llmproviders/LlmProviderGemini.java b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/gemini/LlmProviderGemini.java similarity index 87% rename from apps/opik-backend/src/main/java/com/comet/opik/domain/llmproviders/LlmProviderGemini.java rename to apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/gemini/LlmProviderGemini.java index 3ae0c2efa9..f3abd047e1 100644 --- a/apps/opik-backend/src/main/java/com/comet/opik/domain/llmproviders/LlmProviderGemini.java +++ b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/gemini/LlmProviderGemini.java @@ -1,5 +1,7 @@ -package com.comet.opik.domain.llmproviders; +package com.comet.opik.infrastructure.llm.gemini; +import com.comet.opik.api.ChunkedResponseHandler; +import com.comet.opik.domain.llm.LlmProviderService; import dev.ai4j.openai4j.chat.ChatCompletionRequest; import dev.ai4j.openai4j.chat.ChatCompletionResponse; import io.dropwizard.jersey.errors.ErrorMessage; @@ -11,7 +13,7 @@ @RequiredArgsConstructor public class LlmProviderGemini implements LlmProviderService { - private final @NonNull LlmProviderClientGenerator llmProviderClientGenerator; + private final @NonNull GeminiClientGenerator llmProviderClientGenerator; private final @NonNull String apiKey; @Override diff --git a/apps/opik-backend/src/main/java/com/comet/opik/domain/llmproviders/LlmProviderGeminiMapper.java b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/gemini/LlmProviderGeminiMapper.java similarity index 98% rename from apps/opik-backend/src/main/java/com/comet/opik/domain/llmproviders/LlmProviderGeminiMapper.java rename to apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/gemini/LlmProviderGeminiMapper.java index 65fc410a1c..a5fdfc0560 100644 --- a/apps/opik-backend/src/main/java/com/comet/opik/domain/llmproviders/LlmProviderGeminiMapper.java +++ b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/gemini/LlmProviderGeminiMapper.java @@ -1,4 +1,4 @@ -package com.comet.opik.domain.llmproviders; +package com.comet.opik.infrastructure.llm.gemini; import dev.ai4j.openai4j.chat.AssistantMessage; import dev.ai4j.openai4j.chat.ChatCompletionChoice; @@ -25,7 +25,7 @@ import java.util.List; @Mapper -public interface LlmProviderGeminiMapper { +interface LlmProviderGeminiMapper { String ERR_UNEXPECTED_ROLE = "unexpected role '%s'"; String ERR_ROLE_MSG_TYPE_MISMATCH = "role and message instance are not matching, role: '%s', instance: '%s'"; diff --git a/apps/opik-backend/src/main/java/com/comet/opik/domain/llmproviders/LlmProviderOpenAi.java b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/openai/LlmProviderOpenAi.java similarity index 94% rename from apps/opik-backend/src/main/java/com/comet/opik/domain/llmproviders/LlmProviderOpenAi.java rename to apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/openai/LlmProviderOpenAi.java index 60c902d97a..d1eb422c3e 100644 --- a/apps/opik-backend/src/main/java/com/comet/opik/domain/llmproviders/LlmProviderOpenAi.java +++ b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/openai/LlmProviderOpenAi.java @@ -1,5 +1,6 @@ -package com.comet.opik.domain.llmproviders; +package com.comet.opik.infrastructure.llm.openai; +import com.comet.opik.domain.llm.LlmProviderService; import dev.ai4j.openai4j.OpenAiClient; import dev.ai4j.openai4j.OpenAiHttpException; import dev.ai4j.openai4j.chat.ChatCompletionRequest; diff --git a/apps/opik-backend/src/main/java/com/comet/opik/domain/llmproviders/LlmProviderClientGenerator.java b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/openai/OpenAIClientGenerator.java similarity index 50% rename from apps/opik-backend/src/main/java/com/comet/opik/domain/llmproviders/LlmProviderClientGenerator.java rename to apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/openai/OpenAIClientGenerator.java index 6671e534c1..94f742b230 100644 --- a/apps/opik-backend/src/main/java/com/comet/opik/domain/llmproviders/LlmProviderClientGenerator.java +++ b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/openai/OpenAIClientGenerator.java @@ -1,13 +1,9 @@ -package com.comet.opik.domain.llmproviders; +package com.comet.opik.infrastructure.llm.openai; -import com.comet.opik.api.AutomationRuleEvaluatorLlmAsJudge; import com.comet.opik.infrastructure.LlmProviderClientConfig; +import com.comet.opik.infrastructure.llm.LlmProviderClientGenerator; import dev.ai4j.openai4j.OpenAiClient; -import dev.ai4j.openai4j.chat.ChatCompletionRequest; -import dev.langchain4j.model.anthropic.internal.client.AnthropicClient; import dev.langchain4j.model.chat.ChatLanguageModel; -import dev.langchain4j.model.googleai.GoogleAiGeminiChatModel; -import dev.langchain4j.model.googleai.GoogleAiGeminiStreamingChatModel; import dev.langchain4j.model.openai.OpenAiChatModel; import lombok.NonNull; import lombok.RequiredArgsConstructor; @@ -15,34 +11,13 @@ import java.util.Optional; +import static com.comet.opik.api.AutomationRuleEvaluatorLlmAsJudge.LlmAsJudgeModelParameters; + @RequiredArgsConstructor -public class LlmProviderClientGenerator { - private static final int MAX_RETRIES = 1; +public class OpenAIClientGenerator implements LlmProviderClientGenerator { private final @NonNull LlmProviderClientConfig llmProviderClientConfig; - public AnthropicClient newAnthropicClient(@NonNull String apiKey) { - var anthropicClientBuilder = AnthropicClient.builder(); - Optional.ofNullable(llmProviderClientConfig.getAnthropicClient()) - .map(LlmProviderClientConfig.AnthropicClientConfig::url) - .filter(StringUtils::isNotEmpty) - .ifPresent(anthropicClientBuilder::baseUrl); - Optional.ofNullable(llmProviderClientConfig.getAnthropicClient()) - .map(LlmProviderClientConfig.AnthropicClientConfig::version) - .filter(StringUtils::isNotBlank) - .ifPresent(anthropicClientBuilder::version); - Optional.ofNullable(llmProviderClientConfig.getLogRequests()) - .ifPresent(anthropicClientBuilder::logRequests); - Optional.ofNullable(llmProviderClientConfig.getLogResponses()) - .ifPresent(anthropicClientBuilder::logResponses); - // anthropic client builder only receives one timeout variant - Optional.ofNullable(llmProviderClientConfig.getCallTimeout()) - .ifPresent(callTimeout -> anthropicClientBuilder.timeout(callTimeout.toJavaDuration())); - return anthropicClientBuilder - .apiKey(apiKey) - .build(); - } - public OpenAiClient newOpenAiClient(@NonNull String apiKey) { var openAiClientBuilder = OpenAiClient.builder(); Optional.ofNullable(llmProviderClientConfig.getOpenAiClient()) @@ -62,19 +37,7 @@ public OpenAiClient newOpenAiClient(@NonNull String apiKey) { .build(); } - public GoogleAiGeminiChatModel newGeminiClient(@NonNull String apiKey, @NonNull ChatCompletionRequest request) { - return LlmProviderGeminiMapper.INSTANCE.toGeminiChatModel(apiKey, request, - llmProviderClientConfig.getCallTimeout().toJavaDuration(), MAX_RETRIES); - } - - public GoogleAiGeminiStreamingChatModel newGeminiStreamingClient( - @NonNull String apiKey, @NonNull ChatCompletionRequest request) { - return LlmProviderGeminiMapper.INSTANCE.toGeminiStreamingChatModel(apiKey, request, - llmProviderClientConfig.getCallTimeout().toJavaDuration(), MAX_RETRIES); - } - - public ChatLanguageModel newOpenAiChatLanguageModel(String apiKey, - AutomationRuleEvaluatorLlmAsJudge.@NonNull LlmAsJudgeModelParameters modelParameters) { + public ChatLanguageModel newOpenAiChatLanguageModel(String apiKey, LlmAsJudgeModelParameters modelParameters) { var builder = OpenAiChatModel.builder() .modelName(modelParameters.name()) .apiKey(apiKey) @@ -93,4 +56,14 @@ public ChatLanguageModel newOpenAiChatLanguageModel(String apiKey, return builder.build(); } + + @Override + public OpenAiClient generate(@NonNull String apiKey, Object... args) { + return newOpenAiClient(apiKey); + } + + @Override + public ChatLanguageModel generateChat(@NonNull String apiKey, @NonNull LlmAsJudgeModelParameters modelParameters) { + return newOpenAiChatLanguageModel(apiKey, modelParameters); + } } diff --git a/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/openai/OpenAILlmServiceProvider.java b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/openai/OpenAILlmServiceProvider.java new file mode 100644 index 0000000000..b5e0b810e2 --- /dev/null +++ b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/openai/OpenAILlmServiceProvider.java @@ -0,0 +1,29 @@ +package com.comet.opik.infrastructure.llm.openai; + +import com.comet.opik.api.AutomationRuleEvaluatorLlmAsJudge; +import com.comet.opik.api.LlmProvider; +import com.comet.opik.domain.llm.LlmProviderFactory; +import com.comet.opik.domain.llm.LlmProviderService; +import com.comet.opik.infrastructure.llm.LlmServiceProvider; +import dev.langchain4j.model.chat.ChatLanguageModel; + +class OpenAILlmServiceProvider implements LlmServiceProvider { + + private final OpenAIClientGenerator clientGenerator; + + OpenAILlmServiceProvider(OpenAIClientGenerator clientGenerator, LlmProviderFactory factory) { + this.clientGenerator = clientGenerator; + factory.register(LlmProvider.OPEN_AI, this); + } + + @Override + public LlmProviderService getService(String apiKey) { + return new LlmProviderOpenAi(clientGenerator.newOpenAiClient(apiKey)); + } + + @Override + public ChatLanguageModel getLanguageModel(String apiKey, + AutomationRuleEvaluatorLlmAsJudge.LlmAsJudgeModelParameters modelParameters) { + return clientGenerator.newOpenAiChatLanguageModel(apiKey, modelParameters); + } +} diff --git a/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/openai/OpenAIModule.java b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/openai/OpenAIModule.java new file mode 100644 index 0000000000..50e3043993 --- /dev/null +++ b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/openai/OpenAIModule.java @@ -0,0 +1,28 @@ +package com.comet.opik.infrastructure.llm.openai; + +import com.comet.opik.domain.llm.LlmProviderFactory; +import com.comet.opik.infrastructure.LlmProviderClientConfig; +import com.comet.opik.infrastructure.llm.LlmServiceProvider; +import com.google.inject.AbstractModule; +import com.google.inject.Provides; +import com.google.inject.Singleton; +import jakarta.inject.Named; +import lombok.NonNull; +import ru.vyarus.dropwizard.guice.module.yaml.bind.Config; + +public class OpenAIModule extends AbstractModule { + + @Provides + @Singleton + public OpenAIClientGenerator clientGenerator(@NonNull @Config("llmProviderClient") LlmProviderClientConfig config) { + return new OpenAIClientGenerator(config); + } + + @Provides + @Singleton + @Named("openai") + public LlmServiceProvider llmServiceProvider(@NonNull LlmProviderFactory llmProviderFactory, + @NonNull OpenAIClientGenerator clientGenerator) { + return new OpenAILlmServiceProvider(clientGenerator, llmProviderFactory); + } +} diff --git a/apps/opik-backend/src/main/java/com/comet/opik/domain/llmproviders/OpenaiModelName.java b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/openai/OpenaiModelName.java similarity index 96% rename from apps/opik-backend/src/main/java/com/comet/opik/domain/llmproviders/OpenaiModelName.java rename to apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/openai/OpenaiModelName.java index c014c40ced..2b57219ed9 100644 --- a/apps/opik-backend/src/main/java/com/comet/opik/domain/llmproviders/OpenaiModelName.java +++ b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/openai/OpenaiModelName.java @@ -1,4 +1,4 @@ -package com.comet.opik.domain.llmproviders; +package com.comet.opik.infrastructure.llm.openai; import lombok.RequiredArgsConstructor; diff --git a/apps/opik-backend/src/test/java/com/comet/opik/api/resources/utils/TestDropwizardAppExtensionUtils.java b/apps/opik-backend/src/test/java/com/comet/opik/api/resources/utils/TestDropwizardAppExtensionUtils.java index 24de1f7737..5c51ce6a0c 100644 --- a/apps/opik-backend/src/test/java/com/comet/opik/api/resources/utils/TestDropwizardAppExtensionUtils.java +++ b/apps/opik-backend/src/test/java/com/comet/opik/api/resources/utils/TestDropwizardAppExtensionUtils.java @@ -6,6 +6,7 @@ import com.github.tomakehurst.wiremock.junit5.WireMockRuntimeInfo; import com.google.common.eventbus.EventBus; import com.google.inject.AbstractModule; +import com.google.inject.Module; import lombok.Builder; import lombok.experimental.UtilityClass; import org.apache.commons.collections4.CollectionUtils; @@ -48,6 +49,7 @@ public record AppContextConfig( EventBus mockEventBus, boolean corsEnabled, List customConfigs, + List> disableModules, List modules) { } @@ -129,9 +131,9 @@ public static TestDropwizardAppExtension newTestDropwizardAppExtension(AppContex GuiceyConfigurationHook hook = injector -> { injector.modulesOverride(TestHttpClientUtils.testAuthModule()); - Optional.ofNullable(appContextConfig.modules) + Optional.ofNullable(appContextConfig.disableModules) .orElse(List.of()) - .forEach(injector::modulesOverride); + .forEach(injector::disableModules); if (appContextConfig.mockEventBus() != null) { injector.modulesOverride(new EventModule() { @@ -154,6 +156,9 @@ public void run(GuiceyEnvironment environment) { } }); + Optional.ofNullable(appContextConfig.modules) + .orElse(List.of()) + .forEach(injector::modulesOverride); }; if (appContextConfig.redisUrl() != null) { diff --git a/apps/opik-backend/src/test/java/com/comet/opik/api/resources/utils/resources/AutomationRuleEvaluatorResourceClient.java b/apps/opik-backend/src/test/java/com/comet/opik/api/resources/utils/resources/AutomationRuleEvaluatorResourceClient.java index bb3b181695..5dea375edc 100644 --- a/apps/opik-backend/src/test/java/com/comet/opik/api/resources/utils/resources/AutomationRuleEvaluatorResourceClient.java +++ b/apps/opik-backend/src/test/java/com/comet/opik/api/resources/utils/resources/AutomationRuleEvaluatorResourceClient.java @@ -2,6 +2,7 @@ import com.comet.opik.api.AutomationRuleEvaluator; import com.comet.opik.api.AutomationRuleEvaluatorUpdate; +import com.comet.opik.api.LogItem.LogPage; import com.comet.opik.api.resources.utils.TestHttpClientUtils; import com.comet.opik.api.resources.utils.TestUtils; import jakarta.ws.rs.HttpMethod; @@ -71,10 +72,11 @@ public void updateEvaluator(UUID evaluatorId, UUID projectId, String workspaceNa } } - public AutomationRuleEvaluator getEvaluator(UUID evaluatorId, UUID projectId, String workspaceName, + public LogPage getEvaluatorLogs(UUID evaluatorId, UUID projectId, String workspaceName, String apiKey) { try (var actualResponse = client.target(RESOURCE_PATH.formatted(baseURI, projectId)) .path(evaluatorId.toString()) + .path("logs") .request() .header(HttpHeaders.AUTHORIZATION, apiKey) .accept(MediaType.APPLICATION_JSON_TYPE) @@ -83,7 +85,7 @@ public AutomationRuleEvaluator getEvaluator(UUID evaluatorId, UUID projectId, assertThat(actualResponse.getStatusInfo().getStatusCode()).isEqualTo(200); - return actualResponse.readEntity(AutomationRuleEvaluator.class); + return actualResponse.readEntity(LogPage.class); } } } diff --git a/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/events/OnlineScoringEngineTest.java b/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/events/OnlineScoringEngineTest.java index e8de4415da..c0f26c949d 100644 --- a/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/events/OnlineScoringEngineTest.java +++ b/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/events/OnlineScoringEngineTest.java @@ -17,8 +17,8 @@ import com.comet.opik.api.resources.utils.WireMockUtils; import com.comet.opik.api.resources.utils.resources.AutomationRuleEvaluatorResourceClient; import com.comet.opik.api.resources.utils.resources.ProjectResourceClient; -import com.comet.opik.domain.ChatCompletionService; import com.comet.opik.domain.FeedbackScoreService; +import com.comet.opik.domain.llm.ChatCompletionService; import com.comet.opik.infrastructure.DatabaseAnalyticsFactory; import com.comet.opik.podam.PodamFactoryUtils; import com.comet.opik.utils.JsonUtils; diff --git a/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/AutomationRuleEvaluatorsResourceTest.java b/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/AutomationRuleEvaluatorsResourceTest.java index 5aefa8d8ee..d5f8f3ea9b 100644 --- a/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/AutomationRuleEvaluatorsResourceTest.java +++ b/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/AutomationRuleEvaluatorsResourceTest.java @@ -18,7 +18,8 @@ import com.comet.opik.api.resources.utils.resources.AutomationRuleEvaluatorResourceClient; import com.comet.opik.api.resources.utils.resources.ProjectResourceClient; import com.comet.opik.api.resources.utils.resources.TraceResourceClient; -import com.comet.opik.domain.ChatCompletionService; +import com.comet.opik.domain.llm.LlmProviderFactory; +import com.comet.opik.infrastructure.llm.LlmModule; import com.comet.opik.podam.PodamFactoryUtils; import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.databind.ObjectMapper; @@ -31,6 +32,7 @@ import jakarta.ws.rs.client.Entity; import jakarta.ws.rs.core.HttpHeaders; import jakarta.ws.rs.core.MediaType; +import jakarta.ws.rs.core.Response; import org.apache.http.HttpStatus; import org.jdbi.v3.core.Jdbi; import org.junit.jupiter.api.AfterAll; @@ -44,6 +46,7 @@ import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.MethodSource; +import org.mockito.Mockito; import org.testcontainers.clickhouse.ClickHouseContainer; import org.testcontainers.containers.MySQLContainer; import org.testcontainers.lifecycle.Startables; @@ -73,9 +76,9 @@ import static com.github.tomakehurst.wiremock.client.WireMock.urlPathEqualTo; import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.params.provider.Arguments.arguments; -import static org.mockito.Mockito.any; -import static org.mockito.Mockito.doReturn; -import static org.mockito.Mockito.mock; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.when; @TestInstance(TestInstance.Lifecycle.PER_CLASS) @DisplayName("Automation Rule Evaluators Resource Test") @@ -142,12 +145,10 @@ class AutomationRuleEvaluatorsResourceTest { private static final ClickHouseContainer CLICKHOUSE = ClickHouseContainerUtils.newClickHouseContainer(); @RegisterExtension - private static final TestDropwizardAppExtension APP; + private static TestDropwizardAppExtension APP; private static final WireMockUtils.WireMockRuntime wireMock; - public static final ChatCompletionService COMPLETION_SERVICE = mock(ChatCompletionService.class); - static { Startables.deepStart(REDIS, MYSQL, CLICKHOUSE).join(); @@ -161,11 +162,13 @@ class AutomationRuleEvaluatorsResourceTest { .databaseAnalyticsFactory(databaseAnalyticsFactory) .redisUrl(REDIS.getRedisURI()) .runtimeInfo(wireMock.runtimeInfo()) + .disableModules(List.of(LlmModule.class)) .modules(List.of(new AbstractModule() { @Override - protected void configure() { - bind(ChatCompletionService.class).toInstance(COMPLETION_SERVICE); + public void configure() { + bind(LlmProviderFactory.class) + .toInstance(Mockito.mock(LlmProviderFactory.class, Mockito.RETURNS_DEEP_STUBS)); } })) @@ -521,6 +524,107 @@ void deleteProjectAutomationRuleEvaluators__whenApiKeyIsPresent__thenReturnPrope } } } + + @ParameterizedTest + @MethodSource("credentials") + @DisplayName("get logs per rule evaluators: when api key is present, then return proper response") + void getLogsPerRuleEvaluators__whenSessionTokenIsPresent__thenReturnProperResponse( + String apikey, + boolean isAuthorized, + LlmProviderFactory llmProviderFactory) throws JsonProcessingException { + + ChatResponse chatResponse = ChatResponse.builder() + .aiMessage(AiMessage.from(validAiMsgTxt)) + .build(); + + when(llmProviderFactory.getLanguageModel(anyString(), any()) + .chat(any())) + .thenAnswer(invocationOnMock -> chatResponse); + + String projectName = UUID.randomUUID().toString(); + + String workspaceName = "workspace-" + UUID.randomUUID(); + String workspaceId = UUID.randomUUID().toString(); + + mockTargetWorkspace(okApikey, workspaceName, workspaceId); + + ObjectMapper mapper = new ObjectMapper(); + + var projectId = projectResourceClient.createProject(projectName, okApikey, workspaceName); + + var evaluator = factory.manufacturePojo(AutomationRuleEvaluatorLlmAsJudge.class).toBuilder() + .id(null) + .code(mapper.readValue(testEvaluator, AutomationRuleEvaluatorLlmAsJudge.LlmAsJudgeCode.class)) + .samplingRate(1f) + .build(); + + var trace = factory.manufacturePojo(Trace.class).toBuilder() + .projectName(projectName) + .input(mapper.readTree(input)) + .output(mapper.readTree(output)) + .build(); + + var id = evaluatorsResourceClient.createEvaluator(evaluator, projectId, workspaceName, okApikey); + + Instant startTime = Instant.now(); + traceResourceClient.createTrace(trace, okApikey, workspaceName); + + Awaitility.await().untilAsserted(() -> { + + try (var actualResponse = client.target(URL_TEMPLATE.formatted(baseURI, projectId)) + .path(id.toString()) + .path("logs") + .request() + .accept(MediaType.APPLICATION_JSON_TYPE) + .header(HttpHeaders.AUTHORIZATION, apikey) + .header(WORKSPACE_HEADER, workspaceName) + .get()) { + + if (isAuthorized) { + assertLogResponse(actualResponse, startTime, id, trace); + } else { + assertThat(actualResponse.getStatusInfo().getStatusCode()) + .isEqualTo(HttpStatus.SC_UNAUTHORIZED); + assertThat(actualResponse.readEntity(io.dropwizard.jersey.errors.ErrorMessage.class)) + .isEqualTo(UNAUTHORIZED_RESPONSE); + } + } + }); + } + } + + private static void assertLogResponse(Response actualResponse, Instant startTime, UUID id, Trace trace) { + assertThat(actualResponse.getStatusInfo().getStatusCode()).isEqualTo(HttpStatus.SC_OK); + assertThat(actualResponse.hasEntity()).isTrue(); + + var actualEntity = actualResponse.readEntity(LogItem.LogPage.class); + + assertThat(actualEntity.content()).hasSize(4); + assertThat(actualEntity.total()).isEqualTo(4); + assertThat(actualEntity.size()).isEqualTo(4); + assertThat(actualEntity.page()).isEqualTo(1); + + assertThat(actualEntity.content()) + .allSatisfy(log -> { + assertThat(log.timestamp()).isBetween(startTime, Instant.now()); + assertThat(log.ruleId()).isEqualTo(id); + assertThat(log.markers()).isEqualTo(Map.of("trace_id", trace.id().toString())); + assertThat(log.level()).isEqualTo(LogItem.LogLevel.INFO); + }); + + assertThat(actualEntity.content()) + .anyMatch(log -> log.message() + .matches("Scores for traceId '.*' stored successfully:\\n\\n.*")); + + assertThat(actualEntity.content()) + .anyMatch(log -> log.message().matches("Received response for traceId '.*':\\n\\n.*")); + + assertThat(actualEntity.content()) + .anyMatch(log -> log.message().matches( + "(?s)Sending traceId '([^']*)' to LLM using the following input:\\n\\n.*")); + + assertThat(actualEntity.content()) + .anyMatch(log -> log.message().matches("Evaluating traceId '.*' sampled by rule '.*'")); } @Nested @@ -808,7 +912,16 @@ void deleteProjectAutomationRuleEvaluators__whenSessionTokenIsPresent__thenRetur void getLogsPerRuleEvaluators__whenSessionTokenIsPresent__thenReturnProperResponse( String sessionToken, boolean isAuthorized, - String workspaceName) throws JsonProcessingException { + String workspaceName, + LlmProviderFactory llmProviderFactory) throws JsonProcessingException { + + ChatResponse chatResponse = ChatResponse.builder() + .aiMessage(AiMessage.from(validAiMsgTxt)) + .build(); + + when(llmProviderFactory.getLanguageModel(anyString(), any()) + .chat(any())) + .thenAnswer(invocationOnMock -> chatResponse); String projectName = UUID.randomUUID().toString(); @@ -830,12 +943,6 @@ void getLogsPerRuleEvaluators__whenSessionTokenIsPresent__thenReturnProperRespon var id = evaluatorsResourceClient.createEvaluator(evaluator, projectId, WORKSPACE_NAME, API_KEY); - var chatResponse = ChatResponse.builder().aiMessage(AiMessage.from(validAiMsgTxt)).build(); - - doReturn(chatResponse) - .when(COMPLETION_SERVICE) - .scoreTrace(any(), any(), any()); - Instant startTime = Instant.now(); traceResourceClient.createTrace(trace, API_KEY, WORKSPACE_NAME); @@ -851,38 +958,7 @@ void getLogsPerRuleEvaluators__whenSessionTokenIsPresent__thenReturnProperRespon .get()) { if (isAuthorized) { - assertThat(actualResponse.getStatusInfo().getStatusCode()).isEqualTo(HttpStatus.SC_OK); - assertThat(actualResponse.hasEntity()).isTrue(); - - var actualEntity = actualResponse.readEntity(LogItem.LogPage.class); - - assertThat(actualEntity.content()).hasSize(4); - assertThat(actualEntity.total()).isEqualTo(4); - assertThat(actualEntity.size()).isEqualTo(4); - assertThat(actualEntity.page()).isEqualTo(1); - - assertThat(actualEntity.content()) - .allSatisfy(log -> { - assertThat(log.timestamp()).isBetween(startTime, Instant.now()); - assertThat(log.ruleId()).isEqualTo(id); - assertThat(log.markers()).isEqualTo(Map.of("trace_id", trace.id().toString())); - assertThat(log.level()).isEqualTo(LogItem.LogLevel.INFO); - }); - - assertThat(actualEntity.content()) - .anyMatch(log -> log.message() - .matches("Scores for traceId '.*' stored successfully:\\n\\n.*")); - - assertThat(actualEntity.content()) - .anyMatch(log -> log.message().matches("Received response for traceId '.*':\\n\\n.*")); - - assertThat(actualEntity.content()) - .anyMatch(log -> log.message().matches( - "(?s)Sending traceId '([^']*)' to LLM using the following input:\\n\\n.*")); - - assertThat(actualEntity.content()) - .anyMatch(log -> log.message().matches("Evaluating traceId '.*' sampled by rule '.*'")); - + assertLogResponse(actualResponse, startTime, id, trace); } else { assertThat(actualResponse.getStatusInfo().getStatusCode()) .isEqualTo(HttpStatus.SC_UNAUTHORIZED); diff --git a/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/ChatCompletionsResourceTest.java b/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/ChatCompletionsResourceTest.java index 8c3d5cc830..3d9f529957 100644 --- a/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/ChatCompletionsResourceTest.java +++ b/apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/ChatCompletionsResourceTest.java @@ -11,9 +11,10 @@ import com.comet.opik.api.resources.utils.WireMockUtils; import com.comet.opik.api.resources.utils.resources.ChatCompletionsClient; import com.comet.opik.api.resources.utils.resources.LlmProviderApiKeyResourceClient; -import com.comet.opik.domain.llmproviders.AnthropicModelName; -import com.comet.opik.domain.llmproviders.GeminiModelName; -import com.comet.opik.domain.llmproviders.OpenaiModelName; +import com.comet.opik.domain.llm.LlmProviderFactory; +import com.comet.opik.infrastructure.llm.antropic.AnthropicModelName; +import com.comet.opik.infrastructure.llm.gemini.GeminiModelName; +import com.comet.opik.infrastructure.llm.openai.OpenaiModelName; import com.comet.opik.podam.PodamFactoryUtils; import com.redis.testcontainers.RedisContainer; import dev.ai4j.openai4j.chat.ChatCompletionRequest; @@ -41,9 +42,9 @@ import java.util.UUID; import java.util.stream.Stream; -import static com.comet.opik.domain.ChatCompletionService.ERROR_EMPTY_MESSAGES; -import static com.comet.opik.domain.ChatCompletionService.ERROR_NO_COMPLETION_TOKENS; -import static com.comet.opik.domain.llmproviders.LlmProviderFactory.ERROR_MODEL_NOT_SUPPORTED; +import static com.comet.opik.domain.llm.ChatCompletionService.ERROR_EMPTY_MESSAGES; +import static com.comet.opik.domain.llm.ChatCompletionService.ERROR_NO_COMPLETION_TOKENS; +import static com.comet.opik.domain.llm.LlmProviderFactory.ERROR_MODEL_NOT_SUPPORTED; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assumptions.assumeThat; import static org.junit.jupiter.api.Named.named; @@ -182,7 +183,7 @@ void createReturnsBadRequestWhenModelIsInvalid(String model) { assertThat(errorMessage.getCode()).isEqualTo(HttpStatus.SC_BAD_REQUEST); assertThat(errorMessage.getMessage()) - .containsIgnoringCase(ERROR_MODEL_NOT_SUPPORTED.formatted(model)); + .containsIgnoringCase(LlmProviderFactory.ERROR_MODEL_NOT_SUPPORTED.formatted(model)); } @ParameterizedTest diff --git a/apps/opik-backend/src/test/java/com/comet/opik/domain/llmproviders/LlmProviderFactoryTest.java b/apps/opik-backend/src/test/java/com/comet/opik/infrastructure/llm/LlmProviderFactoryTest.java similarity index 65% rename from apps/opik-backend/src/test/java/com/comet/opik/domain/llmproviders/LlmProviderFactoryTest.java rename to apps/opik-backend/src/test/java/com/comet/opik/infrastructure/llm/LlmProviderFactoryTest.java index 5d7a2725f6..6dd9279eb1 100644 --- a/apps/opik-backend/src/test/java/com/comet/opik/domain/llmproviders/LlmProviderFactoryTest.java +++ b/apps/opik-backend/src/test/java/com/comet/opik/infrastructure/llm/LlmProviderFactoryTest.java @@ -1,11 +1,21 @@ -package com.comet.opik.domain.llmproviders; +package com.comet.opik.infrastructure.llm; import com.comet.opik.api.LlmProvider; import com.comet.opik.api.ProviderApiKey; import com.comet.opik.domain.LlmProviderApiKeyService; +import com.comet.opik.domain.llm.LlmProviderService; import com.comet.opik.infrastructure.EncryptionUtils; import com.comet.opik.infrastructure.LlmProviderClientConfig; import com.comet.opik.infrastructure.OpikConfiguration; +import com.comet.opik.infrastructure.llm.antropic.AnthropicClientGenerator; +import com.comet.opik.infrastructure.llm.antropic.AnthropicModelName; +import com.comet.opik.infrastructure.llm.antropic.AnthropicModule; +import com.comet.opik.infrastructure.llm.gemini.GeminiClientGenerator; +import com.comet.opik.infrastructure.llm.gemini.GeminiModelName; +import com.comet.opik.infrastructure.llm.gemini.GeminiModule; +import com.comet.opik.infrastructure.llm.openai.OpenAIClientGenerator; +import com.comet.opik.infrastructure.llm.openai.OpenAIModule; +import com.comet.opik.infrastructure.llm.openai.OpenaiModelName; import com.fasterxml.jackson.databind.ObjectMapper; import io.dropwizard.configuration.ConfigurationException; import io.dropwizard.configuration.FileConfigurationSourceProvider; @@ -50,7 +60,7 @@ void setUpAll() throws ConfigurationException, IOException { @ParameterizedTest @MethodSource - void testGetService(String model, LlmProvider llmProvider, Class providerClass) { + void testGetService(String model, LlmProvider llmProvider, String providerClass) { // setup LlmProviderApiKeyService llmProviderApiKeyService = mock(LlmProviderApiKeyService.class); String workspaceId = UUID.randomUUID().toString(); @@ -67,22 +77,34 @@ void testGetService(String model, LlmProvider llmProvider, Class testGetService() { var openAiModels = EnumUtils.getEnumList(OpenaiModelName.class).stream() - .map(model -> arguments(model.toString(), LlmProvider.OPEN_AI, LlmProviderOpenAi.class)); + .map(model -> arguments(model.toString(), LlmProvider.OPEN_AI, "LlmProviderOpenAi")); var anthropicModels = EnumUtils.getEnumList(AnthropicModelName.class).stream() - .map(model -> arguments(model.toString(), LlmProvider.ANTHROPIC, LlmProviderAnthropic.class)); + .map(model -> arguments(model.toString(), LlmProvider.ANTHROPIC, "LlmProviderAnthropic")); var geminiModels = EnumUtils.getEnumList(GeminiModelName.class).stream() - .map(model -> arguments(model.toString(), LlmProvider.GEMINI, LlmProviderGemini.class)); + .map(model -> arguments(model.toString(), LlmProvider.GEMINI, "LlmProviderGemini")); return Stream.of(openAiModels, anthropicModels, geminiModels).flatMap(Function.identity()); } diff --git a/apps/opik-backend/src/test/java/com/comet/opik/infrastructure/llm/antropic/AnthropicMappersTest.java b/apps/opik-backend/src/test/java/com/comet/opik/infrastructure/llm/antropic/AnthropicMappersTest.java new file mode 100644 index 0000000000..0c610fa710 --- /dev/null +++ b/apps/opik-backend/src/test/java/com/comet/opik/infrastructure/llm/antropic/AnthropicMappersTest.java @@ -0,0 +1,73 @@ +package com.comet.opik.infrastructure.llm.antropic; + +import com.comet.opik.podam.PodamFactoryUtils; +import dev.ai4j.openai4j.chat.AssistantMessage; +import dev.ai4j.openai4j.chat.ChatCompletionChoice; +import dev.ai4j.openai4j.chat.ChatCompletionRequest; +import dev.ai4j.openai4j.chat.Role; +import dev.ai4j.openai4j.shared.Usage; +import dev.langchain4j.model.anthropic.internal.api.AnthropicCreateMessageRequest; +import dev.langchain4j.model.anthropic.internal.api.AnthropicCreateMessageResponse; +import org.junit.jupiter.api.Nested; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.TestInstance; +import uk.co.jemos.podam.api.PodamFactory; + +import java.util.List; + +import static org.assertj.core.api.Assertions.assertThat; + +public class AnthropicMappersTest { + private final PodamFactory podamFactory = PodamFactoryUtils.newPodamFactory(); + + @Nested + @TestInstance(TestInstance.Lifecycle.PER_CLASS) + class AnthropicMappers { + @Test + void testToResponse() { + var response = podamFactory.manufacturePojo(AnthropicCreateMessageResponse.class); + + var actual = LlmProviderAnthropicMapper.INSTANCE.toResponse(response); + assertThat(actual).isNotNull(); + assertThat(actual.id()).isEqualTo(response.id); + assertThat(actual.choices()).isEqualTo(List.of(ChatCompletionChoice.builder() + .message(AssistantMessage.builder() + .name(response.content.getFirst().name) + .content(response.content.getFirst().text) + .build()) + .finishReason(response.stopReason) + .build())); + assertThat(actual.usage()).isEqualTo(Usage.builder() + .promptTokens(response.usage.inputTokens) + .completionTokens(response.usage.outputTokens) + .totalTokens(response.usage.inputTokens + response.usage.outputTokens) + .build()); + } + + @Test + void toCreateMessage() { + var request = podamFactory.manufacturePojo(ChatCompletionRequest.class); + + AnthropicCreateMessageRequest actual = LlmProviderAnthropicMapper.INSTANCE + .toCreateMessageRequest(request); + + assertThat(actual).isNotNull(); + assertThat(actual.model).isEqualTo(request.model()); + assertThat(actual.stream).isEqualTo(request.stream()); + assertThat(actual.temperature).isEqualTo(request.temperature()); + assertThat(actual.topP).isEqualTo(request.topP()); + assertThat(actual.stopSequences).isEqualTo(request.stop()); + assertThat(actual.messages).usingRecursiveComparison().ignoringCollectionOrder().isEqualTo( + request.messages().stream() + .filter(message -> List.of(Role.USER, Role.ASSISTANT).contains(message.role())) + .map(LlmProviderAnthropicMapper.INSTANCE::mapToAnthropicMessage) + .toList()); + assertThat(actual.system).usingRecursiveComparison().ignoringCollectionOrder().isEqualTo( + request.messages().stream() + .filter(message -> message.role() == Role.SYSTEM) + .map(LlmProviderAnthropicMapper.INSTANCE::mapToSystemMessage) + .toList()); + } + } + +} diff --git a/apps/opik-backend/src/test/java/com/comet/opik/domain/llmproviders/LlmProviderClientsMappersTest.java b/apps/opik-backend/src/test/java/com/comet/opik/infrastructure/llm/gemini/GeminiMappersTest.java similarity index 54% rename from apps/opik-backend/src/test/java/com/comet/opik/domain/llmproviders/LlmProviderClientsMappersTest.java rename to apps/opik-backend/src/test/java/com/comet/opik/infrastructure/llm/gemini/GeminiMappersTest.java index 37f073194b..6b795a6314 100644 --- a/apps/opik-backend/src/test/java/com/comet/opik/domain/llmproviders/LlmProviderClientsMappersTest.java +++ b/apps/opik-backend/src/test/java/com/comet/opik/infrastructure/llm/gemini/GeminiMappersTest.java @@ -1,18 +1,15 @@ -package com.comet.opik.domain.llmproviders; +package com.comet.opik.infrastructure.llm.gemini; import com.comet.opik.podam.PodamFactoryUtils; import dev.ai4j.openai4j.chat.AssistantMessage; import dev.ai4j.openai4j.chat.ChatCompletionChoice; import dev.ai4j.openai4j.chat.ChatCompletionRequest; import dev.ai4j.openai4j.chat.Message; -import dev.ai4j.openai4j.chat.Role; import dev.ai4j.openai4j.shared.Usage; import dev.langchain4j.data.message.AiMessage; import dev.langchain4j.data.message.ChatMessage; import dev.langchain4j.data.message.SystemMessage; import dev.langchain4j.data.message.UserMessage; -import dev.langchain4j.model.anthropic.internal.api.AnthropicCreateMessageRequest; -import dev.langchain4j.model.anthropic.internal.api.AnthropicCreateMessageResponse; import dev.langchain4j.model.output.FinishReason; import dev.langchain4j.model.output.Response; import dev.langchain4j.model.output.TokenUsage; @@ -31,59 +28,9 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.params.provider.Arguments.arguments; -public class LlmProviderClientsMappersTest { +public class GeminiMappersTest { private final PodamFactory podamFactory = PodamFactoryUtils.newPodamFactory(); - @Nested - @TestInstance(TestInstance.Lifecycle.PER_CLASS) - class AnthropicMappers { - @Test - void testToResponse() { - var response = podamFactory.manufacturePojo(AnthropicCreateMessageResponse.class); - - var actual = LlmProviderAnthropicMapper.INSTANCE.toResponse(response); - assertThat(actual).isNotNull(); - assertThat(actual.id()).isEqualTo(response.id); - assertThat(actual.choices()).isEqualTo(List.of(ChatCompletionChoice.builder() - .message(AssistantMessage.builder() - .name(response.content.getFirst().name) - .content(response.content.getFirst().text) - .build()) - .finishReason(response.stopReason) - .build())); - assertThat(actual.usage()).isEqualTo(Usage.builder() - .promptTokens(response.usage.inputTokens) - .completionTokens(response.usage.outputTokens) - .totalTokens(response.usage.inputTokens + response.usage.outputTokens) - .build()); - } - - @Test - void toCreateMessage() { - var request = podamFactory.manufacturePojo(ChatCompletionRequest.class); - - AnthropicCreateMessageRequest actual = LlmProviderAnthropicMapper.INSTANCE - .toCreateMessageRequest(request); - - assertThat(actual).isNotNull(); - assertThat(actual.model).isEqualTo(request.model()); - assertThat(actual.stream).isEqualTo(request.stream()); - assertThat(actual.temperature).isEqualTo(request.temperature()); - assertThat(actual.topP).isEqualTo(request.topP()); - assertThat(actual.stopSequences).isEqualTo(request.stop()); - assertThat(actual.messages).usingRecursiveComparison().ignoringCollectionOrder().isEqualTo( - request.messages().stream() - .filter(message -> List.of(Role.USER, Role.ASSISTANT).contains(message.role())) - .map(LlmProviderAnthropicMapper.INSTANCE::mapToAnthropicMessage) - .toList()); - assertThat(actual.system).usingRecursiveComparison().ignoringCollectionOrder().isEqualTo( - request.messages().stream() - .filter(message -> message.role() == Role.SYSTEM) - .map(LlmProviderAnthropicMapper.INSTANCE::mapToSystemMessage) - .toList()); - } - } - @Nested @TestInstance(TestInstance.Lifecycle.PER_CLASS) class GeminiMappers { From 57f232e8086dd6aa4128d696abe5610b8b2257ea Mon Sep 17 00:00:00 2001 From: Thiago Hora Date: Tue, 21 Jan 2025 20:41:53 +0100 Subject: [PATCH 15/17] Fix references --- .../log/ClickHouseAppender.java | 21 +++++++++++-------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/ClickHouseAppender.java b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/ClickHouseAppender.java index 52e54dc025..533aae0c66 100644 --- a/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/ClickHouseAppender.java +++ b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/ClickHouseAppender.java @@ -18,6 +18,7 @@ import java.util.concurrent.LinkedBlockingQueue; import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicReference; import java.util.stream.Collectors; import static java.util.stream.Collectors.groupingBy; @@ -51,13 +52,14 @@ private static synchronized void setInstance(ClickHouseAppender instance) { private volatile boolean running = true; private final BlockingQueue logQueue = new LinkedBlockingQueue<>(); - private ScheduledExecutorService scheduler = Executors.newSingleThreadScheduledExecutor(); + private AtomicReference scheduler = new AtomicReference<>( + Executors.newSingleThreadScheduledExecutor()); @Override public void start() { // Background flush thread - scheduler.scheduleAtFixedRate(this::flushLogs, flushIntervalDuration.toMillis(), + scheduler.get().scheduleAtFixedRate(this::flushLogs, flushIntervalDuration.toMillis(), flushIntervalDuration.toMillis(), TimeUnit.MILLISECONDS); super.start(); @@ -108,7 +110,7 @@ protected void append(ILoggingEvent event) { } if (logQueue.size() >= batchSize) { - scheduler.execute(this::flushLogs); + scheduler.get().execute(this::flushLogs); } } @@ -118,22 +120,23 @@ public void stop() { super.stop(); flushLogs(); setInstance(null); - scheduler.shutdown(); + scheduler.get().shutdown(); awaitTermination(); logQueue.clear(); - scheduler = Executors.newSingleThreadScheduledExecutor(); + scheduler.set(Executors.newSingleThreadScheduledExecutor()); } private void awaitTermination() { try { - if (!scheduler.awaitTermination(5, TimeUnit.SECONDS)) { - scheduler.shutdownNow(); - if (!scheduler.awaitTermination(5, TimeUnit.SECONDS)) { // Final attempt + if (!scheduler.get().awaitTermination(5, TimeUnit.SECONDS)) { + scheduler.get().shutdownNow(); + if (!scheduler.get().awaitTermination(5, TimeUnit.SECONDS)) { // Final attempt log.error("ClickHouseAppender did not terminate"); } } } catch (InterruptedException ex) { - scheduler.shutdownNow(); + Thread.currentThread().interrupt(); + scheduler.get().shutdownNow(); log.warn("ClickHouseAppender interrupted while waiting for termination", ex); } } From df6a3df285f8295c26cf0269319984fe9722df31 Mon Sep 17 00:00:00 2001 From: Thiago Hora Date: Wed, 22 Jan 2025 09:25:47 +0100 Subject: [PATCH 16/17] Fix log message --- .../v1/events/OnlineScoringLlmAsJudgeScorer.java | 9 +++++++-- .../infrastructure/llm/LlmProviderClientGenerator.java | 2 +- .../llm/antropic/AnthropicClientGenerator.java | 2 +- .../infrastructure/llm/gemini/GeminiClientGenerator.java | 6 ++++-- .../infrastructure/llm/gemini/LlmProviderGemini.java | 2 +- .../infrastructure/llm/openai/OpenAIClientGenerator.java | 2 +- 6 files changed, 15 insertions(+), 8 deletions(-) diff --git a/apps/opik-backend/src/main/java/com/comet/opik/api/resources/v1/events/OnlineScoringLlmAsJudgeScorer.java b/apps/opik-backend/src/main/java/com/comet/opik/api/resources/v1/events/OnlineScoringLlmAsJudgeScorer.java index 2a973b2bc2..df999de488 100644 --- a/apps/opik-backend/src/main/java/com/comet/opik/api/resources/v1/events/OnlineScoringLlmAsJudgeScorer.java +++ b/apps/opik-backend/src/main/java/com/comet/opik/api/resources/v1/events/OnlineScoringLlmAsJudgeScorer.java @@ -81,9 +81,14 @@ public OnlineScoringLlmAsJudgeScorer(@NonNull @Config("onlineScoring") OnlineSco @Override public void start() { - log.info("OnlineScoringLlmAsJudgeScorer started."); + if (stream != null) { + log.warn("OnlineScoringLlmAsJudgeScorer already started. Ignoring start request."); + return; + } + // as we are a LLM consumer, lets check only LLM stream stream = initStream(config, redisson); + log.info("OnlineScoringLlmAsJudgeScorer started."); } @Override @@ -208,7 +213,7 @@ private void processReceivedMessages(RStreamReactive { - T generate(String apiKey, Object... args); + T generate(String apiKey, Object... params); ChatLanguageModel generateChat(String apiKey, LlmAsJudgeModelParameters modelParameters); } diff --git a/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/antropic/AnthropicClientGenerator.java b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/antropic/AnthropicClientGenerator.java index b29dc2ba4b..b599da7bf2 100644 --- a/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/antropic/AnthropicClientGenerator.java +++ b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/antropic/AnthropicClientGenerator.java @@ -59,7 +59,7 @@ private ChatLanguageModel newChatLanguageModel(String apiKey, LlmAsJudgeModelPar } @Override - public AnthropicClient generate(@NonNull String apiKey, Object... args) { + public AnthropicClient generate(@NonNull String apiKey, Object... params) { return newAnthropicClient(apiKey); } diff --git a/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/gemini/GeminiClientGenerator.java b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/gemini/GeminiClientGenerator.java index 497a8e825c..52b53468c2 100644 --- a/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/gemini/GeminiClientGenerator.java +++ b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/gemini/GeminiClientGenerator.java @@ -2,6 +2,7 @@ import com.comet.opik.infrastructure.LlmProviderClientConfig; import com.comet.opik.infrastructure.llm.LlmProviderClientGenerator; +import com.google.common.base.Preconditions; import dev.ai4j.openai4j.chat.ChatCompletionRequest; import dev.langchain4j.model.chat.ChatLanguageModel; import dev.langchain4j.model.googleai.GoogleAiGeminiChatModel; @@ -33,8 +34,9 @@ public GoogleAiGeminiStreamingChatModel newGeminiStreamingClient( } @Override - public GoogleAiGeminiChatModel generate(String apiKey, Object... args) { - ChatCompletionRequest request = (ChatCompletionRequest) Objects.requireNonNull(args[0], + public GoogleAiGeminiChatModel generate(String apiKey, Object... params) { + Preconditions.checkArgument(params.length >= 1, "Expected at least 1 parameter, got " + params.length); + ChatCompletionRequest request = (ChatCompletionRequest) Objects.requireNonNull(params[0], "ChatCompletionRequest is required"); return newGeminiClient(apiKey, request); } diff --git a/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/gemini/LlmProviderGemini.java b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/gemini/LlmProviderGemini.java index f3abd047e1..94b5b26c11 100644 --- a/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/gemini/LlmProviderGemini.java +++ b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/gemini/LlmProviderGemini.java @@ -19,7 +19,7 @@ public class LlmProviderGemini implements LlmProviderService { @Override public ChatCompletionResponse generate(@NonNull ChatCompletionRequest request, @NonNull String workspaceId) { var mapper = LlmProviderGeminiMapper.INSTANCE; - var response = llmProviderClientGenerator.newGeminiClient(apiKey, request) + var response = llmProviderClientGenerator.generate(apiKey, request) .generate(request.messages().stream().map(mapper::toChatMessage).toList()); return mapper.toChatCompletionResponse(request, response); diff --git a/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/openai/OpenAIClientGenerator.java b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/openai/OpenAIClientGenerator.java index 94f742b230..56aa3189de 100644 --- a/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/openai/OpenAIClientGenerator.java +++ b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/llm/openai/OpenAIClientGenerator.java @@ -58,7 +58,7 @@ public ChatLanguageModel newOpenAiChatLanguageModel(String apiKey, LlmAsJudgeMod } @Override - public OpenAiClient generate(@NonNull String apiKey, Object... args) { + public OpenAiClient generate(@NonNull String apiKey, Object... params) { return newOpenAiClient(apiKey); } From 7633e656cba60e80c0fab3c30e83e67d5de5babc Mon Sep 17 00:00:00 2001 From: Thiago Hora Date: Wed, 22 Jan 2025 09:39:49 +0100 Subject: [PATCH 17/17] Reuse DAO --- .../AutomationRuleEvaluatorLogsDAO.java | 69 ++++++++++++++- .../tables/AutomationRuleEvaluatorLogDAO.java | 84 ------------------- .../log/tables/UserLogTableFactory.java | 3 +- 3 files changed, 70 insertions(+), 86 deletions(-) delete mode 100644 apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/tables/AutomationRuleEvaluatorLogDAO.java diff --git a/apps/opik-backend/src/main/java/com/comet/opik/domain/AutomationRuleEvaluatorLogsDAO.java b/apps/opik-backend/src/main/java/com/comet/opik/domain/AutomationRuleEvaluatorLogsDAO.java index 2dcca620ae..bcdce6657b 100644 --- a/apps/opik-backend/src/main/java/com/comet/opik/domain/AutomationRuleEvaluatorLogsDAO.java +++ b/apps/opik-backend/src/main/java/com/comet/opik/domain/AutomationRuleEvaluatorLogsDAO.java @@ -1,7 +1,9 @@ package com.comet.opik.domain; +import ch.qos.logback.classic.spi.ILoggingEvent; import com.comet.opik.api.LogCriteria; import com.comet.opik.api.LogItem; +import com.comet.opik.utils.TemplateUtils; import com.google.inject.ImplementedBy; import io.r2dbc.spi.ConnectionFactory; import io.r2dbc.spi.Row; @@ -22,9 +24,15 @@ import static com.comet.opik.api.LogItem.LogLevel; import static com.comet.opik.api.LogItem.LogPage; +import static com.comet.opik.infrastructure.log.tables.UserLogTableFactory.*; +import static com.comet.opik.utils.TemplateUtils.getQueryItemPlaceHolder; @ImplementedBy(AutomationRuleEvaluatorLogsDAOImpl.class) -public interface AutomationRuleEvaluatorLogsDAO { +public interface AutomationRuleEvaluatorLogsDAO extends UserLogTableDAO { + + static AutomationRuleEvaluatorLogsDAO create(ConnectionFactory factory) { + return new AutomationRuleEvaluatorLogsDAOImpl(factory); + } Mono findLogs(LogCriteria criteria); @@ -35,6 +43,22 @@ public interface AutomationRuleEvaluatorLogsDAO { @RequiredArgsConstructor(onConstructor_ = @Inject) class AutomationRuleEvaluatorLogsDAOImpl implements AutomationRuleEvaluatorLogsDAO { + private static final String INSERT_STATEMENT = """ + INSERT INTO automation_rule_evaluator_logs (timestamp, level, workspace_id, rule_id, message, markers) + VALUES , 9), + :level, + :workspace_id, + :rule_id, + :message, + mapFromArrays(:marker_keys, :marker_values) + ) + , + }> + ; + """; + public static final String FIND_ALL = """ SELECT * FROM automation_rule_evaluator_logs WHERE workspace_id = :workspaceId @@ -99,4 +123,47 @@ private void bindParameters(LogCriteria criteria, Statement statement) { Optional.ofNullable(criteria.size()).ifPresent(limit -> statement.bind("limit", limit)); } + @Override + public Mono saveAll(@NonNull List events) { + return Mono.from(connectionFactory.create()) + .flatMapMany(connection -> { + var template = new ST(INSERT_STATEMENT); + List queryItems = getQueryItemPlaceHolder(events.size()); + + template.add("items", queryItems); + + Statement statement = connection.createStatement(template.render()); + + for (int i = 0; i < events.size(); i++) { + ILoggingEvent event = events.get(i); + + String logLevel = event.getLevel().toString(); + String workspaceId = Optional.ofNullable(event.getMDCPropertyMap().get("workspace_id")) + .orElseThrow(() -> failWithMessage("workspace_id is not set")); + String traceId = Optional.ofNullable(event.getMDCPropertyMap().get("trace_id")) + .orElseThrow(() -> failWithMessage("trace_id is not set")); + String ruleId = Optional.ofNullable(event.getMDCPropertyMap().get("rule_id")) + .orElseThrow(() -> failWithMessage("rule_id is not set")); + + statement + .bind("timestamp" + i, event.getInstant().toString()) + .bind("level" + i, logLevel) + .bind("workspace_id" + i, workspaceId) + .bind("rule_id" + i, ruleId) + .bind("message" + i, event.getFormattedMessage()) + .bind("marker_keys" + i, new String[]{"trace_id"}) + .bind("marker_values" + i, new String[]{traceId}); + } + + return statement.execute(); + }) + .collectList() + .then(); + } + + private IllegalStateException failWithMessage(String message) { + log.error(message); + return new IllegalStateException(message); + } + } \ No newline at end of file diff --git a/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/tables/AutomationRuleEvaluatorLogDAO.java b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/tables/AutomationRuleEvaluatorLogDAO.java deleted file mode 100644 index a0b976c816..0000000000 --- a/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/tables/AutomationRuleEvaluatorLogDAO.java +++ /dev/null @@ -1,84 +0,0 @@ -package com.comet.opik.infrastructure.log.tables; - -import ch.qos.logback.classic.spi.ILoggingEvent; -import com.comet.opik.utils.TemplateUtils; -import io.r2dbc.spi.ConnectionFactory; -import io.r2dbc.spi.Statement; -import lombok.NonNull; -import lombok.RequiredArgsConstructor; -import lombok.extern.slf4j.Slf4j; -import org.stringtemplate.v4.ST; -import reactor.core.publisher.Mono; - -import java.util.List; -import java.util.Optional; - -import static com.comet.opik.infrastructure.log.tables.UserLogTableFactory.UserLogTableDAO; -import static com.comet.opik.utils.TemplateUtils.getQueryItemPlaceHolder; - -@RequiredArgsConstructor -@Slf4j -class AutomationRuleEvaluatorLogDAO implements UserLogTableDAO { - - private final ConnectionFactory factory; - - private static final String INSERT_STATEMENT = """ - INSERT INTO automation_rule_evaluator_logs (timestamp, level, workspace_id, rule_id, message, markers) - VALUES , 9), - :level, - :workspace_id, - :rule_id, - :message, - mapFromArrays(:marker_keys, :marker_values) - ) - , - }> - ; - """; - - @Override - public Mono saveAll(@NonNull List events) { - return Mono.from(factory.create()) - .flatMapMany(connection -> { - var template = new ST(INSERT_STATEMENT); - List queryItems = getQueryItemPlaceHolder(events.size()); - - template.add("items", queryItems); - - Statement statement = connection.createStatement(template.render()); - - for (int i = 0; i < events.size(); i++) { - ILoggingEvent event = events.get(i); - - String logLevel = event.getLevel().toString(); - String workspaceId = Optional.ofNullable(event.getMDCPropertyMap().get("workspace_id")) - .orElseThrow(() -> failWithMessage("workspace_id is not set")); - String traceId = Optional.ofNullable(event.getMDCPropertyMap().get("trace_id")) - .orElseThrow(() -> failWithMessage("trace_id is not set")); - String ruleId = Optional.ofNullable(event.getMDCPropertyMap().get("rule_id")) - .orElseThrow(() -> failWithMessage("rule_id is not set")); - - statement - .bind("timestamp" + i, event.getInstant().toString()) - .bind("level" + i, logLevel) - .bind("workspace_id" + i, workspaceId) - .bind("rule_id" + i, ruleId) - .bind("message" + i, event.getFormattedMessage()) - .bind("marker_keys" + i, new String[]{"trace_id"}) - .bind("marker_values" + i, new String[]{traceId}); - } - - return statement.execute(); - }) - .collectList() - .then(); - } - - private IllegalStateException failWithMessage(String message) { - log.error(message); - return new IllegalStateException(message); - } - -} diff --git a/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/tables/UserLogTableFactory.java b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/tables/UserLogTableFactory.java index def849d8c0..5900326a46 100644 --- a/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/tables/UserLogTableFactory.java +++ b/apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/tables/UserLogTableFactory.java @@ -1,6 +1,7 @@ package com.comet.opik.infrastructure.log.tables; import ch.qos.logback.classic.spi.ILoggingEvent; +import com.comet.opik.domain.AutomationRuleEvaluatorLogsDAO; import com.comet.opik.domain.UserLog; import io.r2dbc.spi.ConnectionFactory; import lombok.NonNull; @@ -29,7 +30,7 @@ class UserLogTableFactoryImpl implements UserLogTableFactory { UserLogTableFactoryImpl(@NonNull ConnectionFactory factory) { daoMap = Map.of( - UserLog.AUTOMATION_RULE_EVALUATOR, new AutomationRuleEvaluatorLogDAO(factory)); + UserLog.AUTOMATION_RULE_EVALUATOR, AutomationRuleEvaluatorLogsDAO.create(factory)); } @Override