perf(coderd/rbac): build span role attributes only when recording (#27310)

`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.

<details>
<summary>Decision log</summary>

- 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.

</details>

---

Authored with Coder Agents.
This commit is contained in:
Jeremy Ruppel
2026-08-06 09:18:46 -04:00
committed by GitHub
parent 51a9aa1bfc
commit d9d6ce9ddf
+23 -21
View File
@@ -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)),