[OPIK-2856] [BE] Implement UUIDv7 time-based filtering for traces #3921
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,71 @@ | ||
| package com.comet.opik.api; | ||
|
|
||
| import com.comet.opik.domain.IdGenerator; | ||
| import com.google.inject.Singleton; | ||
| import jakarta.inject.Inject; | ||
| import lombok.RequiredArgsConstructor; | ||
| import lombok.extern.slf4j.Slf4j; | ||
|
|
||
| import java.time.Instant; | ||
| import java.util.UUID; | ||
|
|
||
| /** | ||
| * Mapper for converting Instant time boundaries to UUIDv7 bounds for efficient BETWEEN queries. | ||
| * | ||
| * UUIDv7 encodes the timestamp in the first 48 bits, allowing for lexicographic sorting by time. | ||
| * This mapper creates UUID boundaries to ensure correct BETWEEN query semantics for time-based filtering. | ||
| * | ||
| * Implementation Note: | ||
| * We use IdGenerator.getTimeOrderedEpoch() which reliably creates UUIDs from timestamps. | ||
| * Per UUIDv7 RFC, the sub-millisecond 12 bits are optional and depend on implementation. | ||
| * Our implementation uses monotonic values with millisecond granularity, so using the start/end | ||
| * interval semantics with ±1ms ensures correct BETWEEN query results. | ||
| * This approach has been battle-tested for months without issues. | ||
| */ | ||
| @Singleton | ||
| @RequiredArgsConstructor(onConstructor_ = @Inject) | ||
| @Slf4j | ||
| public class InstantToUUIDMapper { | ||
|
|
||
| private final IdGenerator idGenerator; | ||
|
|
||
| /** | ||
| * Generates a UUIDv7 lower bound from a timestamp for BETWEEN queries. | ||
| * Creates the lexicographically smallest UUID with this timestamp. | ||
| * | ||
| * @param timestamp the instant in time | ||
| * @return the lower bound UUIDv7 (min UUID at this timestamp) | ||
| */ | ||
| public UUID toLowerBound(Instant timestamp) { | ||
|
Member
There was a problem hiding this comment. Minor: probably this whole class is no longer needed, as it adds verbosity with little benefit. You could move these to |
||
| if (timestamp == null) { | ||
| return null; | ||
| } | ||
|
|
||
| return idGenerator.getTimeOrderedEpoch(timestamp.toEpochMilli()); | ||
| } | ||
|
|
||
| /** | ||
| * Generates a UUIDv7 upper bound from a timestamp for BETWEEN queries. | ||
| * Creates the UUID at the next millisecond to ensure inclusive BETWEEN semantics. | ||
| * | ||
| * This ensures BETWEEN includes all UUIDs created within the end timestamp millisecond. | ||
| * For example, if querying traces between 10:00:00.000 and 10:00:01.000: | ||
| * - toLowerBound(10:00:00.000) gives the UUID at 10:00:00 | ||
| * - toUpperBound(10:00:01.000) gives the UUID at 10:00:02 (next millisecond) | ||
| * - BETWEEN x AND y includes all UUIDs from 10:00:00 up to but NOT including 10:00:02 | ||
| * - Which correctly includes all UUIDs from 10:00:00 through 10:00:01.999 | ||
| * | ||
| * @param timestamp the instant in time | ||
| * @return the upper bound UUIDv7 (UUID at timestamp+1ms) | ||
| */ | ||
| public UUID toUpperBound(Instant timestamp) { | ||
| if (timestamp == null) { | ||
| return null; | ||
| } | ||
|
|
||
| // Add 1ms to get the first UUID AFTER the end time | ||
| // BETWEEN will include all UUIDs from toLowerBound to (but not including) this value | ||
| long nextMillis = timestamp.toEpochMilli() + 1; | ||
| return idGenerator.getTimeOrderedEpoch(nextMillis); | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,66 @@ | ||
| package com.comet.opik.infrastructure.web; | ||
|
|
||
| import jakarta.ws.rs.BadRequestException; | ||
| import jakarta.ws.rs.ext.ParamConverter; | ||
| import jakarta.ws.rs.ext.ParamConverterProvider; | ||
| import jakarta.ws.rs.ext.Provider; | ||
| import lombok.extern.slf4j.Slf4j; | ||
| import org.apache.commons.lang3.StringUtils; | ||
|
|
||
| import java.lang.annotation.Annotation; | ||
| import java.lang.reflect.Type; | ||
| import java.time.Instant; | ||
| import java.time.format.DateTimeParseException; | ||
| import java.util.Objects; | ||
|
|
||
| /** | ||
| * JAX-RS ParamConverter for automatic Instant conversion from query parameters. | ||
| * Supports both ISO-8601 format (e.g., "2024-01-01T00:00:00Z") and milliseconds since epoch. | ||
| */ | ||
| @Provider | ||
| @Slf4j | ||
| public class InstantParamConverter implements ParamConverterProvider { | ||
|
|
||
| private static final InstantConverter INSTANCE = new InstantConverter(); | ||
|
|
||
| @Override | ||
| public <T> ParamConverter<T> getConverter(Class<T> rawType, Type genericType, Annotation[] annotations) { | ||
| if (rawType != Instant.class) { | ||
| return null; | ||
| } | ||
|
|
||
| return (ParamConverter<T>) INSTANCE; | ||
| } | ||
|
|
||
| private static class InstantConverter implements ParamConverter<Instant> { | ||
|
|
||
| @Override | ||
| public Instant fromString(String value) { | ||
| if (StringUtils.isEmpty(value)) { | ||
| return null; | ||
| } | ||
|
|
||
| try { | ||
| // Try parsing as ISO-8601 format first | ||
| return Instant.parse(value); | ||
| } catch (DateTimeParseException exception) { | ||
| log.debug("Failed to parse '{}' as ISO-8601, attempting to parse as milliseconds since epoch", value, | ||
| exception); | ||
| try { | ||
| // Fall back to parsing as milliseconds since epoch | ||
| long epochMillis = Long.parseLong(value); | ||
| return Instant.ofEpochMilli(epochMillis); | ||
| } catch (NumberFormatException numberFormatException) { | ||
| throw new BadRequestException( | ||
| "Invalid instant format: '%s'. Expected ISO-8601 (e.g., 2024-01-01T00:00:00Z) or milliseconds since epoch" | ||
| .formatted(value)); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public String toString(Instant value) { | ||
| return Objects.toString(value, null); | ||
| } | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
For this, I'd rather have a single source of truth in
IdGenerator. SeegetTimeOrderedEpochmethod which relies on our librarycom.fasterxml.uuid.java-uuid-generator, specifically on methodInitially, I'd rather rely on a reliable library implementation, than re-inventing the wheel ourselves. That way we don't need to maintain the implementation ourselves.
I propose the following:
IdGenerator.getTimeOrderedEpoch(based onTimeBasedEpochGenerator.construct.OpenTelemetryMapper.convertOtelIdToUUIDv7.Let me know how this sounds.
Uh oh!
There was an error while loading. Please reload this page.
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.
Hi @andrescrz,
Thanks for your comment. On why not use
getTimeOrderedEpoch:While IdGenerator.getTimeOrderedEpoch() also creates UUIDs from timestamps using the java-uuid-generator library, it uses random bits for the lower 80 bits of the UUID. For time-range queries, we need:
This ensures that BETWEEN queries correctly include all traces within the specified time range. IdGenerator's random bits would make the bounds non-deterministic, breaking BETWEEN semantics.
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.
I remember discussing this with @BorisTkachenko and I raised the same concerns, but we came to the conclusion that
getTimeOrderedEpochworks fine in practice. Some reasons:With all this in mind, I still recommend going with
getTimeOrderedEpoch. Some queries based ongetTimeOrderedEpochare already in place and working for months, without bug reports so far.But not concern if you prefer to move forward. This would be just a small piece of tech debt to centralise and remove duplications in the future (if we ever do 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.
Let me double check