[OPIK-797] Implement get log API #1079

thiagohora · 2025-01-20T11:12:50Z

Details

Implement get log endpoint

This PR also unveiled two issues:

The LLM providers' setup was based on components, not modules, which made it very hard to replace beans to mock them and avoid calling the actual LLM provider server.
The new Redis stream consumer needs to follow the application lifecycle. For some reason, the consumer is not automatically deleted once the application's context is shut down, which caused side effects between tests. The solution was to make the consumer implement the Managed interface, which added the bean to the application lifecycle and allowed us to close the stream during the application shutdown.

Issues

OPIK-797

…ora/OPIK-796_implement_clickhouse_user_logs

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

…r_logs

…o thiagohora/OPIK-796_implement_get_log_api

ldaugusto

Looks very good, I'd like some extra logging for easier debugging, but it's a great work and easy to extend no other log types.

...end/src/main/java/com/comet/opik/api/resources/v1/priv/AutomationRuleEvaluatorsResource.java

ldaugusto · 2025-01-21T11:57:03Z

...end/src/main/java/com/comet/opik/api/resources/v1/priv/AutomationRuleEvaluatorsResource.java

+                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(),


trivial: missing whitespace

Sorry, where?

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

ldaugusto · 2025-01-21T12:12:45Z

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

-    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) {


maybe an else log to know its returning the existent instance?

This "if" only init the instance; the final line always returns the instance.

I mean it means init(...) was called multiple times. Maybe it can be useful to know it was called again and its not creating an instance but reusing the existent one - unless its happening way to often. Maybe the naming sugests me a 2nd execution would be unexpected. If it was called 'getOrCreate' I would expect multiple calls naturaly.

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

ldaugusto · 2025-01-21T12:14:06Z

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

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


use final too?

The scheduler cannot be final due to tests, where a JVM instance the application multiple times. When this happened the scheduler is shutdown after each test, so we need a new one

Make it volatile then.

I used atomic reference, as sonar complains about using volatile in this case.

andrescrz

Please reuse the existing DAO.

andrescrz · 2025-01-21T14:51:49Z

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

+import static com.comet.opik.api.LogItem.LogPage;
+
+@ImplementedBy(AutomationRuleEvaluatorLogsDAOImpl.class)
+public interface AutomationRuleEvaluatorLogsDAO {


Please re-use the existing DAO: AutomationRuleEvaluatorLogDAO

andrescrz · 2025-01-21T15:03:39Z

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

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


Make it volatile then.

…lication lifecycle

…ithub.com/comet-ml/opik into thiagohora/OPIK-796_implement_get_log_api

andrescrz · 2025-01-22T09:54:28Z

Let's break this PR in two parts:

The fixes for the tests.
The get log API implementation.

This is hard to review and it will hard to identify an issue during deployment and to rollback it.

thiagohora · 2025-01-22T16:48:13Z

Closing to favor the one which splits scope

thiagohora and others added 13 commits January 16, 2025 12:57

OPIK-795: Create rule logs table

755d147

OPIK-796: Implement clickhouse user facing logs

9fa8f0b

Address PR review

e28974e

OPIK-795: Create rule logs table

e6863eb

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

8b5b7a5

…ora/OPIK-796_implement_clickhouse_user_logs

Fix file name and column name

6ab6a14

Fix tests

249d384

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

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

b43bfa0

…r_logs

[OPIK-796] Implement get log API

a400485

thiagohora requested a review from a team as a code owner January 20, 2025 11:12

thiagohora added 6 commits January 20, 2025 13:49

Address PR comments

357be97

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

Merge branch 'thiagohora/OPIK-796_implement_clickhouse_user_logs' int…

fa8e002

…o thiagohora/OPIK-796_implement_get_log_api

Base automatically changed from thiagohora/OPIK-796_implement_clickhouse_user_logs to main January 20, 2025 16:34

thiagohora added 2 commits January 20, 2025 17:55

Address PR review

49d1ae4

Merge branch 'main' into thiagohora/OPIK-796_implement_get_log_api

6ba985b

thiagohora force-pushed the thiagohora/OPIK-796_implement_get_log_api branch from 0bd37c9 to 6ba985b Compare January 20, 2025 17:04

thiagohora self-assigned this Jan 20, 2025

thiagohora added 2 commits January 21, 2025 09:55

Merge branch 'main' into thiagohora/OPIK-796_implement_get_log_api

9424a28

Merge branch 'main' into thiagohora/OPIK-796_implement_get_log_api

e466b80

ldaugusto requested changes Jan 21, 2025

View reviewed changes

andrescrz requested changes Jan 21, 2025

View reviewed changes

Make LLM Providers configurable via tests and add judge scorer to app…

b9476b6

…lication lifecycle

Merge branch 'thiagohora/OPIK-796_implement_get_log_api' of https://g…

48afa67

…ithub.com/comet-ml/opik into thiagohora/OPIK-796_implement_get_log_api

thiagohora requested review from ldaugusto and andrescrz January 21, 2025 19:42

Fix references

57f232e

thiagohora force-pushed the thiagohora/OPIK-796_implement_get_log_api branch from cfffdc9 to 57f232e Compare January 21, 2025 20:13

thiagohora added 2 commits January 22, 2025 09:25

Fix log message

df6a3df

Reuse DAO

7633e65

thiagohora closed this Jan 22, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[OPIK-797] Implement get log API #1079

[OPIK-797] Implement get log API #1079

thiagohora commented Jan 20, 2025 •

edited

Loading

ldaugusto left a comment

ldaugusto Jan 21, 2025

thiagohora Jan 21, 2025

ldaugusto Jan 21, 2025

thiagohora Jan 21, 2025

ldaugusto Jan 21, 2025

ldaugusto Jan 21, 2025

thiagohora Jan 21, 2025

andrescrz Jan 21, 2025

thiagohora Jan 22, 2025

andrescrz left a comment

andrescrz Jan 21, 2025

thiagohora Jan 22, 2025

andrescrz Jan 21, 2025

andrescrz commented Jan 22, 2025

thiagohora commented Jan 22, 2025

[OPIK-797] Implement get log API #1079

[OPIK-797] Implement get log API #1079

Conversation

thiagohora commented Jan 20, 2025 • edited Loading

Details

Issues

ldaugusto 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

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

andrescrz commented Jan 22, 2025

thiagohora commented Jan 22, 2025

thiagohora commented Jan 20, 2025 •

edited

Loading