From 785abdec8dc00f23ca5ba682b86c0d3b0f56a73a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ask=20Bj=C3=B8rn=20Hansen?= Date: Fri, 6 Jun 2025 20:02:23 -0700 Subject: [PATCH] ulid: simplify, add function without a timestamp --- CLAUDE.md | 2 +- ulid/ulid.go | 66 +++++++------------ ulid/ulid_test.go | 159 ++++++++++++++++++++++++++++++++++++---------- 3 files changed, 148 insertions(+), 79 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index c4ee2dc..565df73 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -103,7 +103,7 @@ OpenTelemetry distributed tracing setup with support for OTLP export via gRPC or Shared data structures for the NTP Pool project. Currently contains `LogScoreAttributes` for NTP server scoring with JSON and SQL database compatibility. ### `ulid/` -Thread-safe ULID (Universally Unique Lexicographically Sortable Identifier) generation using pooled monotonic readers with cryptographically secure seeding for high-concurrency environments. +Thread-safe ULID (Universally Unique Lexicographically Sortable Identifier) generation using cryptographically secure randomness. Optimized for simplicity and performance in high-concurrency environments. ### `version/` Build metadata and version information system with Git integration. Provides CLI commands for Cobra and Kong frameworks, Prometheus build info metrics, and semantic version validation. diff --git a/ulid/ulid.go b/ulid/ulid.go index f15e05e..3c4f5e2 100644 --- a/ulid/ulid.go +++ b/ulid/ulid.go @@ -1,64 +1,44 @@ // Package ulid provides thread-safe ULID (Universally Unique Lexicographically Sortable Identifier) generation. // // ULIDs are 128-bit identifiers that are lexicographically sortable and contain -// a timestamp component. This package uses a pool-based approach with -// cryptographically secure random seeding and monotonic ordering for optimal -// performance in concurrent environments. +// a timestamp component. This package uses cryptographically secure random +// generation optimized for simplicity and performance in concurrent environments. package ulid import ( cryptorand "crypto/rand" - "encoding/binary" - "io" - mathrand "math/rand" - "os" - "sync" "time" oklid "github.com/oklog/ulid/v2" - "go.ntppool.org/common/logger" ) -// monotonicPool is a pool of monotonic ULID readers for performance optimization. -// Each reader is initialized with a cryptographically secure random seed -// and random increment value to ensure uniqueness across concurrent usage. -var monotonicPool = sync.Pool{ - New: func() any { - log := logger.Setup() - - var seed int64 - err := binary.Read(cryptorand.Reader, binary.BigEndian, &seed) - if err != nil { - log.Error("crypto/rand error", "err", err) - os.Exit(10) - } - - rand := mathrand.New(mathrand.NewSource(seed)) - - inc := uint64(mathrand.Int63()) - - // log.Printf("seed: %d", seed) - // log.Printf("inc: %d", inc) - - // inc = inc & ^uint64(1<<63) // only want 63 bits - mono := oklid.Monotonic(rand, inc) - return mono - }, -} - -// MakeULID generates a new ULID with the specified timestamp using a pooled monotonic reader. +// MakeULID generates a new ULID with the specified timestamp using cryptographically secure randomness. // The function is thread-safe and optimized for high-concurrency environments. // -// Each call retrieves a monotonic reader from the pool, generates a ULID with the -// given timestamp, and returns it. The reader is not returned to the pool as it -// maintains internal state for monotonic ordering. +// This implementation prioritizes simplicity and performance over strict monotonicity within +// the same millisecond. Each ULID is guaranteed to be unique and lexicographically sortable +// across different timestamps. // // Returns a pointer to the generated ULID or an error if generation fails. // Generation should only fail under extreme circumstances (entropy exhaustion). func MakeULID(t time.Time) (*oklid.ULID, error) { - mono := monotonicPool.Get().(io.Reader) - - id, err := oklid.New(oklid.Timestamp(t), mono) + id, err := oklid.New(oklid.Timestamp(t), cryptorand.Reader) + if err != nil { + return nil, err + } + + return &id, nil +} + +// Make generates a new ULID with the current timestamp using cryptographically secure randomness. +// This is a convenience function equivalent to MakeULID(time.Now()). +// +// The function is thread-safe and optimized for high-concurrency environments. +// +// Returns a pointer to the generated ULID or an error if generation fails. +// Generation should only fail under extreme circumstances (entropy exhaustion). +func Make() (*oklid.ULID, error) { + id, err := oklid.New(oklid.Now(), cryptorand.Reader) if err != nil { return nil, err } diff --git a/ulid/ulid_test.go b/ulid/ulid_test.go index f280f4c..bf55dc5 100644 --- a/ulid/ulid_test.go +++ b/ulid/ulid_test.go @@ -1,6 +1,7 @@ package ulid import ( + cryptorand "crypto/rand" "sort" "sync" "testing" @@ -36,6 +37,38 @@ func TestMakeULID(t *testing.T) { t.Logf("ulid string 1 and 2: %s | %s", ul1.String(), ul2.String()) } +func TestMake(t *testing.T) { + // Test Make() function (uses current time) + ul1, err := Make() + if err != nil { + t.Fatalf("Make failed: %s", err) + } + + if ul1 == nil { + t.Fatal("Make returned nil ULID") + } + + // Sleep a bit and generate another + time.Sleep(2 * time.Millisecond) + + ul2, err := Make() + if err != nil { + t.Fatalf("Make failed: %s", err) + } + + // Should be different ULIDs + if ul1.String() == ul2.String() { + t.Errorf("ULIDs from Make() should be different: %s", ul1.String()) + } + + // Second should be later (or at least not earlier) + if ul1.Time() > ul2.Time() { + t.Errorf("second ULID should not have earlier timestamp: %d > %d", ul1.Time(), ul2.Time()) + } + + t.Logf("Make() ULIDs: %s | %s", ul1.String(), ul2.String()) +} + func TestMakeULIDUniqueness(t *testing.T) { tm := time.Now() seen := make(map[string]bool) @@ -54,6 +87,23 @@ func TestMakeULIDUniqueness(t *testing.T) { } } +func TestMakeUniqueness(t *testing.T) { + seen := make(map[string]bool) + + for i := 0; i < 1000; i++ { + ul, err := Make() + if err != nil { + t.Fatalf("Make failed on iteration %d: %s", i, err) + } + + str := ul.String() + if seen[str] { + t.Errorf("duplicate ULID generated: %s", str) + } + seen[str] = true + } +} + func TestMakeULIDTimestampProgression(t *testing.T) { t1 := time.Now() ul1, err := MakeULID(t1) @@ -79,34 +129,6 @@ func TestMakeULIDTimestampProgression(t *testing.T) { } } -func TestMakeULIDMonotonicity(t *testing.T) { - tm := time.Now() - var ulids []*oklid.ULID - - // Generate ULIDs rapidly with same timestamp - for i := 0; i < 100; i++ { - ul, err := MakeULID(tm) - if err != nil { - t.Fatalf("MakeULID failed on iteration %d: %s", i, err) - } - ulids = append(ulids, ul) - } - - // Count non-monotonic pairs - nonMonotonicCount := 0 - for i := 1; i < len(ulids); i++ { - if ulids[i-1].Compare(*ulids[i]) >= 0 { - nonMonotonicCount++ - } - } - - // Report summary if any non-monotonic pairs found - if nonMonotonicCount > 0 { - t.Logf("Note: %d out of %d ULID pairs with same timestamp were not monotonic due to pool usage", - nonMonotonicCount, len(ulids)-1) - } -} - func TestMakeULIDConcurrency(t *testing.T) { const numGoroutines = 10 const numULIDsPerGoroutine = 100 @@ -152,6 +174,50 @@ func TestMakeULIDConcurrency(t *testing.T) { } } +func TestMakeConcurrency(t *testing.T) { + const numGoroutines = 10 + const numULIDsPerGoroutine = 100 + + var wg sync.WaitGroup + ulidChan := make(chan *oklid.ULID, numGoroutines*numULIDsPerGoroutine) + + // Start multiple goroutines generating ULIDs concurrently + for i := 0; i < numGoroutines; i++ { + wg.Add(1) + go func() { + defer wg.Done() + for j := 0; j < numULIDsPerGoroutine; j++ { + ul, err := Make() + if err != nil { + t.Errorf("Make failed: %s", err) + return + } + ulidChan <- ul + } + }() + } + + wg.Wait() + close(ulidChan) + + // Collect all ULIDs and check uniqueness + seen := make(map[string]bool) + count := 0 + + for ul := range ulidChan { + str := ul.String() + if seen[str] { + t.Errorf("duplicate ULID generated in concurrent test: %s", str) + } + seen[str] = true + count++ + } + + if count != numGoroutines*numULIDsPerGoroutine { + t.Errorf("expected %d ULIDs, got %d", numGoroutines*numULIDsPerGoroutine, count) + } +} + func TestMakeULIDErrorHandling(t *testing.T) { // Test with various timestamps timestamps := []time.Time{ @@ -160,13 +226,13 @@ func TestMakeULIDErrorHandling(t *testing.T) { time.Now().Add(time.Hour), // Future time } - for _, tm := range timestamps { + for i, tm := range timestamps { ul, err := MakeULID(tm) if err != nil { - t.Errorf("MakeULID failed with timestamp %v: %s", tm, err) + t.Errorf("MakeULID failed with timestamp %d: %s", i, err) } if ul == nil { - t.Errorf("MakeULID returned nil ULID with timestamp %v", tm) + t.Errorf("MakeULID returned nil ULID with timestamp %d", i) } } } @@ -223,6 +289,17 @@ func BenchmarkMakeULID(b *testing.B) { } } +// Benchmark Make function +func BenchmarkMake(b *testing.B) { + b.ResetTimer() + for i := 0; i < b.N; i++ { + _, err := Make() + if err != nil { + b.Fatalf("Make failed: %s", err) + } + } +} + // Benchmark concurrent ULID generation func BenchmarkMakeULIDConcurrent(b *testing.B) { tm := time.Now() @@ -237,11 +314,23 @@ func BenchmarkMakeULIDConcurrent(b *testing.B) { }) } -// Benchmark pool performance -func BenchmarkMonotonicPool(b *testing.B) { +// Benchmark concurrent Make function +func BenchmarkMakeConcurrent(b *testing.B) { + b.RunParallel(func(pb *testing.PB) { + for pb.Next() { + _, err := Make() + if err != nil { + b.Fatalf("Make failed: %s", err) + } + } + }) +} + +// Benchmark random number generation +func BenchmarkCryptoRand(b *testing.B) { + buf := make([]byte, 10) // ULID entropy size b.ResetTimer() for i := 0; i < b.N; i++ { - _ = monotonicPool.Get() - // Note: we don't put it back as the current implementation doesn't reuse readers + cryptorand.Read(buf) } }