[OPIK-2856] [BE] Refactor test code to use TraceResourceClient instead of direct URL_TEMPLATE calls by thiagohora · Pull Request #3947 · comet-ml/opik

thiagohora · 2025-11-05T13:20:29Z

Details

This PR refactors test code to use TraceResourceClient instead of direct HTTP client calls with URL_TEMPLATE, improving code maintainability and reducing duplication. Also, move the get trace threads tests to FindTraceThreadsResourceTest.

Changes Made

TracesResourceTest.java - Removed URL_TEMPLATE usage
- Replaced all direct client.target(URL_TEMPLATE) calls with traceResourceClient methods
- Created new methods in TraceResourceClient to encapsulate HTTP operations
- Added support for both API key and session token cookie authentication
- Fixed cookie-based authentication in session token tests
GetTracesByProjectResourceTest.java - Refactored endpoint calls
- Refactored 8 instances of direct client.target(URL_TEMPLATE) calls
- Simplified getAndAssertPage() method by using callGetTracesWithQueryParams()
- Changed from building WebTarget conditionally to building a Map of query parameters
- Replaced search endpoint calls with callSearchTracesStream()
- Fixed time filtering test to use mutable HashMap instead of immutable Map.of()
FindTraceThreadsResourceTest.java - New test file with TraceResourceClient usage
- Added as part of the refactoring to test trace threads retrieval
- Uses TraceResourceClient methods for all HTTP operations
- Tests both API key and session token cookie authentication
TraceResourceClient.java - Extended with new methods
- Added 18+ new methods for encapsulating HTTP operations
- Methods include: callGetTracesWithQueryParams(), callSearchTracesStream(), callGetWithPath(), callPutToPath(), callPostToPath(), callDeleteToPath(), callGetTraceThreadsWithSorting()
- Cookie-based variants for session token authentication (_WithCookie methods)
- Removed duplicate methods (callGetTraces, callSearchTraces)
- Made getWebTarget() private to prevent direct access
- Extracted addPathSegments() and addQueryParameters() helper methods to BaseCommentResourceClient
SpanResourceClient.java - Added cookie authentication support
- Added callGetSpansWithQueryParamsAndCookie() and callFeedbackScoresWithContainerAndCookie()
- Extracted addQueryParameters() helper method to BaseCommentResourceClient
BaseCommentResourceClient.java - New shared helper methods
- Added addPathSegments() protected method for building path segments across multiple path parts
- Added addQueryParameters() protected method for adding query parameters to WebTarget
- Reduces code duplication between TraceResourceClient and SpanResourceClient
Code Quality Improvements
- Added missing HashMap import
- Removed unused imports (Entity, WebTarget, HttpHeaders, WORKSPACE_HEADER)
- All compilation errors resolved
- Eliminated code duplication through helper methods

Change checklist

User facing
Documentation update

Issues

Resolves OPIK-2856

Testing

✅ Backend compiles successfully (mvn clean compile)
✅ All 83 tests in GetTracesByProjectResourceTest$FindTraces pass
✅ Time filtering test whenTimeParametersInISO8601Format_thenReturnFilteredTraces passes
✅ All TracesResourceTest tests pass with refactored resource client usage
✅ No new regressions introduced

Documentation

No documentation updates needed for this refactoring. This is an internal test code improvement.

- Add InstantToUUIDMapper to convert Instant timestamps to UUIDv7 bounds - Add InstantParamConverter to parse ISO-8601 and epoch millisecond time parameters - Update TracesResource to accept from_time and to_time parameters on /traces and /traces/stats endpoints - Update TraceDAO to apply UUID-based time filtering using BETWEEN clause on id column - Update TraceSearchCriteria to include uuidFromTime and uuidToTime fields - Add comprehensive integration tests for time filtering with boundary conditions - All tests passing: 10/10 time filtering tests + validation tests

@SuppressWarnings

…d type safety - Fix InstantParamConverter to catch specific DateTimeParseException instead of generic Exception - Add debug logging when falling back to epoch milliseconds parsing - Refactor anonymous ParamConverter class to named InstantConverter inner class for clarity - Suppress unchecked cast with @SuppressWarnings annotation - Fix MySQLContainerUtils return type to use MySQLContainer<?> for type safety

