OPIK-796: Implement ClickHouse user facing logs #1066

thiagohora · 2025-01-16T21:41:01Z

Details

Add a specialized logger to send logs to the user-facing rule logs table.

Issues

OPIK-796

#Testing

Manually tested since currently, this feature is being manually tested due to the problem with the API-KEY in our pipeline.

apps/opik-backend/config.yml

...backend/src/main/java/com/comet/opik/api/resources/v1/events/OnlineScoringEventListener.java

idoberko2

LGTM, left a few minor comments.
Do you plan to add tests in a later PR? Have you tested locally?

thiagohora · 2025-01-17T08:44:54Z

LGTM, left a few minor comments. Do you plan to add tests in a later PR? Have you tested locally?

You are right, sorry I forgot to add the description. Yes, it was manually tested since this feature still doesn't have automated tests due to the issue with API-KEYs running in our pipeline.

…ora/OPIK-796_implement_clickhouse_user_logs

andrescrz · 2025-01-17T09:29:51Z

@thiagohora reviewing your base PR (the table model now). I'll get back to this in a sec. Please hold the merge. Better push them separately.

thiagohora · 2025-01-17T09:31:35Z

@thiagohora reviewing your base PR (the table model now). I'll get back to this in a sec. Please hold the merge. Better push them separately.

Agree, don't worry I will not merge it.

…ora/OPIK-796_implement_clickhouse_user_logs

…ttps://github.com/comet-ml/opik into thiagohora/OPIK-796_implement_clickhouse_user_logs

…r_logs

andrescrz

Let's polish the details of this approach and add test coverage.

andrescrz · 2025-01-20T09:02:02Z

...backend/src/main/java/com/comet/opik/api/resources/v1/events/OnlineScoringEventListener.java

