OPIK-1764: Add endpoint to Open/Close threads manually #2501
Conversation
…hiagohora/OPIK-1765_make_threads_first_class_citizens
… into thiagohora/OPIK-1765_implement_closing_thread_mechanism
… into thiagohora/OPIK-1765_implement_closing_thread_mechanism
Backend Tests Results 175 files + 1 175 suites +1 18m 8s ⏱️ + 3m 17s Results for commit 5eaf057. ± Comparison against base commit 1f53469. This pull request removes 1 and adds 1 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
SDK E2E Tests Results0 tests 0 ✅ 0s ⏱️ Results for commit 10c6f20. |
SDK E2E Tests Results0 tests 0 ✅ 0s ⏱️ Results for commit 10c6f20. |
SDK E2E Tests Results0 tests 0 ✅ 0s ⏱️ Results for commit 10c6f20. ♻️ This comment has been updated with latest results. |
10c6f20 to
77aaa6d
Compare
| .sortingFields(List.of()) | ||
| .build(); | ||
|
|
||
| Flux<Trace> items = service.search(request.limit(), searchCriteria) |
There was a problem hiding this comment.
service -> can be renamed to traceService?
There was a problem hiding this comment.
Here, we use context awareness; the controller is scoped to cover trace domain-related endpoints. Having this in mind, the service is assumed to be related to traces, so when it's not, a more descriptive name is used. Rename it would not add much context
There was a problem hiding this comment.
i see that you mean, but try to think about a junior developer - would it be clear to him? :) along with that this resource have multiple services so just not to create a confusion - it will be more readable and less error prompt
|
|
||
| log.info("Streaming traces search results by '{}', workspaceId '{}'", request, workspaceId); | ||
|
|
||
| var searchCriteria = TraceSearchCriteria.builder() |
There was a problem hiding this comment.
can this been pushed into the service layer?
There was a problem hiding this comment.
Same, this is not business logic, it's a mapper from request DTO to criteria object, which belongs to the domain
There was a problem hiding this comment.
the resources should not be aware to anything related to the searchCriteria or any other logic beside passing it to the service / facade layer
There was a problem hiding this comment.
Not exactly, the responsibility of api/controller layer is mapping objects from domain to api and vice versa and validate it. you can see all endpoint follow this pattern
| @QueryParam("filters") String filters) { | ||
|
|
||
| validateProjectNameAndProjectId(projectName, projectId); | ||
| var traceFilters = filtersFactory.newFilters(filters, TraceFilter.LIST_TYPE_REFERENCE); |
There was a problem hiding this comment.
same, logic should be part of the serivce layer to keep the resoueces clean as possible without logic
There was a problem hiding this comment.
This is related to validation of the param receiced from the request, so they don't classify as business logic
|
|
||
| validateProjectNameAndProjectId(projectName, projectId); | ||
| var traceFilters = filtersFactory.newFilters(filters, TraceFilter.LIST_TYPE_REFERENCE); | ||
| var searchCriteria = TraceSearchCriteria.builder() |
apps/opik-backend/src/main/java/com/comet/opik/api/resources/v1/priv/TracesResource.java
Outdated
Show resolved
Hide resolved
|
|
||
| log.info("Find feedback score names by project_id '{}', on workspaceId '{}'", | ||
| projectId, workspaceId); | ||
| FeedbackScoreNames feedbackScoreNames = feedbackScoreService |
There was a problem hiding this comment.
do we want to return an object that contains the names + ids of the feedback score?
There was a problem hiding this comment.
This was just moved, it's out of the scope of the PR
| public boolean isValid(TraceThreadIdentifier traceThreadIdentifier, | ||
| ConstraintValidatorContext constraintValidatorContext) { | ||
| return !(traceThreadIdentifier.projectName() == null && traceThreadIdentifier.projectId() == null); | ||
| } |
There was a problem hiding this comment.
public boolean isValid(TraceThreadIdentifier traceThreadIdentifier,
ConstraintValidatorContext constraintValidatorContext) {
return traceThreadIdentifier != null &&
(traceThreadIdentifier.projectName() != null || traceThreadIdentifier.projectId() != null);
}
do we want to simplify it maybe to be more direct?
There was a problem hiding this comment.
I see it as more verbose, but we can extract the condition to a variable to improve readability
| import lombok.Builder; | ||
|
|
||
| import java.util.UUID; | ||
|
|
||
| @Builder(toBuilder = true) | ||
| @JsonIgnoreProperties(ignoreUnknown = true) | ||
| @JsonNaming(PropertyNamingStrategies.SnakeCaseStrategy.class) | ||
| public record TraceThreadIdentifier(@NotNull UUID projectId, @NotBlank String threadId) { | ||
| @TraceThreadIdentifierValidation | ||
| public record TraceThreadIdentifier(String projectName, UUID projectId, @NotBlank String threadId) { |
There was a problem hiding this comment.
i saw we removed the @NotNull - by defenition or mistake? as well dose the projectName can be blank?
There was a problem hiding this comment.
Intended since now we can use either projectName or projectId
| @@ -38,7 +38,11 @@ Flux<ProjectWithPendingClosureTraceThreads> getProjectsWithPendingClosureThreads | |||
|
|
|||
| Mono<Void> processProjectWithTraceThreadsPendingClosure(UUID projectId, Instant lastUpdatedUntil); | |||
|
|
|||
| Mono<Boolean> addToPendingQueue(@NonNull UUID projectId); | |||
| Mono<Boolean> addToPendingQueue(UUID projectId); | |||
There was a problem hiding this comment.
can this be null? (saw we remove the restriction)
There was a problem hiding this comment.
No, but adding the Lombok annotation on interfaces has no effect. It has to be added to the implementation class
apps/opik-backend/src/main/java/com/comet/opik/api/resources/v1/priv/TracesResource.java
Outdated
Show resolved
Hide resolved
77aaa6d to
803ac15
Compare
…ose_threads_manually
803ac15 to
5eaf057
Compare
Details