fix(manager): optimize durationToRenewal #6

ndyakov · 2025-05-27T23:13:43Z

I observed the durationToRenewal was significantly slower than before
due to changes done to cover the unnecessary precision. Using
Milliseconds as the TTL is sufficient and significantly faster.

Here are some benchmarks before the optimization:

goarch: arm64
pkg: github.com/redis/go-redis-entraid/manager
cpu: Apple M4 Max
BenchmarkTokenManager_GetToken-14               19352640                60.03 ns/op            0 B/op          0 allocs/op
BenchmarkTokenManager_Start-14                  249475408                4.825 ns/op           0 B/op          0 allocs/op
BenchmarkTokenManager_Close-14                  220564938                5.453 ns/op           0 B/op          0 allocs/op
BenchmarkTokenManager_durationToRenewal-14      12900028                93.18 ns/op            0 B/op          0 allocs/op
PASS
ok      github.com/redis/go-redis-entraid/manager       8.845s

And after:

goos: darwin
goarch: arm64
pkg: github.com/redis/go-redis-entraid/manager
cpu: Apple M4 Max
BenchmarkTokenManager_GetToken-14               37147389                31.68 ns/op            0 B/op          0 allocs/op
BenchmarkTokenManager_Start-14                  258121574                4.650 ns/op           0 B/op          0 allocs/op
BenchmarkTokenManager_Close-14                  226757744                5.254 ns/op           0 B/op          0 allocs/op
BenchmarkTokenManager_durationToRenewal-14      44128057                27.52 ns/op            0 B/op          0 allocs/op
PASS
ok      github.com/redis/go-redis-entraid/manager       9.097s

I observed the durationToRenewal was significantly slower than before due to changes done to cover the unnecessary precision. Using Milliseconds as the TTL is sufficient and significantly faster.

…tion-calculation

Copilot

Pull Request Overview

This PR shifts all token TTL calculations from seconds to milliseconds for improved precision and performance, and rewrites durationToRenewal to use pure integer arithmetic.

Convert TTL in tests and production code to use time.Millisecond units
Replace floating‐point math in durationToRenewal with integer‐based calculations
Bump example module version and remove unnecessary imports

Reviewed Changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
token_listener_test.go	Update test token TTL to milliseconds
token/token_test.go	Switch TTL calculation in tests to `UnixMilli`
token/token.go	Add doc comment noting TTL is in milliseconds
shared/identity_provider_response_test.go	Change TTL in identity provider test to ms
providers_test.go	Update provider tests to ms‐based TTL
manager/token_manager_test.go	Adjust test TTL and add brief waits in tests
manager/entraid_manager.go	Rewrite `durationToRenewal` using integer math
manager/defaults.go	Switch default TTL from Ceil(seconds) to ms
examples/custom_idp/go.mod	Bump `go-redis-entraid` version to v1.0.1
entraid_test.go	Update fake manager TTL to milliseconds
credentials_provider_test.go	Change TTL in provider tests to milliseconds

Comments suppressed due to low confidence (1)

manager/defaults.go:179

Switching from math.Ceil(time.Until(expiresOn).Seconds()) to .Milliseconds() changes rounding semantics (floor vs. ceiling). Verify this behavior change is intended or add documentation/tests to cover the new rounding.

time.Until(expiresOn).Milliseconds(),

manager/entraid_manager.go

manager/token_manager_test.go

Copilot

Pull Request Overview

This PR standardizes TTL units to milliseconds, improves durationToRenewal performance by using integer-based math, and updates tests and examples accordingly.

Convert TTL calculations from seconds to milliseconds across code and tests
Rewrite durationToRenewal to minimize allocations and use integer math
Bump example module version and update indirect dependencies

Reviewed Changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
token_listener_test.go	Update test TTL argument to `time.Hour.Milliseconds()`
token/token_test.go	Switch TTL in tests to `UnixMilli()` subtraction
token/token.go	Document that `ttl` is in milliseconds
shared/identity_provider_response_test.go	Change TTL to `time.Hour.Milliseconds()` in test
providers_test.go	Update several test TTL args to `.Milliseconds()`
manager/token_manager_test.go	Convert TTL to ms and insert small delays in streaming tests
manager/manager_test.go	Change `testTokenValid` TTL to `.Milliseconds()`
manager/entraid_manager.go	Refactor `durationToRenewal` to use ms-based integer math
manager/defaults.go	Remove unused `math` import; use `Milliseconds()` for TTL
examples/custom_idp/go.mod	Bump `go-redis-entraid` to v1.0.1 and indirect deps
entraid_test.go	Update fake token TTL to `.Milliseconds()`
credentials_provider_test.go	Change TTL args in various provider tests to ms

manager/entraid_manager.go

manager/token_manager_test.go

copilot thinks time.After can cause flakiness

Copilot

Pull Request Overview

This PR changes the token TTL from seconds to milliseconds and refactors durationToRenewal to use integer math on millisecond values for a significant performance boost.

