[OPIK-1853] Workspace daily metrics endpoint #2505
Conversation
Backend Tests Results 175 files + 3 175 suites +3 18m 0s ⏱️ + 1m 33s Results for commit 0783275. ± Comparison against base commit 86e4ae3. This pull request removes 7 and adds 19 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
58dcd25 to
d8a3833
Compare
apps/opik-backend/src/main/java/com/comet/opik/api/metrics/WorkspaceMetricRequest.java
Show resolved
Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/api/metrics/WorkspaceMetricRequest.java
Outdated
Show resolved
Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/api/metrics/WorkspaceMetricRequest.java
Show resolved
Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/api/metrics/WorkspaceMetricResponse.java
Outdated
Show resolved
Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/api/metrics/WorkspaceMetricsSummaryResponse.java
Show resolved
Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/domain/WorkspaceMetricsDAO.java
Outdated
Show resolved
Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/domain/WorkspaceMetricsDAO.java
Outdated
Show resolved
Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/domain/WorkspaceMetricsDAO.java
Outdated
Show resolved
Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/domain/WorkspaceMetricsDAO.java
Outdated
Show resolved
Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/domain/WorkspaceMetricsDAO.java
Outdated
Show resolved
Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/domain/WorkspaceMetricsDAO.java
Show resolved
Hide resolved
...kend/src/test/java/com/comet/opik/api/resources/utils/resources/WorkspaceResourceClient.java
Show resolved
Hide resolved
There was a problem hiding this comment.
The approach generally LGTM. There are just some final details to polish, mostly related to the new library adoption, which is important to get right the first time.
Kudos for taking advantage of our UUID v7 for range filtering for the first time, as it's something that we should use heavily when needed.
apps/opik-backend/src/main/java/com/comet/opik/api/metrics/WorkspaceMetricRequest.java
Show resolved
Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/domain/WorkspaceMetricsDAO.java
Outdated
Show resolved
Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/domain/WorkspaceMetricsDAO.java
Outdated
Show resolved
Hide resolved
.../opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/WorkspacesResourceTest.java
Outdated
Show resolved
Hide resolved
|
|
||
| @Override | ||
| public UUID getTimeOrderedEpoch(long rawTimestamp) { | ||
| return generator.construct(rawTimestamp); |
There was a problem hiding this comment.
@BorisTkachenko have you tested this? I don't believe this is correct from the logical perspective:
A quick test of this:
public static void main(String[] args) {
var generator = Generators.timeBasedEpochGenerator();
var now = Instant.now();
for (int i = 0; i < 10; i++) {
var id = generator.construct(now.toEpochMilli());
System.out.println(id);
}
}
Results in:
01979bdc-da28-7d27-a410-e9945a5fa36f
01979bdc-da28-7d27-a410-e9945a5fa370
01979bdc-da28-7d27-a410-e9945a5fa371
01979bdc-da28-7d27-a410-e9945a5fa372
01979bdc-da28-7d27-a410-e9945a5fa373
01979bdc-da28-7d27-a410-e9945a5fa374
01979bdc-da28-7d27-a410-e9945a5fa375
01979bdc-da28-7d27-a410-e9945a5fa376
01979bdc-da28-7d27-a410-e9945a5fa377
01979bdc-da28-7d27-a410-e9945a5fa378
So basically you generate an UUID for the given timestamp for the 1st 48 bits (correct), but the the other 74 are random (incorrect):
What you actually want is the first (or last) UUID for a given timestamp, so you can search an ID in a range for a given time period.
With this implementation, you basically depend on luck/timing to get correct results.
There was a problem hiding this comment.
We discussed that the first 32 bits give second precision, plus the next 12 provide millis precision. Then, depending on the random generator for the final 74, there can be even higher precision (micro or nano).
Given that this is uses lexicographically sorting combined with the BETWEEN clause for in the ClickHouse WHERE SQL statement, and given that aggregations are daily, in the case of any IDs not found, they would be completely neglectable.
They would be probably even neglectable with smaller aggregations, as millis or even less precision is very small.
So this is good to go as it is.
Details
Workspace daily metrics endpoint
Testing
Integration tests
Documentation
https://www.notion.so/cometml/Home-Page-Tech-HLD-2097124010a380d098cfc0b378819713#20d7124010a380918a83ddb12a2494ad