[issue-3494] [FE] [BE] Ability to add tags to multiple threads #3494 #3715
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds the ability to batch update tags for multiple threads, addressing feature request #3494. The implementation follows the existing pattern used for traces and spans, providing a consistent user experience across all entity types.
Key Changes
- Added batch update API endpoint and service layer methods to support updating multiple threads simultaneously
- Extended the frontend UI to include an "Add tags" button in the Threads tab with dialog functionality
- Implemented comprehensive test coverage for batch update scenarios including empty lists and existing tags
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/opik-frontend/src/hooks/useTracesOrSpansList.ts | Added threads enum value to support thread-specific operations |
| apps/opik-frontend/src/components/pages/TracesPage/ThreadsTab/ThreadsTab.tsx | Added callback to clear row selection after tag operations |
| apps/opik-frontend/src/components/pages/TracesPage/ThreadsTab/ThreadsActionsPanel.tsx | Integrated Add Tag dialog and button for threads |
| apps/opik-frontend/src/components/pages-shared/traces/AddTagDialog/AddTagDialog.tsx | Extended dialog to handle thread batch updates with tag merging logic |
| apps/opik-frontend/src/api/traces/useThreadBatchUpdateMutation.ts | Created mutation hook for batch thread updates API calls |
| apps/opik-backend/src/test/java/.../TracesResourceTest.java | Added comprehensive unit tests for batch update functionality |
| apps/opik-backend/src/test/java/.../TraceResourceClient.java | Added test client method for batch update endpoint |
| apps/opik-backend/src/main/java/.../TraceThreadService.java | Implemented service layer batch update logic with validation |
| apps/opik-backend/src/main/java/.../TraceThreadDAO.java | Added DAO method with SQL template for batch thread updates |
| apps/opik-backend/src/main/java/.../TracesResource.java | Created REST endpoint for batch thread updates |
| apps/opik-backend/src/main/java/.../TraceThreadBatchUpdate.java | Defined request DTO with validation constraints |
| toast({ | ||
| title: "Threads updated", | ||
| description: "Tags have been successfully updated", | ||
| }); |
There was a problem hiding this comment.
Duplicate success toast: The mutation's onSuccess displays a generic toast, but AddTagDialog already shows a more specific toast (Tag "${newTag}" added to ${rows.length} selected threads). Remove this duplicate toast from the mutation hook to avoid showing two toasts for the same action.
| template.add("tags", tags.toString()); | ||
| log.debug("DAO: Added tags to template: '{}'", tags); |
There was a problem hiding this comment.
Using tags.toString() for the template will produce incorrect SQL syntax. The toString() method on a Set produces a string like [tag1, tag2] with brackets, which won't work in SQL. Remove this line as the tags are already bound as an array parameter in lines 571-575.
| template.add("tags", tags.toString()); | |
| log.debug("DAO: Added tags to template: '{}'", tags); | |
| log.debug("DAO: Tags present for batch update: '{}'", tags); |
apps/opik-backend/src/main/java/com/comet/opik/api/TraceThreadBatchUpdate.java
Show resolved
Hide resolved
|
Hi @mansi0829 , thanks for your contribution, Ive approved workflows to run on this PR. please make sure it passed all internal check and use the AI code review as well. After all is passing we will work on this review and test |
…BatchUpdate.java Co-authored-by: Copilot <[email protected]>
Thanks @Nimrod007 will this take time since i dont see pipeline has started yet can you confirm on this |
@mansi0829 they started running, do you see which checks are failing / running ? |
@Nimrod007 do you have to approve after each commit i add have added few lint related fix but pipeline is not running now |
@mansi0829 will check, it used to work after Ive approved once. |
Nimrod007
changed the title
There was a problem hiding this comment.
Hi @mansi0829,
Thank you very much for your contribution! I initially focused on reviewing the backend portion, and it looks very promising—it's not far from being ready.
I've left some comments and guidance on areas that could use a bit more attention in the next revision. Please feel free to reach out if you have any questions or need further clarification.
Thanks again for your hard work!
| log.info("Resource: Batch updating '{}' threads on workspace_id: '{}', user: '{}', update: '{}'", | ||
| batchUpdate.threadModelIds().size(), workspaceId, userName, batchUpdate.update()); |
There was a problem hiding this comment.
Minor: let's not print TraceThreadUpdate object in these logs, as it can be very noisy (and leak sensitive information in the future).
Additionally, I'd move the printing of the size together with all parameters at the end of the sentence. This is to facilitate log searching when debugging production issues.
This comment applies to the other logs below and in the rest of the PR.
There was a problem hiding this comment.
Hi @andrescrz sure, agree on this will move logs accordingly
| ; | ||
| """; | ||
|
|
||
| private static final String BATCH_UPDATE_THREADS_SQL = """ |
There was a problem hiding this comment.
For this query and all the related methods in this DAO, the service etc. There's a minimal difference, which is allowing multiple ids instead of one.
Let's just update the existing query and code to allow that:
- Update the original query where clause to
id IN :ids. - Update the methods to accept a Set of UUIDs instead of single one.
2.1 You can keep the old methods, but overload them to just pass a set with a single UUID.
| } | ||
|
|
||
| @Test | ||
| void batchUpdateThreads() { |
There was a problem hiding this comment.
Let's create a new separated subclass for these new tests. It's a different endpoint, so we usually follow that convention.
| var thread1 = createThread(); | ||
| var thread2 = createThread(); | ||
| var thread3 = createThread(); |
There was a problem hiding this comment.
Not a blocker: I'd find or create a flavour of these createThread methods to do it in batches. Test would be faster and more maintainable.
| // Check that they don't have tags | ||
| assertThat(thread1.tags()).isNull(); | ||
| assertThat(thread2.tags()).isNull(); | ||
| assertThat(thread3.tags()).isNull(); |
There was a problem hiding this comment.
Minor: better keep a list of threads and iterate on all of them to assert this.
| assertThat(thread3.tags()).isNull(); | ||
|
|
||
| // Batch update threads | ||
| var tags = Set.of("tag1", "tag2", "tag3"); |
There was a problem hiding this comment.
Please use podam factory to just generate a random TraceThreadUpdate.
| var actualThread1 = traceResourceClient.getTraceThread(thread1.id(), thread1.projectId(), API_KEY, | ||
| TEST_WORKSPACE); | ||
| var actualThread2 = traceResourceClient.getTraceThread(thread2.id(), thread2.projectId(), API_KEY, | ||
| TEST_WORKSPACE); | ||
| var actualThread3 = traceResourceClient.getTraceThread(thread3.id(), thread3.projectId(), API_KEY, | ||
| TEST_WORKSPACE); |
There was a problem hiding this comment.
Same here, please search all threads in one call if possible.
| TraceAssertions.assertThreads(List.of(thread1.toBuilder().tags(tags).build()), List.of(actualThread1)); | ||
| TraceAssertions.assertThreads(List.of(thread2.toBuilder().tags(tags).build()), List.of(actualThread2)); | ||
| TraceAssertions.assertThreads(List.of(thread3.toBuilder().tags(tags).build()), List.of(actualThread3)); |
There was a problem hiding this comment.
Please use one assertion, but passing the list with all threads instead of one by one.
| @Test | ||
| void batchUpdateThreadsWithExistingTags() { |
There was a problem hiding this comment.
This should be a parameterised tests of the first one, basically:
- Initial value would be null or random.
- It's set to some other expected random value.
JetoPistola
assigned
JetoPistola,
YarivHashaiComet and
mansi0829 and unassigned
mansi0829,
JetoPistola and
YarivHashaiComet
6 participants
Details
Added the ability to tag multiple threads at once, similar to how tags can be applied to multiple traces. Users can now select several threads and add or update tags in a single action, improving workflow efficiency and consistency.
Change checklist
Issues
Testing
Documentation