-
Notifications
You must be signed in to change notification settings - Fork 312
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
OPIK-904: Split find project endpoint into two endpoints find and stats #1176
OPIK-904: Split find project endpoint into two endpoints find and stats #1176
Conversation
a9805a0
to
563ff2d
Compare
@@ -188,7 +190,7 @@ public Response deleteById(@PathParam("id") UUID id) { | |||
@ApiResponse(responseCode = "400", description = "Bad Request", content = @Content(schema = @Schema(implementation = ErrorMessage.class))), | |||
@ApiResponse(responseCode = "404", description = "Not Found", content = @Content(schema = @Schema(implementation = ErrorMessage.class))) | |||
}) | |||
@JsonView({Project.View.Public.class}) | |||
@JsonView({Project.View.Detailed.class}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no test that verifies fields marked with Detailed
view, returned by this endpoint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test remains the same since this endpoint didn't change; only the. find was affected
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, old test also didn't check it. So we basically never verified that those values(now annotated with Detailed
) are returned properly by this endpoint. I suppose it should be added to corresponding test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, let me check it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
563ff2d
to
f21e92c
Compare
f21e92c
to
79756fe
Compare
@Builder(toBuilder = true) | ||
@JsonIgnoreProperties(ignoreUnknown = true) | ||
@JsonNaming(PropertyNamingStrategies.SnakeCaseStrategy.class) | ||
public record ProjectStatsSummary(List<ProjectStatsSummaryItem> content) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: this could be a lightweight Page object, with page
, size
on top of the current content
and without total
. But this works as FE is fine with it. We can always extend it in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, we can add in the next iteration
apps/opik-backend/src/main/java/com/comet/opik/domain/SpanDAO.java
Outdated
Show resolved
Hide resolved
3480e68
to
56099b2
Compare
Details
We are splitting this endpoint to optimize the critical path rendering
Also, two remaining orders by clause from the spans and traces queries were removed.
Issues
OPIK-904