-
Notifications
You must be signed in to change notification settings - Fork 190
refactor: [OM-387] governance entitlement performance improvement 2 #4637
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: refactor/OM-387-governance-entitlement-performance-improvement
Are you sure you want to change the base?
Changes from all commits
5eebfa5
d779393
b7c2f2b
e79ec25
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -334,18 +334,36 @@ func (c *service) expandCustomers(ctx context.Context, entitlements []entitlemen | |||||||||||||||||
| return custs, nil | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| func (c *service) GetAccess(ctx context.Context, namespace string, customerId string) (entitlement.Access, error) { | ||||||||||||||||||
| func (c *service) GetAccess(ctx context.Context, namespace string, customerId string, featureKeys ...string) (entitlement.Access, error) { | ||||||||||||||||||
| now := clock.Now() | ||||||||||||||||||
|
|
||||||||||||||||||
| entitlements, err := c.GetEntitlementsOfCustomer(ctx, namespace, customerId, now) | ||||||||||||||||||
| if err != nil { | ||||||||||||||||||
| return entitlement.Access{}, err | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| // When the caller only needs a subset of features, drop the rest before the fan-out so we | ||||||||||||||||||
| // don't compute (and issue ClickHouse usage queries for) balances that would be discarded. | ||||||||||||||||||
| // Empty featureKeys means "all" — the fetch is a single query regardless, so filtering its | ||||||||||||||||||
| // result in memory is enough; the per-entitlement value calc is the cost this avoids. | ||||||||||||||||||
| if len(featureKeys) > 0 { | ||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would extend the
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems the |
||||||||||||||||||
| wanted := lo.SliceToMap(featureKeys, func(k string) (string, struct{}) { return k, struct{}{} }) | ||||||||||||||||||
| entitlements = lo.Filter(entitlements, func(ent entitlement.Entitlement, _ int) bool { | ||||||||||||||||||
| _, ok := wanted[ent.FeatureKey] | ||||||||||||||||||
| return ok | ||||||||||||||||||
| }) | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| if len(entitlements) == 0 { | ||||||||||||||||||
| return entitlement.Access{}, nil | ||||||||||||||||||
| } | ||||||||||||||||||
|
Comment on lines
357
to
359
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win Preserve the empty access map shape. This new early return yields a zero-value 🐛 Suggested fix if len(entitlements) == 0 {
- return entitlement.Access{}, nil
+ return entitlement.Access{
+ Entitlements: map[string]entitlement.EntitlementValueWithId{},
+ }, nil
}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||
|
|
||||||||||||||||||
| // This fans out over all of the customer's entitlements; each metered entitlement's | ||||||||||||||||||
| // DescribeOwner would otherwise re-fetch the same customer once per entitlement. Memoize the | ||||||||||||||||||
| // customer for the duration of this (read-only) call so the fan-out shares one lookup. | ||||||||||||||||||
| // Installed before errgroup.WithContext so the derived ctx carries the memo into every goroutine. | ||||||||||||||||||
| ctx = meteredentitlement.WithCustomerCache(ctx) | ||||||||||||||||||
|
|
||||||||||||||||||
| var result sync.Map | ||||||||||||||||||
|
|
||||||||||||||||||
| g, ctx := errgroup.WithContext(ctx) | ||||||||||||||||||
|
|
||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,71 @@ | ||
| package syncx | ||
|
|
||
| import ( | ||
| "context" | ||
| "sync" | ||
| ) | ||
|
|
||
| // ContextMemo is a request-scoped, keyed memoizer. | ||
| // | ||
| // Install it into a context (Install) to enable caching for that context's lifetime; without | ||
| // installation GetOrLoad falls back to a direct call, so callers that do not opt in are | ||
| // unaffected. Each ContextMemo value carries its own private context key (pointer identity), so | ||
| // several memos — even of the same K/V types — coexist in one context without colliding. Entries | ||
| // are computed at most once per key (sync.Once) and shared across concurrent goroutines. | ||
| // | ||
| // Typical use: dedup repeated lookups of the same immutable entity within one request/operation | ||
| // (e.g. the same customer fetched once per entitlement across a fan-out). | ||
| // | ||
| // INVARIANT — install ONLY around read-only operations. Cached values are never invalidated | ||
| // within a scope, so installing a memo around a call stack that MUTATES the cached entity would | ||
| // make reads after the write return the stale, pre-write value. | ||
| type ContextMemo[K comparable, V any] struct { | ||
| key *contextMemoKey | ||
| } | ||
|
|
||
| // contextMemoKey is an unexported context-key type. It carries a field so it is NOT zero-sized: | ||
| // Go does not guarantee distinct addresses for zero-size allocations, so an empty struct would let | ||
| // two memos share a key and collide. A non-zero size makes each &contextMemoKey{} address unique, | ||
| // giving every ContextMemo its own context key even across identical K/V type parameters. | ||
| type contextMemoKey struct{ _ byte } | ||
|
|
||
| type contextMemoEntry[V any] struct { | ||
| once sync.Once | ||
| val V | ||
| err error | ||
| } | ||
|
|
||
| // NewContextMemo returns a ContextMemo with its own private context key. Create one per logical | ||
| // cache (e.g. package-level var) and reuse it; the value is safe for concurrent use. | ||
| func NewContextMemo[K comparable, V any]() *ContextMemo[K, V] { | ||
| return &ContextMemo[K, V]{key: &contextMemoKey{}} | ||
| } | ||
|
|
||
| // Install returns a context with this memo's store enabled. Nested installs are a no-op: the | ||
| // outermost scope wins and its store is shared by everything below it. | ||
| func (m *ContextMemo[K, V]) Install(ctx context.Context) context.Context { | ||
| if _, ok := ctx.Value(m.key).(*sync.Map); ok { | ||
| return ctx | ||
| } | ||
|
|
||
| return context.WithValue(ctx, m.key, &sync.Map{}) | ||
| } | ||
|
|
||
| // GetOrLoad returns the memoized value for k when a store is installed in ctx; otherwise it calls | ||
| // load directly (no-cache fallback). load runs at most once per key within an installed scope, | ||
| // even under concurrency; both the value and the error are cached for the scope's lifetime. | ||
| func (m *ContextMemo[K, V]) GetOrLoad(ctx context.Context, k K, load func(context.Context) (V, error)) (V, error) { | ||
| store, ok := ctx.Value(m.key).(*sync.Map) | ||
| if !ok { | ||
| return load(ctx) | ||
| } | ||
|
|
||
| v, _ := store.LoadOrStore(k, &contextMemoEntry[V]{}) | ||
| entry := v.(*contextMemoEntry[V]) | ||
|
|
||
| entry.once.Do(func() { | ||
| entry.val, entry.err = load(ctx) | ||
| }) | ||
|
|
||
| return entry.val, entry.err | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Refresh the over-compute description.
After this PR, this benchmark should exercise the fixed selective path, not say
GetAccessstill computes all 50 balances. That wording will confuse future trace comparisons.📝 Suggested wording
📝 Committable suggestion
🤖 Prompt for AI Agents