Converted all TTL calculations and parameters to use time.*.Milliseconds().
Updated durationToRenewal to compute refresh timing with integer arithmetic on millisecond timestamps.
Bumped the example module’s dependency versions.

Reviewed Changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
token_listener_test.go	Updated test token TTL argument to milliseconds
token/token_test.go	Switched `Unix()/UnixMilli()` calculations for TTL
token/token.go	Added doc comment clarifying TTL is in milliseconds
shared/identity_provider_response_test.go	Updated test TTL initialization to milliseconds
providers_test.go	Switched provider test TTL arguments to milliseconds
manager/token_manager_test.go	Replaced `<-time.After` calls with `time.Sleep`; TTL updates
manager/entraid_manager.go	Refactored `durationToRenewal` to use millisecond math
manager/defaults.go	Updated default parser TTL to use milliseconds
examples/custom_idp/go.mod	Bumped module and indirect dependency versions
manager/manager_test.go	Updated test TTL initialization to milliseconds
entraid_test.go	Updated fake manager TTL initialization to milliseconds
credentials_provider_test.go	Switched credentials provider tests to millisecond TTL

Comments suppressed due to low confidence (2)

manager/entraid_manager.go:276

The variable name refreshRationInt has a typo; it should be refreshRatioInt for clarity.

refreshRationInt := int64(e.expirationRefreshRatio * 10000)

manager/token_manager_test.go:808

The raw token value was changed from "test" to "debug"; please verify this change is intentional and won’t affect test logic.

"debug",

manager/entraid_manager.go

manager/token_manager_test.go

Copilot

Pull Request Overview

This PR optimizes durationToRenewal by switching from second-precision TTL to millisecond-precision and rewriting the renewal calculation using integer math.

Changed TTL inputs across code and tests to use time.Duration.Milliseconds()
Rewrote durationToRenewal to operate on millisecond timestamps
Updated example module version and removed unused imports

Reviewed Changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
token/token.go	Documented TTL unit as milliseconds
token/token_test.go	Updated TTL calculation in tests to use milliseconds
manager/entraid_manager.go	Rewrote renewal logic to use millisecond arithmetic
manager/token_manager_test.go	Updated timed waits to `time.Sleep` and TTL inputs
defaults.go	Switched default TTL to milliseconds
Various test and example files	Replaced all TTLs and imports to millisecond usage

Comments suppressed due to low confidence (3)

token/token.go:12

Update the function doc comment to explicitly state that the ttl parameter is in milliseconds, matching the added inline comment.

// New creates a new token with the specified username, password, raw token, expiration time, received at time, and time to live.

token/token.go:14

[nitpick] Consider renaming the parameter ttl to ttlMillis to clearly indicate its unit and avoid confusion.

// ttl is in milliseconds.

manager/token_manager_test.go:808

The raw token literal was changed from "test" to "debug"; ensure this value change is intentional or revert to avoid breaking test expectations.

"debug",

manager/entraid_manager.go

github-actions · 2025-05-29T10:17:30Z

Merging this branch will increase overall coverage

Impacted Packages	Coverage Δ	🤖
github.com/redis/go-redis-entraid	93.18% (ø)
github.com/redis/go-redis-entraid/manager	98.34% (+0.11%)	👍
github.com/redis/go-redis-entraid/shared	100.00% (ø)
github.com/redis/go-redis-entraid/token	100.00% (ø)

Coverage by file

Changed files (no unit tests)

Changed File	Coverage Δ	Total	Covered	Missed	🤖
github.com/redis/go-redis-entraid/manager/defaults.go	100.00% (ø)	69	69	0
github.com/redis/go-redis-entraid/manager/entraid_manager.go	97.14% (+0.33%)	105 (+11)	102 (+11)	3	👍
github.com/redis/go-redis-entraid/manager/token_manager.go	100.00% (ø)	7	7	0
github.com/redis/go-redis-entraid/token/token.go	100.00% (ø)	14 (+1)	14 (+1)	0

Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code.

Changed unit test files

github.com/redis/go-redis-entraid/credentials_provider_test.go
github.com/redis/go-redis-entraid/entraid_test.go
github.com/redis/go-redis-entraid/manager/entraid_manager_test.go
github.com/redis/go-redis-entraid/manager/manager_test.go
github.com/redis/go-redis-entraid/manager/token_manager_test.go
github.com/redis/go-redis-entraid/providers_test.go
github.com/redis/go-redis-entraid/shared/identity_provider_response_test.go
github.com/redis/go-redis-entraid/token/token_test.go
github.com/redis/go-redis-entraid/token_listener_test.go

Copilot

Pull Request Overview

This PR optimizes the token renewal duration by switching TTL calculations from seconds to milliseconds and refines the durationToRenewal calculation for improved performance. Key changes include updating TTL conversions in tests and production code, refining refresh time arithmetic in token managers, and modifying lock types in manager structures.