- Update tests to reflect that toUpperBound uses next millisecond (+1ms) for inclusive BETWEEN queries - Remove outdated assertions expecting same timestamp in both bounds - Verify upper bound is lexicographically greater than lower bound - All 13 tests now passing

- #7: Add INSTANCE singleton pattern for InstantConverter - #8: Use StringUtils.isEmpty() for null-safe empty check - Note: #2 and #3 already addressed in previous commit

@singleton

- Simplified InstantToUUIDMapper to use IdGenerator.getTimeOrderedEpoch() instead of convertOtelIdToUUIDv7 - Per UUIDv7 RFC, sub-millisecond 12 bits are optional with millisecond granularity - Start/end interval semantics with ±1ms ensures correct BETWEEN query results - This approach has been battle-tested for months without issues per reviewer recommendation - Converted InstantToUUIDMapper to @singleton service for proper DI integration - Updated TracesResource to inject InstantToUUIDMapper dependency - Updated tests to properly mock IdGenerator dependency

…hTimestamp helper method

…sed-filter-for-spans

…ce helper method and fix transformTestParams call

…stead of direct URL_TEMPLATE calls - Replace all direct client.target(URL_TEMPLATE) calls with TraceResourceClient methods - Add callFeedbackScoresWithCookie method to TraceResourceClient for session token authentication - Add callRetrieveThreadResponseWithCookie method to TraceResourceClient for session token authentication - Fix feedback batch endpoint by using callFeedbackScores and callFeedbackScoresWithCookie - Add null checks for query parameters to prevent NPE errors - Fix API key vs session token usage in authentication tests - Rename get__whenApiKeyIsPresent__thenReturnTraceThread to get__whenSessionTokenIsPresent__thenReturnTraceThread in SessionTokenCookie class - Add mockGetWorkspaceIdByName() calls for proper workspace mocking - Preserve original test assertions and behavior - All tests properly refactored to use resource client methods instead of direct HTTP calls

- Remove callGetTraces() - duplicate of callGetTracesWithQueryParams() - Remove callSearchTraces() - duplicate of callSearchTracesStream() - Reduced code duplication and maintenance burden

Copilot

Pull Request Overview

This PR refactors test code to use TraceResourceClient and SpanResourceClient instead of direct HTTP client calls, improving maintainability and reducing code duplication. The main changes include extracting HTTP operations into reusable client methods, adding support for cookie-based authentication, and ensuring consistent API patterns across all test methods.

Key changes:

Centralized HTTP client operations in TraceResourceClient and SpanResourceClient with 18+ new helper methods
Replaced 8+ instances of direct client.target(URL_TEMPLATE) calls with client method invocations
Added cookie-based authentication support for session token testing

Reviewed Changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
TracesResourceTest.java	Replaced direct HTTP calls with `traceResourceClient` methods; removed unused imports and fields
GetTracesByProjectResourceTest.java	Refactored endpoint calls to use `callGetTracesWithQueryParams()` and `callSearchTracesStream()`; changed to HashMap for query params
FindTraceThreadsResourceTest.java	New file extracting trace thread tests from TracesResourceTest.java
TraceResourceClient.java	Added 18+ new methods for encapsulating HTTP operations with both API key and cookie authentication
SpanResourceClient.java	Added cookie authentication support methods; added `callGetSpansWithQueryParams()`

…r method and add clarifying comment

github-actions · 2025-11-05T13:28:09Z

Backend Tests Results

281 files 281 suites 53m 20s ⏱️
5 397 tests 5 389 ✅ 8 💤 0 ❌
5 352 runs 5 344 ✅ 8 💤 0 ❌

Results for commit 5be3b4a.

♻️ This comment has been updated with latest results.

github-actions · 2025-11-05T13:32:34Z

SDK E2E Tests Results

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

Results for commit 42a2556.

♻️ This comment has been updated with latest results.

…-test-url-template

…d addPathSegments()

…thSorting() public method

…thods to BaseCommentResourceClient

@SuppressWarnings

