OPIK-1764: Add feedback scores endpoint #2506

thiagohora · 2025-06-18T09:37:13Z

Details

Add feedback scores endpoint

…hiagohora/OPIK-1765_make_threads_first_class_citizens

… into thiagohora/OPIK-1765_implement_closing_thread_mechanism

…ueue

github-actions · 2025-06-18T09:48:23Z

Backend Tests Results

177 files 177 suites 17m 56s ⏱️
3 845 tests 3 842 ✅ 3 💤 0 ❌
3 836 runs 3 833 ✅ 3 💤 0 ❌

Results for commit 0fae314.

♻️ This comment has been updated with latest results.

github-actions · 2025-06-19T16:54:21Z

SDK E2E Tests Results

0 tests 0 ✅ 0s ⏱️
0 suites 0 💤
0 files 0 ❌

Results for commit 27a002b.

github-actions · 2025-06-19T16:54:23Z

SDK E2E Tests Results

0 tests 0 ✅ 0s ⏱️
0 suites 0 💤
0 files 0 ❌

Results for commit 27a002b.

♻️ This comment has been updated with latest results.

…points

.../opik-backend/src/main/java/com/comet/opik/api/resources/v1/jobs/TraceThreadsClosingJob.java

YarivHashaiComet · 2025-06-22T06:34:10Z

apps/opik-backend/src/main/java/com/comet/opik/api/resources/v1/priv/TracesResource.java

+        var workspaceId = requestContext.get().getWorkspaceId();
+        String projectName = scores.projectName();
+
+        Optional<Project> project = projectService.findByNames(workspaceId, List.of(projectName)).stream().findFirst();


we have to find a way to hide the logic in the service (here we have another point where logic been done on the resources level) - any suggestions? - maybe adding a facade?

We have cases where we use a facade, for instance, the experiment build API, which handles many domain services. We need to be careful when adding services as dependencies of other services. As a rule, it's not critical, but this frequently leads to cycle dependencies.< 10000 /p>
In this case, I will change, but before implementing such a practice, I would open a discussion with the team. In cases like this, it doesn't add much value to introducing facades. Usually, facade coordinates complex operations across different domain operations, simplifies the APIs, and reduces the complexity of dealing with such operations. In this case, we are referring to a simple 'fail fast' exit using an already existing dependency of the controller. Again, no big deal changing it, but it's been suggested before starting to implement it in all system.

YarivHashaiComet · 2025-06-22T06:44:05Z

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

