[OPIK-294] Use application events to fill last experiment created at on datasets table. #511

thiagohora · 2024-10-30T10:58:21Z

Details

Adding application events capabilities. Ideally, we could move this implementation to Redis, but the guava implementation was simpler but less resilient.
Add open telemetry tracking to events
Fixing MySQL test container setup to use UTC as Prod and other environments.
Implement Events to update datasets last_experiment_created_at field when experiments are created or deleted.

Issues

OPIK-294

Testing

Updating the last_experiment_created_at field when an experiment is created
Updating the last_experiment_created_at field when an experiment is deleted
Updating the last_experiment_created_at field to null when an experiment is deleted and no other experiments exist.

…field in datasets table

…o_fill_last_experiment_created_t

…_experiment_created_t' of https://github.com/comet-ml/opik into thiagohora/OPIK-294_use_application_events_to_fill_last_experiment_created_t

andrescrz

The majority of comments are minor. Only conventions or things that in theory require some low cost polishing, but that are fine to go in order to favour velocity.

Regarding the instrumentation on the event bus, I'm wondering if it was worthy the effort. But I'm fine to go with it.

My main concern is triggering many small updates on the deletion of experiments. I think it can quickly degrade performance even with very few users. We likely want to double check.

Having said that, I'm happy to move forward for the sake of velocity.

apps/opik-backend/pom.xml

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

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

andrescrz · 2024-10-30T15:31:47Z

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

+    public ExperimentCreated(UUID experimentId, UUID datasetId, Instant createdAt, String workspaceId,
+            String userName) {
+        super(workspaceId, userName);
+        this.experimentId = experimentId;
+        this.datasetId = datasetId;
+        this.createdAt = createdAt;
+    }


Minor: boiler plate constructor, better defining it with Lombok.

Not possible due to the super constructor

andrescrz · 2024-10-30T15:33:38Z

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

