[OPIK-568] Updated uuidv4_to_uuidv7 function to handle timestamps #1204
Conversation
jverre
changed the title
There was a problem hiding this comment.
LGTM, I also ran a couple of manual tests, seems to be working just fine :)
from opik import id_helpers, datetime_helpers
import uuid
import uuid6
import opik
client = opik.Opik()
id4 = str(uuid.uuid4())
id7_converted = id_helpers.uuid4_to_uuid7(datetime_helpers.local_timestamp(), id4)
id7_new = uuid6.uuid7()
print(id4)
print(id7_converted)
print(id7_new)
client.trace(name="test-trace-1", id=id7_converted)
client.trace(name="test-trace-2", id=id7_new)(here are the results from multiple runs)
@Lothiraldan is there anything you think might be worth checking regarding the existing dify integration?
@andrescrz please take a look at the PR as well, you wanted to be on top of that.
I'll review it soon. Thanks. |
|
|
||
| for i in range(NB_ID): | ||
| test_uuids.append((datetime.now(), str(uuid.uuid4()))) | ||
| time.sleep(0.5) |
There was a problem hiding this comment.
I'd personally narrow down this to 1 milli resolution (ok with some gap to prevent flakiness) in order to check how good is the timestamp resolution in Python, as it affects our UUIDs.
The main reason is the multiplication to convert secs to millis. We need to test the precision of that.
No need for a test on sorting on the rand part, as optional in the RFC and because our particular implementation doesn't support as it is based on the inputted UUID v4.
| import time | ||
|
|
||
|
|
||
| def test_uuid4_to_uuid7__generates_valid_uuidv7(): |
There was a problem hiding this comment.
Personally, I miss converting this test in a happy path test that not only asserts the version, but each of the components on the payload:
- unix_ts_ms
- ver
- rand_a
- var
- rand_b
You'd need to check that they contain exactly what is expected. Maybe it's ok to just assert the actual UUID against the expected one, to make the test more readable and maintainable.
|
|
||
| for i in range(NB_ID): | ||
| test_uuids.append((timestamp, str(uuid.uuid4()))) | ||
| time.sleep(0.5) |
There was a problem hiding this comment.
Minor: no need to sleep in this case.
| if uuidv4.version != 4: | ||
| raise ValueError("Input UUID must be version 4") |
There was a problem hiding this comment.
Minor: this should probably be the first thing to do in this method.
Details
Updated the UUIDv4 to UUIDv7 function to handle timestamps correctly, also added unit tests for it