perf: remove cached clock by Flo4604 · Pull Request #4342 · unkeyed/unkey

Flo4604 · 2025-11-14T13:22:56Z

What does this PR do?

Our recent CPU profiles after our deployment yday shows an increased cpu usage to before.
This is mostly due to the 2 timers that run each millisecond to update the cached clock.

So using it might be faster at somepoint but not today.

Type of change

Bug fix (non-breaking change which fixes an issue)
Chore (refactoring code, technical debt, workflow improvements)
Enhancement (small improvements)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How should this be tested?

Tests should still work

Checklist

Required

Appreciated

If a UI change was made: Added a screen recording or screenshots to this PR
Updated the Unkey Docs if changes were necessary

changeset-bot · 2025-11-14T13:23:01Z

⚠️ No Changeset found

Latest commit: db0e24b

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

vercel · 2025-11-14T13:23:03Z

The latest updates on your projects. Learn more about Vercel for GitHub.

2 Skipped Deployments

Project	Deployment	Preview	Comments	Updated (UTC)
dashboard	Ignored	Preview		Nov 14, 2025 2:25pm
engineering	Ignored	Preview		Nov 14, 2025 2:25pm

coderabbitai · 2025-11-14T13:23:24Z

Caution

Review failed

The pull request is closed.

📝 Walkthrough

Walkthrough

Removed the clock abstraction: deleted Handler.Clock and Services.CachedClock, removed clock imports, replaced h.Clock.Now()/cached clock uses with time.Now(), and updated tests and test utilities to stop injecting CachedClock.

Changes

Cohort / File(s)	Summary
Services & Run `go/apps/api/routes/services.go`, `go/apps/api/routes/register.go`, `go/apps/api/run.go`	Removed `CachedClock` field and related imports; stopped wiring/passing cached clock into route registration and services.
Handler Implementation `go/apps/api/routes/v2_ratelimit_multi_limit/handler.go`	Removed `Clock` field from `Handler`; replaced `h.Clock.Now()` with `time.Now()` and removed clock import.
Rate-limit Tests `go/apps/api/routes/v2_ratelimit_multi_limit/{200,400,401,403,410}_test.go`	Removed `Clock:`/`Clock: h.CachedClock` initializations from handler literals in tests.
Test Utilities `go/pkg/testutil/http.go`	Removed `CachedClock` field from `Harness` and eliminated its initialization in `NewHarness`.
UID Generation `go/pkg/uid/uid.go`	Removed clock import and global cached clock; replaced `clk.Now()` usage with `time.Now()` for epoch-based timestamp generation.
Manifest `go.mod`	Implicit updates due to removed clock dependency (reflected in module changes).

Sequence Diagram(s)

(omitted — changes are refactors of time source and do not introduce new control-flow interactions)

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Attention areas:
- Tests that formerly relied on injected cached/test clocks (v2_ratelimit_multi_limit tests, Harness) may become time-sensitive or flaky.
- Verify uid timestamp semantics unchanged after switching to time.Now().
- Ensure no remaining references to the removed clock types/imports across the codebase.

Possibly related PRs

perf: cached clock #4315 — touches the same clock abstraction and cached-clock usage; likely directly related changes to clock API and its callers.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'perf: remove cached clock' directly and clearly summarizes the main change in the changeset, which is the removal of the cached clock implementation across multiple files.
Description check	✅ Passed	The pull request description provides the rationale (CPU profiling showed increased usage from cached clock timers), classifies the change type (bug fix), includes testing guidance ('Tests should still work'), and completes most required checklist items. The description adequately explains the motivation and change scope.

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9021e05 and db0e24b.

📒 Files selected for processing (1)

go/pkg/uid/uid.go (1 hunks)

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

github-actions · 2025-11-14T13:23:32Z

Thank you for following the naming conventions for pull request titles! 🙏

coderabbitai

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 49fecd3 and 9021e05.

📒 Files selected for processing (11)

go/apps/api/routes/register.go (0 hunks)
go/apps/api/routes/services.go (0 hunks)
go/apps/api/routes/v2_ratelimit_multi_limit/200_test.go (0 hunks)
go/apps/api/routes/v2_ratelimit_multi_limit/400_test.go (0 hunks)
go/apps/api/routes/v2_ratelimit_multi_limit/401_test.go (0 hunks)
go/apps/api/routes/v2_ratelimit_multi_limit/403_test.go (0 hunks)
go/apps/api/routes/v2_ratelimit_multi_limit/410_test.go (0 hunks)
go/apps/api/routes/v2_ratelimit_multi_limit/handler.go (3 hunks)
go/apps/api/run.go (0 hunks)
go/pkg/testutil/http.go (1 hunks)
go/pkg/uid/uid.go (1 hunks)

💤 Files with no reviewable changes (8)