+    public DatasetEventListener(EventBus eventBus, DatasetService datasetService, ExperimentService experimentService) {
+        this.datasetService = datasetService;
+        this.experimentService = experimentService;
+        eventBus.register(this);


Minor: better register with EventBus somewhere else, for instance in a module or some DI hook.

I will try to find a better way in the next PR. The problem is that beans are lazy by default, and I think migrating to the module would need to build the Listener, which may involve domain dependencies. I would like to avoid that

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

andrescrz · 2024-10-30T15:52:16Z

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

+    }
+
+    @WithSpan
+    public Mono<List<ExperimentDatasetId>> getExperimentsDatasetIds(Set<UUID> ids) {


Minor: most likely you want to return a flux.

If I do this, I would need to collect immediately because we want to publish a single event with all dataset IDs from the deleted experiments.

And that's fine. The DAO should be more neutral (get arguments, produce neutral response). That type of aggregations are usually better in the service layer, as they're kind of business logic. Anyway, a minor comment.

andrescrz · 2024-10-30T15:53:51Z

apps/opik-backend/src/main/java/com/comet/opik/infrastructure/events/BaseEvent.java

+    protected final String userName;
+
+    protected BaseEvent(String workspaceId, String userName) {
+        this.traceId = Span.current().getSpanContext().getTraceId();


Minor: probably better init in the field declaration. So you can use Lombok constructor. Anyway, it's strange to use OTEL stuff as field for a POJO representing an event. Telemetry shouldn't be invasive

I added it because sometimes we see traces without their root endpoint, and this can help us find them. I just didn't want to call Span.current().getSpanContext().getTraceId() in all the places we publish an event.

apps/opik-backend/src/main/java/com/comet/opik/infrastructure/events/BaseEvent.java

andrescrz · 2024-10-30T15:58:28Z

apps/opik-backend/src/main/java/com/comet/opik/infrastructure/events/EventInterceptor.java

+import static com.comet.opik.infrastructure.instrumentation.InstrumentAsyncUtils.TRACER_NAME;
+
+@Slf4j
+class EventInterceptor implements MethodInterceptor {


Not sure why we need this telemetry part. If the service and DAO methods are well annotated with instrumentation, it might not be necessary to add telemetry to the events itself. Also, maybe the guava Event bus provides telemetry already or it can be simple wrapped with it.

Anyway, I'm fine with this. I was just wondering if it was worthy the effort.

I looked at agent documentation: https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/docs/supported-libraries.md#libraries--frameworks

It doesn't mention the Guava Event Bus. I added it so we could have a proper span for the event's execution as it runs async.

Without instrumentation, we see this:

With this, we see this:

This looks very nice!

…o_fill_last_experiment_created_t

andrescrz

This is ready to go from my side.

andrescrz · 2024-10-31T09:17:18Z

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

@@ -76,4 +77,18 @@ List<Dataset> find(@Bind("limit") int limit,
    @SqlQuery("SELECT * FROM datasets WHERE workspace_id = :workspace_id AND name = :name")
    Optional<Dataset> findByName(@Bind("workspace_id") String workspaceId, @Bind("name") String name);

+    default int[] recordExperiments(Handle handle, String workspaceId,


Not a blocker: you're mixing the fluent and declarative interfaces of JDBI. I believe you can implement batches with the following annotation: https://jdbi.org/#_sqlbatch

I tried, but it seems this only works for inserts.

I can try again later, but the bind of the collection didn't work
https://jdbi.org/#_prepared_batches

andrescrz · 2024-10-31T09:19:22Z

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

+    }
+
+    @WithSpan
+    public Mono<List<ExperimentDatasetId>> getExperimentsDatasetIds(Set<UUID> ids) {


And that's fine. The DAO should be more neutral (get arguments, produce neutral response). That type of aggregations are usually better in the service layer, as they're kind of business logic. Anyway, a minor comment.

andrescrz · 2024-10-31T09:20:33Z

apps/opik-backend/src/main/java/com/comet/opik/infrastructure/events/BaseEvent.java

+
+@Getter
+@Accessors(fluent = true)
+public abstract class BaseEvent {


It wouldn't be a bad idea. But this is ok too.

andrescrz · 2024-10-31T09:21:05Z

apps/opik-backend/src/main/java/com/comet/opik/infrastructure/events/EventInterceptor.java

+import static com.comet.opik.infrastructure.instrumentation.InstrumentAsyncUtils.TRACER_NAME;
+
+@Slf4j
+class EventInterceptor implements MethodInterceptor {


This looks very nice!

andrescrz · 2024-10-31T09:26:36Z

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

+                    .doOnSuccess(newExperiment -> eventBus.post(new ExperimentCreated(
+                            newExperiment.id(),
+                            newExperiment.datasetId(),
+                            newExperiment.createdAt(),
+                            workspaceId,
+                            userName)));


What about @SuperBuilder?

…o_fill_last_experiment_created_t

thiagohora added 4 commits October 28, 2024 14:41

Initial commit

4cd3596

Merge branch 'main' into thiagohora/events

aa4e09a

[OPIK-294] Use application events to fill last experiment created at …

1b3ab39

…field in datasets table

Add delete experiment event

bac6d0f

thiagohora requested a review from a team as a code owner October 30, 2024 10:58

thiagohora self-assigned this Oct 30, 2024

thiagohora added 4 commits October 30, 2024 11:58

Merge branch 'main' into thiagohora/OPIK-294_use_application_events_t…

6f2dbfc

…o_fill_last_experiment_created_t

Fix

9ff5f3a

Merge branch 'thiagohora/OPIK-294_use_application_events_to_fill_last…

eef27f8

…_experiment_created_t' of https://github.com/comet-ml/opik into thiagohora/OPIK-294_use_application_events_to_fill_last_experiment_created_t

Make events async

e0d3628

andrescrz reviewed Oct 30, 2024

View reviewed changes

Addressing PR reviews

06283e5

thiagohora force-pushed the thiagohora/OPIK-294_use_application_events_to_fill_last_experiment_created_t branch from 73bc106 to 06283e5 Compare October 30, 2024 17:58

thiagohora requested a review from andrescrz October 30, 2024 18:02

thiagohora added 2 commits October 30, 2024 21:10

Merge branch 'main' into thiagohora/OPIK-294_use_application_events_t…

645fb46

…o_fill_last_experiment_created_t

Merge branch 'main' into thiagohora/OPIK-294_use_application_events_t…

70a1cb0

…o_fill_last_experiment_created_t

andrescrz previously approved these changes Oct 31, 2024

View reviewed changes

Merge branch 'main' into thiagohora/OPIK-294_use_application_events_t…

fd05c05

…o_fill_last_experiment_created_t

thiagohora dismissed andrescrz’s stale review via fd05c05 October 31, 2024 09:46

thiagohora requested a review from andrescrz October 31, 2024 09:51

andrescrz approved these changes Oct 31, 2024

View reviewed changes

thiagohora merged commit 7ce9bbc into main Oct 31, 2024
7 checks passed

thiagohora deleted the thiagohora/OPIK-294_use_application_events_to_fill_last_experiment_created_t branch October 31, 2024 09:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[OPIK-294] Use application events to fill last experiment created at on datasets table. #511

[OPIK-294] Use application events to fill last experiment created at on datasets table. #511

thiagohora commented Oct 30, 2024

andrescrz left a comment

andrescrz Oct 30, 2024

thiagohora Oct 30, 2024

andrescrz Oct 30, 2024

thiagohora Oct 30, 2024

andrescrz Oct 30, 2024

thiagohora Oct 30, 2024

andrescrz Oct 31, 2024

andrescrz Oct 30, 2024

thiagohora Oct 30, 2024

andrescrz Oct 30, 2024

thiagohora Oct 30, 2024

andrescrz Oct 31, 2024

andrescrz left a comment

andrescrz Oct 31, 2024

thiagohora Oct 31, 2024

andrescrz Oct 31, 2024

andrescrz Oct 31, 2024

andrescrz Oct 31, 2024

andrescrz Oct 31, 2024

[OPIK-294] Use application events to fill last experiment created at on datasets table. #511

[OPIK-294] Use application events to fill last experiment created at on datasets table. #511

Conversation

thiagohora commented Oct 30, 2024

Details

Issues

Testing

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

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