+@Valid public record FeedbackScoreBatch(
+        @NotNull @Size(min = 1, max = 1000) List<@Valid FeedbackScoreBatchItem> scores) implements RateEventContainer {
+
+    public static class View {


can you share more info about this hierarchy?

This is a JSON view, which is used extensively throughout the system. It allows us to use the same Pojo, but with different JSON representations of it. In this case, it will enable us to use the same FeedbackScoreBatchItem POJO used across the system, but adding the `thread_id' to it. This is only required here; for all other endpoints, this property is not exposed to the client.

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

YarivHashaiComet · 2025-06-22T06:50:52Z

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

+    }
+
+    @Override
+    public Mono<UUID> getThreadModelId(@NonNull UUID projectId, @NonNull String threadId) {


what dose the "Model" stands for? (method name)

The model is a Java Pojo that represents the persistence view of the domain object. In this case, the threadId already represents the external ID used by the client, and the threadModelId represents the internal Opik ID, which identifies the thread from the Opik side.

YarivHashaiComet

added comments

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

YarivHashaiComet · 2025-06-22T06:58:08Z

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

+                            .map(threadIdMap -> bindThreadModelId(projectDto, threadIdMap))
+                            .filter(projectDtoWithThreads -> !projectDtoWithThreads.scores().isEmpty())
+                            // score the batch of threads with resolved thread model IDs
+                            .flatMap(score -> dao.scoreBatchOfThreads(score.scores()));


plz rename the dao to spesifi which dao is been used.

Again, I would recommend that we discuss this practice with the team, since we likely have different opinions about it, and it is currently being used in other parts of the system. This is the FeedbackScoresService, so contextually, I believe it is simple to infer that the DAO it relates to is FeedbackScores, unless the name specifies otherwise.

YarivHashaiComet · 2025-06-22T07:02:19Z

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

+                .map(threadModelId -> Map.entry(threadId, threadModelId));
+    }
+
+    private ProjectDto bindThreadModelId(ProjectDto projectDto, Map<String, UUID> threadIdMap) {


dose this method is generating projectDTO with score and threadID? can we have more self explanatory name for this method?

No, if we check the implementation, we only copy the object and add the threadModelId to it since it's an immutable object.

...es/liquibase/db-app-analytics/migrations/000029_add_thread_enum_value_to_feedback_scores.sql

...k-backend/src/test/java/com/comet/opik/api/resources/v1/jobs/TraceThreadsClosingJobTest.java

Copilot

Pull Request Overview

Enhances feedback scoring by introducing thread-level scoring and deletion endpoints, updates the domain and data layers to handle thread entities, and augments the API and client models.

Add “thread” entity type in EntityType, Liquibase migration, DAO, and service layers
Implement new REST endpoints in TracesResource for batch scoring and deleting thread feedback
Extend client utilities, API models (FeedbackScoreBatch, FeedbackScoreBatchItem, DeleteThreadFeedbackScores), and tests for thread support

Reviewed Changes

Copilot reviewed 13 out of 14 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
`TraceResourceClient.java`	Added test client methods for thread feedback endpoints
`000029_add_thread_enum_value_to_feedback_scores.sql`	Liquibase migration: add `thread` to feedback_scores ENUM
`TraceThreadService.java`	New interface methods for retrieving/creating thread IDs
`TraceThreadIdService.java`	Implemented `getThreadModelId` with caching and error handling
`FeedbackScoreService.java`	Added batch scoring & deletion logic for thread feedback
`FeedbackScoreDAO.java`	DAO methods for thread score batch and deletion by name
`EntityType.java`	Introduced `THREAD` enum value
`TracesResource.java`	Exposed PUT/POST endpoints for thread feedback scoring/deletion
`SpansResource.java`	Updated `@JsonView` on span batch scoring endpoint
`FeedbackScoreBatchItem.java`	Extended record with `threadId` and view-based validation
`FeedbackScoreBatch.java`	Added validation groups for tracing vs. thread views
`DeleteThreadFeedbackScores.java`	New request record for deleting thread feedback scores
`DeleteFeedbackScore.java`	Removed validation on `name` field

Comments suppressed due to low confidence (3)

apps/opik-backend/src/main/java/com/comet/opik/api/DeleteFeedbackScore.java:11

Re-add a validation constraint like @NotBlank or @NotNull on the name field to prevent invalid delete requests.

public record DeleteFeedbackScore(String name) {

apps/opik-backend/src/main/java/com/comet/opik/domain/threads/TraceThreadIdService.java:58

Add the missing imports for com.google.common.base.Preconditions and org.apache.commons.lang3.StringUtils to avoid compilation errors.

        Preconditions.checkArgument(!StringUtils.isBlank(workspaceId), "Workspace ID cannot be blank");

apps/opik-backend/src/main/java/com/comet/opik/domain/threads/TraceThreadIdService.java:59

Ensure that Preconditions and StringUtils are imported so these argument checks compile correctly.

        Preconditions.checkArgument(!StringUtils.isBlank(threadId), "Thread ID cannot be blank");

BorisTkachenko

Looks good, a few minor comments

BorisTkachenko · 2025-06-23T09:58:59Z

apps/opik-backend/src/main/java/com/comet/opik/api/resources/v1/priv/TracesResource.java

@@ -689,4 +691,50 @@ public Response closeTraceThread(
        return Response.noContent().build();
    }

+    @PUT
+    @Path("/threads/feedback-scores")


Maybe we want to create a new Resource class for Threads instead of adding them to Traces?

BorisTkachenko · 2025-06-23T10:22:55Z

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

@@ -171,4 +183,112 @@ public Mono<FeedbackScoreNames> getProjectsFeedbackScoreNames(Set<UUID> projectI
                .map(names -> names.stream().map(FeedbackScoreNames.ScoreName::new).toList())
                .map(FeedbackScoreNames::new);
    }
+
+    @Override
+    public Mono<Void> scoreBatchOfThreads(List<FeedbackScoreBatchItem> scores) {


Suggested change

public Mono<Void> scoreBatchOfThreads(List<FeedbackScoreBatchItem> scores) {

public Mono<Void> scoreBatchOfThreads(@NonNull List<FeedbackScoreBatchItem> scores) {

BorisTkachenko · 2025-06-23T10:30:23Z

apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/TracesResourceTest.java

+            String score1 = RandomStringUtils.secure().nextAlphanumeric(10);
+            String score2 = RandomStringUtils.secure().nextAlphanumeric(10);
+            String score3 = RandomStringUtils.secure().nextAlphanumeric(10);
+
+            scoreItems.set(0, scoreItems.get(0).toBuilder()
+                    .threadId(threadId)
+                    .projectName(projectName)
+                    .projectId(null)
+                    .name(score1)
+                    .build());
+
+            scoreItems.set(1, scoreItems.get(1).toBuilder()
+                    .threadId(threadId)
+                    .projectName(projectName)
+                    .projectId(null)
+                    .name(score2)
+                    .build());
+
+            scoreItems.set(2, scoreItems.get(2).toBuilder()
+                    .threadId(threadId)
+                    .projectName(projectName)
+                    .projectId(null)
+                    .name(score3)
+                    .build());


Can we use some for loop or stream for this?

This one doesn't seem worth it since we are changing the object generated by PodamFactoryUtils.manufacturePojoList

thiagohora · 2025-06-23T10:31:31Z

Looks good, a few minor comments

Thanks! I will address those comments in the following PR

thiagohora added 22 commits June 12, 2025 09:33

OPIK-1765: Make threads first class citizens

a208042

Add tests

f80985a

OPIK-1765: Create trace thread tables

f480940

Merge branch 'thiagohora/OPIK-1765_create_trace_thread_tables' into t…

48f6df1

…hiagohora/OPIK-1765_make_threads_first_class_citizens

Fix logs and block statement

719cb25

OPIK-1765: Implement closing trace threads mechanism

9d16bc8

Remove reopen logic

16afcfc

OPIK-1765: Make threads first class citizens

650e4af

Add tests

d94cd42

Fix logs and block statement

f2c562a

Fix last updated at

92d9b4c

Fix name

3277b89

Fix precision

f0555ff

truncate last_updated_at

da31b8b

Merge branch 'thiagohora/OPIK-1765_make_threads_first_class_citizens'…

08e2d2a

… into thiagohora/OPIK-1765_implement_closing_thread_mechanism

Add tests

30bca98

Fix precision

7c60193

Format code

c5c1090

Merge branch 'thiagohora/OPIK-1765_make_threads_first_class_citizens'…

139797b

… into thiagohora/OPIK-1765_implement_closing_thread_mechanism

OPIK-1765: Improve implementation to prevent duplicated messages in q…

d7efe05

…ueue

OPIK-1764: Add endpoint to Open/Close threads manually

8f82187

OPIK-1764: Add feedback scores endpoint

cabea6f

thiagohora force-pushed the thiagohora/OPIK-1764_add_endpoint_to_open_close_threads_manually branch 2 times, most recently from 803ac15 to 5eaf057 Compare June 19, 2025 16:23

Base automatically changed from thiagohora/OPIK-1764_add_endpoint_to_open_close_threads_manually to main June 19, 2025 16:42

thiagohora marked this pull request as ready for review June 19, 2025 16:46

thiagohora requested a review from a team as a code owner June 19, 2025 16:46

Merge branch 'main' into thiagohora/OPIK-1764_add_feedback_scores_end…

9ffb541

…points

thiagohora force-pushed the thiagohora/OPIK-1764_add_feedback_scores_endpoints branch from 27a002b to 9ffb541 Compare June 19, 2025 16:54

YarivHashaiComet reviewed Jun 22, 2025

View reviewed changes

.../opik-backend/src/main/java/com/comet/opik/api/resources/v1/jobs/TraceThreadsClosingJob.java Outdated Show resolved Hide resolved

YarivHashaiComet reviewed Jun 22, 2025

View reviewed changes

apps/opik-backend/src/main/java/com/comet/opik/domain/threads/TraceThreadIdService.java Outdated Show resolved Hide resolved

YarivHashaiComet reviewed Jun 22, 2025

View reviewed changes

thiagohora force-pushed the thiagohora/OPIK-1764_add_feedback_scores_endpoints branch from da5dafa to 0a748a8 Compare June 23, 2025 07:52

thiagohora requested a review from YarivHashaiComet June 23, 2025 08:20

thiagohora self-assigned this Jun 23, 2025

thiagohora requested a review from Copilot June 23, 2025 08:24

Copilot AI reviewed Jun 23, 2025

View reviewed changes

Address review comments

0fae314

thiagohora force-pushed the thiagohora/OPIK-1764_add_feedback_scores_endpoints branch from 0a748a8 to 0fae314 Compare June 23, 2025 09:42

BorisTkachenko approved these changes Jun 23, 2025

View reviewed changes

thiagohora merged commit 39dc460 into main Jun 23, 2025
12 checks passed

thiagohora deleted the thiagohora/OPIK-1764_add_feedback_scores_endpoints branch June 23, 2025 10:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

OPIK-1764: Add feedback scores endpoint #2506

OPIK-1764: Add feedback scores endpoint #2506

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

	public Mono<Void> scoreBatchOfThreads(List<FeedbackScoreBatchItem> scores) {
	public Mono<Void> scoreBatchOfThreads(@NonNull List<FeedbackScoreBatchItem> scores) {

OPIK-1764: Add feedback scores endpoint #2506

OPIK-1764: Add feedback scores endpoint #2506

Uh oh!

Conversation

Uh oh!

Details

Uh oh!

Uh oh!

Backend Tests Results

Uh oh!

SDK E2E Tests Results

Uh oh!

Uh oh!

SDK E2E Tests Results

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!