go/apps/api/routes/v2_ratelimit_multi_limit/200_test.go
go/apps/api/routes/v2_ratelimit_multi_limit/410_test.go
go/apps/api/routes/services.go
go/apps/api/routes/register.go
go/apps/api/run.go
go/apps/api/routes/v2_ratelimit_multi_limit/401_test.go
go/apps/api/routes/v2_ratelimit_multi_limit/403_test.go
go/apps/api/routes/v2_ratelimit_multi_limit/400_test.go

🧰 Additional context used

🧠 Learnings (1)

📚 Learning: 2025-07-15T14:59:30.212Z

Learnt from: chronark
Repo: unkeyed/unkey PR: 3560
File: go/deploy/metald/internal/database/repository.go:0-0
Timestamp: 2025-07-15T14:59:30.212Z
Learning: go/deploy/metald cannot currently import helpers from go/pkg/db because it is not yet part of the main Go module; avoid suggesting such imports until the modules are unified.

Applied to files:

go/apps/api/routes/v2_ratelimit_multi_limit/handler.go

🧬 Code graph analysis (1)

go/pkg/testutil/http.go (2)

go/pkg/clock/interface.go (1)

Clock (12-18)

go/pkg/clock/test_clock.go (1)

TestClock (11-14)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)

GitHub Check: Test Go API Local / Test
GitHub Check: Test API / API Test Local
GitHub Check: Build / Build

🔇 Additional comments (2)

go/pkg/testutil/http.go (1)

39-39: LGTM! TestClock appropriately retained for test control.

The removal of CachedClock while keeping the TestClock field is correct. The TestClock provides controllable time for test fixtures (like line 334 setting up analytics timestamps), which is essential for deterministic tests. This aligns perfectly with the PR objective to remove the production cached clock that was causing CPU regression.

Also applies to: 334-334

go/apps/api/routes/v2_ratelimit_multi_limit/handler.go (1)

201-211: LGTM! Clean transition to direct time.Now() with test mode support.

The replacement of h.Clock.Now() with time.Now() at lines 201, 253, and 338 is correct and consistent. The test mode handling (lines 202-211) via the X-Test-Time header provides an alternative mechanism for controlling time in tests, maintaining test determinism without the overhead of the cached clock. This change directly addresses the CPU regression mentioned in the PR objectives.

Also applies to: 253-253, 338-338

go/pkg/uid/uid.go

graphite-app · 2025-11-14T14:21:17Z

graphite-app · 2025-11-14T14:25:13Z

Graphite Automations

"Post a GIF when PR approved" took an action on this PR • (11/14/25)

1 gif was posted to this PR based on Andreas Thomas's automation.

fix: remove cached clock

9021e05

Flo4604 requested review from chronark, imeyer and perkinsjr as code owners November 14, 2025 13:22

coderabbitai bot reviewed Nov 14, 2025

View reviewed changes

go/pkg/uid/uid.go Show resolved Hide resolved

fix: comment

0cc56b4

ogzhanolguncu approved these changes Nov 14, 2025

View reviewed changes

chronark approved these changes Nov 14, 2025

View reviewed changes

Merge branch 'main' into fix/remove-cached-clock

db0e24b

Flo4604 merged commit b02f2af into main Nov 14, 2025
19 of 20 checks passed

Flo4604 deleted the fix/remove-cached-clock branch November 14, 2025 14:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf: remove cached clock#4342

perf: remove cached clock#4342
Flo4604 merged 3 commits intomainfrom
fix/remove-cached-clock

Flo4604 commented Nov 14, 2025

Uh oh!

changeset-bot bot commented Nov 14, 2025 •

edited

Loading

Uh oh!

vercel bot commented Nov 14, 2025 •

edited

Loading

Uh oh!

coderabbitai bot commented Nov 14, 2025 •

edited

Loading

Review failed

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Possibly related PRs

Uh oh!

github-actions bot commented Nov 14, 2025 •

edited

Loading

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

graphite-app bot commented Nov 14, 2025

Uh oh!

graphite-app bot commented Nov 14, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

Flo4604 commented Nov 14, 2025

What does this PR do?

Type of change

How should this be tested?

Checklist

Required

Appreciated

Uh oh!

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

Uh oh!

⚠️ No Changeset found

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Review failed

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Possibly related PRs

Pre-merge checks and finishing touches

Uh oh!

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

Uh oh!

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

graphite-app bot commented Nov 14, 2025

Uh oh!

graphite-app bot commented Nov 14, 2025

Graphite Automations

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

changeset-bot bot commented Nov 14, 2025 •

edited

Loading

vercel bot commented Nov 14, 2025 •

edited

Loading

coderabbitai bot commented Nov 14, 2025 •

edited

Loading

github-actions bot commented Nov 14, 2025 •

edited

Loading