…d of direct URL_TEMPLATE calls (#3947) * [NA] [BE] Upgrade MySQL container from Testcontainers * Fix imports order * [OPIK-2856] [BE] Implement UUIDv7 time-based filtering for traces - Add InstantToUUIDMapper to convert Instant timestamps to UUIDv7 bounds - Add InstantParamConverter to parse ISO-8601 and epoch millisecond time parameters - Update TracesResource to accept from_time and to_time parameters on /traces and /traces/stats endpoints - Update TraceDAO to apply UUID-based time filtering using BETWEEN clause on id column - Update TraceSearchCriteria to include uuidFromTime and uuidToTime fields - Add comprehensive integration tests for time filtering with boundary conditions - All tests passing: 10/10 time filtering tests + validation tests * [OPIK-2856] Address PR review comments: improve exception handling and type safety - Fix InstantParamConverter to catch specific DateTimeParseException instead of generic Exception - Add debug logging when falling back to epoch milliseconds parsing - Refactor anonymous ParamConverter class to named InstantConverter inner class for clarity - Suppress unchecked cast with @SuppressWarnings annotation - Fix MySQLContainerUtils return type to use MySQLContainer<?> for type safety * [OPIK-2856] Fix InstantToUUIDMapperTest to match implementation - Update tests to reflect that toUpperBound uses next millisecond (+1ms) for inclusive BETWEEN queries - Remove outdated assertions expecting same timestamp in both bounds - Verify upper bound is lexicographically greater than lower bound - All 13 tests now passing * Remove setup duplicated code * Revision 2: Address PR review comments - LOW priority fixes - #7: Add INSTANCE singleton pattern for InstantConverter - #8: Use StringUtils.isEmpty() for null-safe empty check - Note: #2 and #3 already addressed in previous commit * Revision 3: Use IdGenerator.getTimeOrderedEpoch() for UUID bounds - Simplified InstantToUUIDMapper to use IdGenerator.getTimeOrderedEpoch() instead of convertOtelIdToUUIDv7 - Per UUIDv7 RFC, sub-millisecond 12 bits are optional with millisecond granularity - Start/end interval semantics with ±1ms ensures correct BETWEEN query results - This approach has been battle-tested for months without issues per reviewer recommendation - Converted InstantToUUIDMapper to @singleton service for proper DI integration - Updated TracesResource to inject InstantToUUIDMapper dependency - Updated tests to properly mock IdGenerator dependency * Fix tests * [OPIK-2856] [BE] Split Get spans Tests * Fix format * Revision 2: Extract duplicated span creation logic into createSpanWithTimestamp helper method * [OPIK-2856] [BE] Implement UUIDv7 time-based filtering for spans * Revision 3: Extract workspace setup duplication into setupTestWorkspace helper method and fix transformTestParams call * [OPIK-2856] Refactor TracesResourceTest to use TraceResourceClient instead of direct URL_TEMPLATE calls - Replace all direct client.target(URL_TEMPLATE) calls with TraceResourceClient methods - Add callFeedbackScoresWithCookie method to TraceResourceClient for session token authentication - Add callRetrieveThreadResponseWithCookie method to TraceResourceClient for session token authentication - Fix feedback batch endpoint by using callFeedbackScores and callFeedbackScoresWithCookie - Add null checks for query parameters to prevent NPE errors - Fix API key vs session token usage in authentication tests - Rename get__whenApiKeyIsPresent__thenReturnTraceThread to get__whenSessionTokenIsPresent__thenReturnTraceThread in SessionTokenCookie class - Add mockGetWorkspaceIdByName() calls for proper workspace mocking - Preserve original test assertions and behavior - All tests properly refactored to use resource client methods instead of direct HTTP calls * [OPIK-2856] Remove duplicate methods from TraceResourceClient - Remove callGetTraces() - duplicate of callGetTracesWithQueryParams() - Remove callSearchTraces() - duplicate of callSearchTracesStream() - Reduced code duplication and maintenance burden * Fix tests * Revision 2: Address Copilot review comments - remove redundant wrapper method and add clarifying comment * Revision 3: Extract duplicated path splitting logic into helper method addPathSegments() * Revision 4: Make getWebTarget() private and add callGetTraceThreadsWithSorting() public method * Revision 7: Move addPathSegments() and addQueryParameters() helper methods to BaseCommentResourceClient

thiagohora added 20 commits November 3, 2025 10:08

[NA] [BE] Upgrade MySQL container from Testcontainers

cf912c6

Fix imports order

3d9126a

Merge branch 'main' into thiaghora/OPIK-2856-uuidv7-time-based-filtering

037c880

Merge branch 'main' into thiaghora/OPIK-2856-uuidv7-time-based-filtering

551e9b8

Remove setup duplicated code

fc34b96

Revision 2: Address PR review comments - LOW priority fixes

4a0842a

- #7: Add INSTANCE singleton pattern for InstantConverter - #8: Use StringUtils.isEmpty() for null-safe empty check - Note: #2 and #3 already addressed in previous commit

Fix tests

bdd2249

[OPIK-2856] [BE] Split Get spans Tests

bdec560

Fix format

43d7eaf

Revision 2: Extract duplicated span creation logic into createSpanWit…

253e0cb

…hTimestamp helper method

[OPIK-2856] [BE] Implement UUIDv7 time-based filtering for spans

4002a19

Merge branch 'main' into thiaghora/OPIK-2856-implement-uuidv7-time-ba…

77014b0

…sed-filter-for-spans

Revision 3: Extract workspace setup duplication into setupTestWorkspa…

9957c2a

…ce helper method and fix transformTestParams call

[OPIK-2856] Remove duplicate methods from TraceResourceClient

952486f

- Remove callGetTraces() - duplicate of callGetTracesWithQueryParams() - Remove callSearchTraces() - duplicate of callSearchTracesStream() - Reduced code duplication and maintenance burden

Fix tests

9a048f9

Copilot AI review requested due to automatic review settings November 5, 2025 13:20

thiagohora requested a review from a team as a code owner November 5, 2025 13:20

github-actions bot assigned thiagohora Nov 5, 2025

Copilot AI reviewed Nov 5, 2025

View reviewed changes

Comment thread ...ckend/src/test/java/com/comet/opik/api/resources/v1/priv/GetTracesByProjectResourceTest.java

Comment thread ...-backend/src/test/java/com/comet/opik/api/resources/utils/resources/TraceResourceClient.java Outdated

Revision 2: Address Copilot review comments - remove redundant wrappe…

e72697a

…r method and add clarifying comment

comet-ml deleted a comment from github-actions bot Nov 5, 2025

Merge branch 'main' into thiaghora/OPIK-2856-refactor-traces-resource…

9c84962

…-test-url-template

thiagohora force-pushed the thiaghora/OPIK-2856-refactor-traces-resource-test-url-template branch from 42a2556 to 9c84962 Compare November 5, 2025 13:33

thiagohora added 3 commits November 5, 2025 14:47

Revision 3: Extract duplicated path splitting logic into helper metho…

c5b353e

…d addPathSegments()

Revision 4: Make getWebTarget() private and add callGetTraceThreadsWi…

465c0fe

…thSorting() public method

Revision 7: Move addPathSegments() and addQueryParameters() helper me…

5be3b4a

…thods to BaseCommentResourceClient

andrescrz approved these changes Nov 5, 2025

View reviewed changes

thiagohora merged commit 99aa306 into main Nov 5, 2025
16 checks passed

thiagohora deleted the thiaghora/OPIK-2856-refactor-traces-resource-test-url-template branch November 5, 2025 16:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[OPIK-2856] [BE] Refactor test code to use TraceResourceClient instead of direct URL_TEMPLATE calls#3947

[OPIK-2856] [BE] Refactor test code to use TraceResourceClient instead of direct URL_TEMPLATE calls#3947
thiagohora merged 25 commits intomainfrom
thiaghora/OPIK-2856-refactor-traces-resource-test-url-template

thiagohora commented Nov 5, 2025 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Nov 5, 2025 •

edited

Loading

Uh oh!

github-actions bot commented Nov 5, 2025 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

thiagohora commented Nov 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Changes Made

Change checklist

Issues

Testing

Documentation

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Nov 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Backend Tests Results

Uh oh!

github-actions bot commented Nov 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

SDK E2E Tests Results

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

thiagohora commented Nov 5, 2025 •

edited

Loading

github-actions bot commented Nov 5, 2025 •

edited

Loading

github-actions bot commented Nov 5, 2025 •

edited

Loading