Replaced seconds with time.Hour.Milliseconds() across the codebase.
Refined durationToRenewal computation using integer arithmetic with a precision constant.
Adjusted lock fields to pointer types in some structures.

Reviewed Changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
token_listener_test.go	Updated token initialization TTL conversion to milliseconds.
token/token_test.go	Converted TTL calculation from seconds to milliseconds.
token/token.go	Updated documentation to clarify TTL unit changes.
shared/identity_provider_response_test.go	Updated TTL conversion in test token initialization.
providers_test.go	Updated TTL conversion in various token initialization calls.
manager/token_manager_test.go	Updated benchmark and test calls to use milliseconds for TTL.
manager/token_manager.go	Refined durationToRenewal logic and ensured proper context cancelation.
manager/manager_test.go	Updated TTL conversion in test utilities.
entraid_manager.go	Added precision constant and modified lock field types; note spelling correction.
manager/defaults.go	Changed TTL calculation in default parser from seconds to milliseconds.
examples/custom_idp/go.mod	Updated dependency version to reflect new v1.0.1 release.
entraid_test.go	Updated TTL conversion in token creation in tests.
credentials_provider_test.go	Updated TTL conversion and lock usage in test mocks and utilities.

htemelski

🚀

ndyakov added 3 commits May 28, 2025 01:53

fix(manager): optimize token durationToRenewal calculation

67d856a

I observed the durationToRenewal was significantly slower than before due to changes done to cover the unnecessary precision. Using Milliseconds as the TTL is sufficient and significantly faster.

Merge remote-tracking branch 'origin/main' into ndyakov/optimize-dura…

c43e0de

…tion-calculation

chore(example): update deps in example

c950c66

ndyakov requested a review from Copilot May 27, 2025 23:13

Copilot AI reviewed May 27, 2025

View reviewed changes

manager/entraid_manager.go Outdated Show resolved Hide resolved

manager/token_manager_test.go Outdated Show resolved Hide resolved

ndyakov added 2 commits May 28, 2025 02:20

chore(tests): restore TestConcurrentTokenManagerOperations

3f6754f

fix(manager): use floating point for refresh ratio

9310819

ndyakov force-pushed the ndyakov/optimize-duration-calculation branch from 282a634 to 9310819 Compare May 27, 2025 23:31

ndyakov requested review from Copilot, htemelski and ofekshenawa May 27, 2025 23:32

Copilot AI reviewed May 27, 2025

View reviewed changes

manager/entraid_manager.go Outdated Show resolved Hide resolved

manager/entraid_manager.go Outdated Show resolved Hide resolved

manager/token_manager_test.go Outdated Show resolved Hide resolved

ndyakov added 2 commits May 28, 2025 02:37

refactor(tests): use time.Sleep when possible

fa01924

copilot thinks time.After can cause flakiness

fix(manager): use integer math with higher precision

7998ce1

ndyakov requested a review from Copilot May 27, 2025 23:42

Copilot AI reviewed May 27, 2025

View reviewed changes

manager/entraid_manager.go Outdated Show resolved Hide resolved

manager/entraid_manager.go Outdated Show resolved Hide resolved

manager/token_manager_test.go Show resolved Hide resolved

ndyakov requested a review from Copilot May 28, 2025 19:03

Copilot AI reviewed May 28, 2025

View reviewed changes

manager/entraid_manager.go Outdated Show resolved Hide resolved

fix(manager): precision as variable, fix typo

934c9f3

ndyakov force-pushed the ndyakov/optimize-duration-calculation branch from 70db08e to 934c9f3 Compare May 28, 2025 19:13

ndyakov added 5 commits May 28, 2025 22:47

test(manager): test durationToRenewal calculation

abf9f25

test(manager): add 100% coverage on GetToken

d3d6001

fix(manager): skip racy test and address comments

defa5a3

actually, the defer will be executed even if the function panics

4e61f88

chore(manager): improve comments

1c5016a

ndyakov requested a review from Copilot May 29, 2025 10:18

Copilot AI reviewed May 29, 2025

View reviewed changes

htemelski approved these changes May 29, 2025

View reviewed changes

ndyakov merged commit 8a4b7e1 into main May 29, 2025
7 checks passed

ndyakov deleted the ndyakov/optimize-duration-calculation branch May 30, 2025 09:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(manager): optimize durationToRenewal #6

fix(manager): optimize durationToRenewal #6

Uh oh!

ndyakov commented May 27, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

github-actions bot commented May 29, 2025

Changed files (no unit tests)

Changed unit test files

Uh oh!

Copilot AI left a comment

Uh oh!

htemelski left a comment

Uh oh!

Uh oh!

Uh oh!

fix(manager): optimize durationToRenewal #6

fix(manager): optimize durationToRenewal #6

Uh oh!

Conversation

ndyakov commented May 27, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

github-actions bot commented May 29, 2025

Merging this branch will increase overall coverage

Changed files (no unit tests)

Changed unit test files

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

htemelski left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!