@@ -29,6 +37,7 @@ public class OnlineScoringEventListener {
    private final AutomationRuleEvaluatorService ruleEvaluatorService;
    private final ChatCompletionService aiProxyService;
    private final FeedbackScoreService feedbackScoreService;
+    private final Logger userFacingLogger;


Minor: let's add non-null validation to all these fields.

Minor: after looking a the constructor, actually this annotation that I suggested previously doesn't do anything. But thanks for adding the others to the constructor arguments, those indeed validate.

andrescrz · 2025-01-20T09:14:11Z

apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/ClickHouseAppender.java

+import ch.qos.logback.classic.spi.ILoggingEvent;
+import ch.qos.logback.core.AppenderBase;


Is there any way to not to rely on the particular log framework implementation (Logback) and just rely on the facade (SL4J)?

No, apparently, SLF4J doesn't have an abstraction for appenders.

apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/ClickHouseAppender.java

andrescrz · 2025-01-20T09:18:10Z

apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/ClickHouseAppender.java

+        }
+    }
+
+    public static ClickHouseAppender getInstance() {


This needs to be synchronized as well. But I think it's better to merge init and getInstance methods into a single one.

We probably need to think about this because many configuration settings must be made statically. Doing this in the get instance is challenging because we must pass the connection factory. Another option would be to make the log factory a component.

In your current implementation you always call both in a row. I'd personally just return the instance from init and maybe remove it to getInstance, so you end up with a method that both inits and returns the instance.

apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/ClickHouseAppender.java

andrescrz · 2025-01-20T09:31:41Z

apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/ClickHouseAppender.java

+
+    @Override
+    protected void append(ILoggingEvent event) {
+        if (!running) return;


Add some logs to these conditionals, so we can know that they were executed. Ok to be at debug level if you think they can be too noisy. Review others in this class.

You missed a few of these. But not a blocker.

Added debug log

andrescrz · 2025-01-20T09:32:29Z

apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/ClickHouseAppender.java

+        boolean added = logQueue.offer(event);
+        if (!added) {
+            log.warn("Log queue is full, dropping log: {}", event.getFormattedMessage());
+        }


Consider flushing before, to reduce the chances of dropping logs when the queue is full.

The idea is the log to ran async, to I would about blocking for long during append

I see the call below is in the async executor

if (logQueue.size() >= batchSize) { scheduler.execute(this::flushLogs); }

I didn't notice that in my first review.

Moving boolean added = logQueue.offer(event); before is pointless therefore, ignore this comment then.

apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/ClickHouseAppender.java

andrescrz · 2025-01-20T09:36:05Z

...ik-backend/src/main/java/com/comet/opik/infrastructure/log/UserFacingRuleLoggingFactory.java

+@UtilityClass
+public class UserFacingRuleLoggingFactory {
+
+    private static final LoggerContext CONTEXT = (LoggerContext) LoggerFactory.getILoggerFactory();


Minor: avoid the cast if possible. Ignore otherwise.

This cast is needed

...ik-backend/src/main/java/com/comet/opik/infrastructure/log/UserFacingRuleLoggingFactory.java

…r_logs

thiagohora · 2025-01-20T13:55:24Z

Let's polish the details of this approach and add test coverage.

The test is in the next PR since there were no tests for the online scoring before.

andrescrz

I left some more comments, but this can go as most of them are either minor or can be done in the next PR.

The main one is to cache the DAOs so they're just retrieved and used, instead of instantiated each time.

The rest have less importance.

andrescrz · 2025-01-20T15:52:16Z

apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/ClickHouseAppender.java

+        logQueue = new LinkedBlockingQueue<>();
+        scheduler = Executors.newSingleThreadScheduledExecutor();


Not sure if I follow. If you move it here:

private BlockingQueue<ILoggingEvent> logQueue = new LinkedBlockingQueue<>(); private ScheduledExecutorService scheduler = = Executors.newSingleThreadScheduledExecutor();

Creation of those objects is moved to ClickHouseAppender instantiation time. But you would still schedule only when invoking this start method.

Anyway, this is a minor improvement for separation of responsibilities.

andrescrz · 2025-01-20T15:56:20Z

apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/ClickHouseAppender.java

+    }
+
+    private void flushLogs() {
+        if (logQueue.isEmpty()) return;


I agree, but that's why you control the order in stop method.

However, now that you have shutdownNow with a grace period after shutdown this isn't critical anymore. I agree it's better to allow the scheduler to continue flushing during that grace period.

andrescrz · 2025-01-20T16:10:17Z

apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/ClickHouseAppender.java

+        boolean added = logQueue.offer(event);
+        if (!added) {
+            log.warn("Log queue is full, dropping log: {}", event.getFormattedMessage());
+        }


I see the call below is in the async executor

if (logQueue.size() >= batchSize) { scheduler.execute(this::flushLogs); }

I didn't notice that in my first review.

Moving boolean added = logQueue.offer(event); before is pointless therefore, ignore this comment then.

andrescrz · 2025-01-20T16:11:10Z

apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/ClickHouseAppender.java

+            if (!scheduler.awaitTermination(5, TimeUnit.SECONDS)) {
+                scheduler.shutdownNow();
+                if (!scheduler.awaitTermination(5, TimeUnit.SECONDS)) { // Final attempt


Minor: grace period could be configurable (consider that you wait twice).

andrescrz · 2025-01-20T16:11:45Z

apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/ClickHouseAppender.java

+                    log.error("ClickHouseAppender did not terminate");
+                }
+            }
+        } catch (InterruptedException ex) {


You're missing the Thread.currentThread().interrupt(); call.

andrescrz · 2025-01-20T16:14:13Z

...backend/src/main/java/com/comet/opik/api/resources/v1/events/OnlineScoringEventListener.java

@@ -29,6 +37,7 @@ public class OnlineScoringEventListener {
    private final AutomationRuleEvaluatorService ruleEvaluatorService;
    private final ChatCompletionService aiProxyService;
    private final FeedbackScoreService feedbackScoreService;
+    private final Logger userFacingLogger;


Minor: after looking a the constructor, actually this annotation that I suggested previously doesn't do anything. But thanks for adding the others to the constructor arguments, those indeed validate.

andrescrz · 2025-01-20T16:15:13Z

apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/ClickHouseAppender.java

+        }
+    }
+
+    public static ClickHouseAppender getInstance() {


In your current implementation you always call both in a row. I'd personally just return the instance from init and maybe remove it to getInstance, so you end up with a method that both inits and returns the instance.

andrescrz · 2025-01-20T16:16:57Z

apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/ClickHouseAppender.java

+    }
+
+    private IllegalStateException failWithMessage(String message) {
+        log.error(message);


What about this log:

subscribe( noop -> { }, e -> log.error("Failed to insert logs", e));

andrescrz · 2025-01-20T16:17:13Z

apps/opik-backend/src/main/java/com/comet/opik/infrastructure/log/ClickHouseAppender.java

+
+    @Override
+    protected void append(ILoggingEvent event) {
+        if (!running) return;


You missed a few of these. But not a blocker.

andrescrz · 2025-01-20T16:23:45Z

...opik-backend/src/main/java/com/comet/opik/infrastructure/log/tables/UserLogTableFactory.java

+        return new UserLogTableFactoryImpl(factory);
+    }
+
+    interface UserLogTableDAO {


As a small joke this is probably the only DAO in our service that deserves having an interface.

thiagohora · 2025-01-20T16:34:07Z

e main one is to cache the DAOs so they're just retrieve

I agree; I will merge this one and address this in the following PR.

thiagohora added 2 commits January 16, 2025 12:57

OPIK-795: Create rule logs table

755d147

OPIK-796: Implement clickhouse user facing logs

9fa8f0b

thiagohora requested a review from a team as a code owner January 16, 2025 21:41

thiagohora self-assigned this Jan 16, 2025

idoberko2 reviewed Jan 17, 2025

View reviewed changes

apps/opik-backend/config.yml Outdated Show resolved Hide resolved

idoberko2 reviewed Jan 17, 2025

View reviewed changes

...backend/src/main/java/com/comet/opik/api/resources/v1/events/OnlineScoringEventListener.java Outdated Show resolved Hide resolved

idoberko2 reviewed Jan 17, 2025

View reviewed changes

...backend/src/main/java/com/comet/opik/api/resources/v1/events/OnlineScoringEventListener.java Outdated Show resolved Hide resolved

idoberko2 reviewed Jan 17, 2025

View reviewed changes

Address PR review

e28974e

thiagohora force-pushed the thiagohora/OPIK-796_implement_clickhouse_user_logs branch from 4217cd5 to e28974e Compare January 17, 2025 08:40

OPIK-795: Create rule logs table

e6863eb

thiagohora force-pushed the thiagohora/OPIK-795_create_rule_log_table branch from 755d147 to e6863eb Compare January 17, 2025 08:49

Merge branch 'thiagohora/OPIK-795_create_rule_log_table' into thiagoh…

8b5b7a5

…ora/OPIK-796_implement_clickhouse_user_logs

thiagohora added 2 commits January 17, 2025 12:14

Fix file name and column name

6ab6a14

Fix tests

249d384

thiagohora force-pushed the thiagohora/OPIK-796_implement_clickhouse_user_logs branch from df53b40 to 249d384 Compare January 17, 2025 12:53

thiagohora added 4 commits January 17, 2025 16:22

Change table sortable key

69fc3e3

Merge branch 'thiagohora/OPIK-795_create_rule_log_table' into thiagoh…

f3ac125

…ora/OPIK-796_implement_clickhouse_user_logs

Fix marker field ingestion

22feff1

Merge branch 'thiagohora/OPIK-796_implement_clickhouse_user_logs' of h…

cd06f0e

…ttps://github.com/comet-ml/opik into thiagohora/OPIK-796_implement_clickhouse_user_logs

Base automatically changed from thiagohora/OPIK-795_create_rule_log_table to main January 17, 2025 15:49

thiagohora requested review from idoberko2 and a team January 17, 2025 15:50

Merge branch 'main' into thiagohora/OPIK-796_implement_clickhouse_use…

b43bfa0

…r_logs

andrescrz reviewed Jan 20, 2025

View reviewed changes

Address PR comments

357be97

thiagohora force-pushed the thiagohora/OPIK-796_implement_clickhouse_user_logs branch from f3ef241 to 357be97 Compare January 20, 2025 12:50

thiagohora added 4 commits January 20, 2025 13:55

Merge branch 'main' into thiagohora/OPIK-796_implement_clickhouse_use…

0135816

…r_logs

Fix tests

5a2e109

Extract DAO from log appender

f9245b1

Merge branch 'main' into thiagohora/OPIK-796_implement_clickhouse_use…

2815348

…r_logs

thiagohora requested a review from andrescrz January 20, 2025 13:51

andrescrz approved these changes Jan 20, 2025

View reviewed changes

thiagohora merged commit b003ab1 into main Jan 20, 2025
8 checks passed

thiagohora deleted the thiagohora/OPIK-796_implement_clickhouse_user_logs branch January 20, 2025 16:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OPIK-796: Implement ClickHouse user facing logs #1066

OPIK-796: Implement ClickHouse user facing logs #1066

thiagohora commented Jan 16, 2025 •

edited

Loading

idoberko2 left a comment

thiagohora commented Jan 17, 2025

andrescrz commented Jan 17, 2025

thiagohora commented Jan 17, 2025

andrescrz left a comment

andrescrz Jan 20, 2025

andrescrz Jan 20, 2025

andrescrz Jan 20, 2025

thiagohora Jan 20, 2025

andrescrz Jan 20, 2025

thiagohora Jan 20, 2025

andrescrz Jan 20, 2025

andrescrz Jan 20, 2025

andrescrz Jan 20, 2025

thiagohora Jan 20, 2025

andrescrz Jan 20, 2025

thiagohora Jan 20, 2025

andrescrz Jan 20, 2025

andrescrz Jan 20, 2025

thiagohora Jan 20, 2025

thiagohora commented Jan 20, 2025

andrescrz left a comment •

edited

Loading

andrescrz Jan 20, 2025

andrescrz Jan 20, 2025 •

edited

Loading

andrescrz Jan 20, 2025

andrescrz Jan 20, 2025

andrescrz Jan 20, 2025

andrescrz Jan 20, 2025

andrescrz Jan 20, 2025

andrescrz Jan 20, 2025

andrescrz Jan 20, 2025

andrescrz Jan 20, 2025

thiagohora commented Jan 20, 2025

		import ch.qos.logback.classic.spi.ILoggingEvent;
		import ch.qos.logback.core.AppenderBase;

		logQueue = new LinkedBlockingQueue<>();
		scheduler = Executors.newSingleThreadScheduledExecutor();

OPIK-796: Implement ClickHouse user facing logs #1066

OPIK-796: Implement ClickHouse user facing logs #1066

Conversation

thiagohora commented Jan 16, 2025 • edited Loading

Details

Issues

idoberko2 left a comment

Choose a reason for hiding this comment

thiagohora commented Jan 17, 2025

andrescrz commented Jan 17, 2025

thiagohora commented Jan 17, 2025

andrescrz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thiagohora commented Jan 20, 2025

andrescrz left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrescrz Jan 20, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thiagohora commented Jan 20, 2025

thiagohora commented Jan 16, 2025 •

edited

Loading

andrescrz left a comment •

edited

Loading

andrescrz Jan 20, 2025 •

edited

Loading