[OPIK-2497] Add bulk tag operations for traces, spans and threads #3920
Conversation
YarivHashaiComet
force-pushed
the
yariv-h/OPIK-2497-bulk-tag-operations-traces-spans-threads
branch
from
83f159f to
e59dbc9
Compare
|
🌿 Preview your docs: https://opik-preview-16b57c7e-3e71-4ffa-94fd-9b35c46744b2.docs.buildwithfern.com/docs/opik The following broken links where found: Page: Page: Page: Page: Page: https://opik-preview-16b57c7e-3e71-4ffa-94fd-9b35c46744b2.docs.buildwithfern.com/docs/opik/integrations/gretel Page: https://opik-preview-16b57c7e-3e71-4ffa-94fd-9b35c46744b2.docs.buildwithfern.com/docs/opik/integrations/gretel/ |
|
🌿 Preview your docs: https://opik-preview-e0aa25e5-f681-4d62-b690-25f6aa194226.docs.buildwithfern.com/docs/opik The following broken links where found: Page: https://opik-preview-e0aa25e5-f681-4d62-b690-25f6aa194226.docs.buildwithfern.com/docs/opik/integrations/gretel Page: https://opik-preview-e0aa25e5-f681-4d62-b690-25f6aa194226.docs.buildwithfern.com/docs/opik/integrations/gretel/ |
- Changed paths from /batch to /batch/tags for traces and spans - Changed path from /threads/batch to /threads/batch/tags - Renamed methods from batchUpdate to batchUpdateTags - Updated operationIds to batchUpdateTracesTags, batchUpdateSpansTags, batchUpdateThreadsTags - Updated frontend API calls to use new paths - Updated test helpers to use new paths - Updated log messages to be more specific about tag updates
|
🌿 Preview your docs: https://opik-preview-9b77cd39-88fa-46c6-99e4-cc329e42e5c0.docs.buildwithfern.com/docs/opik The following broken links where found: Page: https://opik-preview-9b77cd39-88fa-46c6-99e4-cc329e42e5c0.docs.buildwithfern.com/docs/opik/integrations/gretel Page: https://opik-preview-9b77cd39-88fa-46c6-99e4-cc329e42e5c0.docs.buildwithfern.com/docs/opik/integrations/gretel/ Page: https://opik-preview-9b77cd39-88fa-46c6-99e4-cc329e42e5c0.docs.buildwithfern.com/docs/opik/integrations/gretel Page: https://opik-preview-9b77cd39-88fa-46c6-99e4-cc329e42e5c0.docs.buildwithfern.com/docs/opik/integrations/gretel/ Page: Page: Page: Page: |
Backend Tests Results 267 files 267 suites 46m 11s ⏱️ For more details on these failures, see this check. Results for commit 1cac7b5. ♻️ This comment has been updated with latest results. |
Reduced duplication by combining merge/replace tests into single parameterized test
|
Let's check the overlap with this other PR: There was a PR review there and feedback comments, and also a decent implementation that you can use as a base. |
andrescrz
left a comment
andrescrz
left a comment
There was a problem hiding this comment.
Left a preliminary review on this draft, with the stuff to be addressed.
| @JsonIgnoreProperties(ignoreUnknown = true) | ||
| @JsonNaming(PropertyNamingStrategies.SnakeCaseStrategy.class) | ||
| @Schema(description = "Request to batch update tags for multiple spans") | ||
| public record SpanBatchTagUpdate( |
There was a problem hiding this comment.
Generally, these new endpoints shouldn't be tied to just tags. We might use them to update other fields in batches for spans, traces and threads, not just for tags.
We can't have an endpoint per field, so make them neutral. We will reuse them.
Review this across this whole PR: request and other objects, resource and paths, service, DAO, tests etc.
| } | ||
|
|
||
| @WithSpan | ||
| public Flux<Span> getByIds(@NonNull Set<UUID> ids) { |
There was a problem hiding this comment.
The general convention for our patching endpoints is to query at the DAO level, not at the service level. That includes batch endpoints. Please see other endpoints as an example.
| if (!mergeTags || batchUpdate.update().tags() == null) { | ||
| // Simple case: apply the same update to all spans | ||
| return Flux.fromIterable(batchUpdate.ids()) | ||
| .flatMap(id -> update(id, batchUpdate.update())) |
There was a problem hiding this comment.
We can't do individual update up to the batch limit, due to performance and scalability reasons.
In addition, it defeats the purpose of implementing batch endpoints.
Let's discuss if we really need the merge functionality for tags as in the past we discussed overwriting them. If so, we could delegate to the DAO.
There was a problem hiding this comment.
@andrescrz - just doubled checked it with product - we want to append
There was a problem hiding this comment.
will fix the batch opperation and will move it to the dao
There was a problem hiding this comment.
This is an auto-generated file. Please do not edit manually.
…or bulk update chunk size and stream buffer size. Update DAO implementations to utilize these new configuration parameters for improved performance in bulk operations.
Details
This PR adds bulk tag operations for traces, spans, and threads, removing the previous 10-item limitation and enabling users to tag up to 1000 items at once.
Backend Changes
PATCH /v1/private/traces/batch- Batch update tags for tracesPATCH /v1/private/spans/batch- Batch update tags for spansPATCH /v1/private/traces/threads/batch- Batch update tags for threadsTraceBatchTagUpdate,SpanBatchTagUpdate, andTraceThreadBatchTagUpdateDTOsmergeTagsparameter allows merging with existing tags or replacing themFrontend Changes
useTraceBatchUpdateMutation,useSpanBatchUpdateMutation,useThreadBatchUpdateMutationAddTagDialogto use batch operations and removed the 10-item limitThreadsActionsPanelandThreadsTabChange checklist
Issues
Testing
Documentation
merge_tagsparameter for flexible tag management