From d9d6ce9ddf80ca89faf30424f9d9b5e0aa245412 Mon Sep 17 00:00:00 2001 From: Jeremy Ruppel Date: Thu, 6 Aug 2026 09:18:46 -0400 Subject: [PATCH] perf(coderd/rbac): build span role attributes only when recording (#27310) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `rbacTraceAttributes` materialized the subject's role names (one string allocation per role) and was passed into every `Filter`, `Authorize`, and `Prepare` span at creation time, so the O(roles) work ran even when no tracer was recording. It also called `SafeRoleNames()` twice. Replace it with `setRBACAttributes`, which attaches the same attributes *after* the span is created and only when `span.IsRecording()` is true, reading `SafeRoleNames()` once. Recorded spans are unchanged; untraced and unsampled calls skip the per-role work. This originated from #27309: once `/authcheck` checks are batched through `rbac.Filter`, each below-threshold group paid the role-attribute build for the `Filter` span *and* for every per-object `Authorize` span, so the redundant per-call work showed up as extra allocations per request. ## Benchmarks `AMD EPYC 9575F`, `benchstat`, no tracer configured (exercises the `IsRecording()==false` path). **`BenchmarkRBACManyOrgs`** (general RBAC eval), before vs after: wall time flat (geomean −0.04%), allocations strictly lower everywhere (geomean B/op −0.52%; `Authorize` −1.0 to −1.3% B/op), no regressions. **Authcheck path** (`BenchmarkAuthcheckGrouping`, #27309 vs #27310, back-to-back): this change is an **allocation reduction and is time-neutral**. On the endpoint (`Grouped`) path, per-request allocations drop ~4-5% B/op at common org counts (1-10); on the pure per-object path the reduction grows with org count (B/op −2.6% → −7.3% at 100 orgs). Wall time is flat within noise: low-org deltas sit inside this host's ±10-23% run-to-run variance, so no wall-time claim is made. Net: same speed, less garbage per request, which also lowers GC pressure under real concurrent load.
Decision log - The `Filter` span wraps the whole filtering routine (total latency + `num_objects`); it is the valuable span and is kept. The costly part was `rbacTraceAttributes`, not the span itself. - `rbacTraceAttributes` was O(roles): it allocated a string per role for the `subject_roles` attribute and called `SafeRoleNames()` twice. On `Filter`'s below-threshold fallback it ran once for the `Filter` span and again for each per-object `Authorize` span, so a group of N objects paid N+1 builds vs the old loop's N. Benchmarks confirm this as real per-call allocation; its wall-time cost is below the authcheck benchmark's noise floor. - Deferring attribute construction behind `IsRecording()` requires the span object, so the three callsites moved from `StartSpan(ctx, rbacTraceAttributes(...))` to `StartSpan(ctx)` then `setRBACAttributes(span, ...)`. No spans were removed or renamed; recorded output is identical. - Tradeoff: when a span is not recording, `subject_roles`/`num_subject_roles`/etc. are not computed. Unsampled spans emit nothing anyway, so there is no observable output change.
--- Authored with Coder Agents. --- coderd/rbac/authz.go | 44 +++++++++++++++++++++++--------------------- 1 file changed, 23 insertions(+), 21 deletions(-) diff --git a/coderd/rbac/authz.go b/coderd/rbac/authz.go index 55998ef5e1..d2c5e6da8b 100644 --- a/coderd/rbac/authz.go +++ b/coderd/rbac/authz.go @@ -246,16 +246,13 @@ func Filter[O Objecter](ctx context.Context, auth Authorizer, subject Subject, a // Start the span after the object type is detected. If we are filtering 0 // objects, then the span is not interesting. It would just add excessive // 0 time spans that provide no insight. - ctx, span := tracing.StartSpan(ctx, - rbacTraceAttributes(subject, action, objectType, - // For filtering, we are only measuring the total time for the entire - // set of objects. This and the 'Prepare' span time - // is all that is required to measure the performance of this - // function on a per-object basis. - attribute.Int("num_objects", len(objects)), - ), - ) + ctx, span := tracing.StartSpan(ctx) defer span.End() + // For filtering, we are only measuring the total time for the entire set of + // objects. This and the 'Prepare' span time is all that is required to + // measure the performance of this function on a per-object basis. + setRBACAttributes(span, subject, action, objectType, + attribute.Int("num_objects", len(objects))) // Below the threshold, a full evaluation per object is faster than paying // the Prepare overhead once and reusing it. @@ -443,14 +440,13 @@ func (a RegoAuthorizer) Authorize(ctx context.Context, subject Subject, action p start := time.Now() ctx, span := tracing.StartSpan(ctx, trace.WithTimestamp(start), // Reuse the time.Now for metric and trace - rbacTraceAttributes(subject, action, object.Type, - // For authorizing a single object, this data is useful to know how - // complex our objects are getting. - attribute.Int("object_num_groups", len(object.ACLGroupList)), - attribute.Int("object_num_users", len(object.ACLUserList)), - ), ) defer span.End() + // For authorizing a single object, this data is useful to know how complex + // our objects are getting. + setRBACAttributes(span, subject, action, object.Type, + attribute.Int("object_num_groups", len(object.ACLGroupList)), + attribute.Int("object_num_users", len(object.ACLUserList))) err := a.authorize(ctx, subject, action, object) authorized := err == nil @@ -508,9 +504,9 @@ func (a RegoAuthorizer) Prepare(ctx context.Context, subject Subject, action pol start := time.Now() ctx, span := tracing.StartSpan(ctx, trace.WithTimestamp(start), - rbacTraceAttributes(subject, action, objectType), ) defer span.End() + setRBACAttributes(span, subject, action, objectType) prepared, err := a.newPartialAuthorizer(ctx, subject, action, objectType) if err != nil { @@ -825,18 +821,24 @@ func (c *authCache) Prepare(ctx context.Context, subject Subject, action policy. return c.authz.Prepare(ctx, subject, action, objectType) } -// rbacTraceAttributes are the attributes that are added to all spans created by -// the rbac package. These attributes should help to debug slow spans. -func rbacTraceAttributes(actor Subject, action policy.Action, objectType string, extra ...attribute.KeyValue) trace.SpanStartOption { +// setRBACAttributes attaches the attributes added to all spans created by the +// rbac package, to help debug slow spans. It only does the work when the span +// is recording: materializing the subject's role names allocates one string +// per role, so untraced calls (benchmarks, unsampled spans) skip the O(roles) +// cost entirely. +func setRBACAttributes(span trace.Span, actor Subject, action policy.Action, objectType string, extra ...attribute.KeyValue) { + if !span.IsRecording() { + return + } uniqueRoleNames := actor.SafeRoleNames() roleStrings := make([]string, 0, len(uniqueRoleNames)) for _, roleName := range uniqueRoleNames { roleStrings = append(roleStrings, roleName.String()) } - return trace.WithAttributes( + span.SetAttributes( append(extra, attribute.StringSlice("subject_roles", roleStrings), - attribute.Int("num_subject_roles", len(actor.SafeRoleNames())), + attribute.Int("num_subject_roles", len(uniqueRoleNames)), attribute.Int("num_groups", len(actor.Groups)), attribute.String("scope", actor.SafeScopeName()), attribute.String("action", string(action)),