Skip to content

feat: Add authenticated environment attributes to relay.auth span and metrics - #781

Draft
keelerm84 wants to merge 6 commits into
v9from
mk/SDK-2774/add-env-attribute
Draft

feat: Add authenticated environment attributes to relay.auth span and metrics#781
keelerm84 wants to merge 6 commits into
v9from
mk/SDK-2774/add-env-attribute

Conversation

@keelerm84

@keelerm84 keelerm84 commented Jul 29, 2026

Copy link
Copy Markdown
Member

Summary

Emit the authenticated environment as telemetry attributes so traces and metrics can be correlated by environment.

  • relay.auth span: on successful auth, both auth middlewares now set relay.auth.environment.name (always, the human-readable display name) and relay.auth.environment.id (only when an EnvironmentID credential is configured). The relay.auth.result=success attribute remains unconditional.
  • Metrics: the per-environment attribute set keeps the existing environment.name and now also emits environment.id when available.

The environment ID may be absent for SDK-key-only environments in a manual configuration, so it is only added when present.


Note

Low Risk
Observability-only attribute and middleware changes with broad test coverage; no auth or request-handling behavior changes beyond span/metric labels.

Overview
Adds shared environment.name and environment.id telemetry so authenticated traffic can be joined across signals.

On successful auth, both server-side and client-side middleware call setEnvSpanAttributes on the request span (the otelmux parent), not on relay.auth, so every handler span in the trace inherits filterable environment context. Names use tracing.SanitizeAttributeValue (same rules as metrics); environment.id is omitted for SDK-key-only environments.

Metrics gain optional environment.id, reuse the same attribute keys from internal/tracing, and AddEnvironment now accepts an env ID. Per-environment attributes are immutable snapshots behind an atomic pointer; SetEnvironmentName (from SetIdentifiers on upstream renames) starts a new metric series under the new name while preserving env ID. Event metrics read attributes at record time so renames apply to async event counters too.

docs/metrics.md documents trace/metric correlation and rename behavior.

Reviewed by Cursor Bugbot for commit 688a491. Bugbot is set up for automated code reviews on this repo. Configure here.

@keelerm84
keelerm84 changed the base branch from v8 to v9 July 29, 2026 18:36
@keelerm84
keelerm84 marked this pull request as ready for review July 30, 2026 15:43
@keelerm84
keelerm84 requested a review from a team as a code owner July 30, 2026 15:43
Comment thread internal/middleware/middleware.go Outdated
The relay.auth span reported the environment display name verbatim while the
environment.name metric attribute reported it through the metrics package's
sanitizer, so the same environment could appear under two different values --
display names are user-supplied and may contain slashes or be blank.

Promote the sanitizer to internal/tracing as SanitizeAttributeValue, the shared
low-level telemetry package, and use it from both the metrics attribute set and
the auth span attributes.
- Trim surrounding whitespace in SanitizeAttributeValue. It trimmed only for
  the blank check and returned the original string, so an auto-configured
  environment with no project name (display name " Production") became its own
  time series while rendering identically to "Production".
- Copy the environment attributes in GetAttributes before attribute.NewSet,
  which sorts the slice it is given in place. NewEventMetricsRecorder already
  guards against this; GetAttributes shared the slice.
- Document environment.id in docs/metrics.md, including that it is absent for
  environments with no client-side ID and shared across payload-filter variants.
- Do not claim spans and metrics always report the same environment name: the
  metric attribute set is built when the environment is created while the span
  reads the identifiers per request, so a rename in auto-configuration or
  offline mode diverges until restart.
- Cover the exported span with a tracer-provider recorder: attributes survive
  the round trip, the display name is sanitized, and the ID is omitted when no
  EnvironmentID is configured.
The per-environment metric attributes were built once when the environment was
created, while the relay.auth span reads the identifiers per request. After a
rename arrived from auto-configuration or offline mode via SetIdentifiers, the
span reported the new name and every metric kept reporting the old one until
restart.

SetIdentifiers now rebuilds the environment's metric attributes. Attributes are
held in an immutable snapshot behind an atomic pointer, so recording paths get a
consistent set without locking, and the event metrics recorder reads the snapshot
per record instead of capturing a private copy at construction -- otherwise event
metrics alone would have kept the old name.

Renaming starts a new metric time series under the new name; the old series stops
receiving data points rather than being relabeled. That is now documented in
docs/metrics.md.

The environment ID cannot change for a live environment -- the key rotator is
seeded from EnvConfig.EnvID and rotation only replaces SDK and mobile keys -- so
it is carried over rather than re-derived.
…span

The auth span ends before the next handler runs, so environment attributes set
on it covered nothing else in the trace. Set them on the request span -- the
parent of the auth span and of every handler span below it -- so a whole trace
can be filtered by environment.

The keys are now the same ones the metrics use, environment.name and
environment.id, moved to internal/tracing so both packages share one
definition. These replace the relay.auth.environment.name and
relay.auth.environment.id attributes added earlier in this branch, so nothing
released changes. relay.auth keeps reporting the auth outcome.

Covered end to end in the relay package, where the real otelmux request span is
the root, for both a server-side SDK key and a client-side environment ID.
@keelerm84
keelerm84 force-pushed the mk/SDK-2774/add-env-attribute branch from 70b8963 to 688a491 Compare August 3, 2026 13:53
@keelerm84
keelerm84 marked this pull request as draft August 11, 2026 16:37
@keelerm84

Copy link
Copy Markdown
Member Author

Context from a now-abandoned branch that may be useful to a reviewer: the attribute-snapshot rework here is load-bearing, not cosmetic.

While building an eventsource ServerTrace OTel bridge (#801, since closed unmerged), a multi-agent review found that EnvironmentManager.GetAttributes() on v9 is attribute.NewSet(em.envKVs...), and attribute.NewSet sorts the slice it is given in place (otel v1.44.0 attribute/set.go:238, plus element swaps at :256 that run even when keys are unique and already ordered). So that method is a write to envKVs, not a read of it.

On v9 today it is dormant only because GetAttributes() has zero production callers -- it is reachable only from metrics_test.go. #801 added a single innocuous-looking caller (registerStreamChannel, on the credential-rotation goroutine) and that was enough to produce a real data race against the request path, which reads em.envKVs unsynchronized via buildRequestAttributes / buildDurationAttributes on every request goroutine:

WARNING: DATA RACE
Write by goroutine 11: slices.SortStableFunc -> attribute.NewSetWithFiltered (set.go:238)
  -> metrics.(*EnvironmentManager).GetAttributes (metrics.go:297)
Previous read by goroutine 15: runtime.slicecopy
  -> metrics.buildRequestAttributes (constants.go:67)

Two notes in this PR's favour:

  1. Returning a precomputed set from the immutable snapshot removes the sort-in-place write entirely, so a future caller cannot reintroduce the race by accident. That is the part that mattered for feat: Add an OTel bridge for eventsource ServerTrace hooks #801.
  2. The capacity clip (kvs[:len(kvs):len(kvs)]) closes a second door that a simple defensive copy would leave open: an append to a slice with spare capacity writing into a slot another reader can see. Worth keeping, and worth keeping the comment that explains it.

Also worth noting the reorder is a genuine permutation rather than a same-value self-swap, since envKVs is built [relay.id, environment.name], which is not in key order.

One caveat on the evidence: the race itself is proven under -race, but the downstream consequence (a torn multi-word KeyValue read producing a mislabeled point) is not -- 24k iterations produced zero corrupt sets, so the honest framing is undefined behavior the detector will flag, not "metrics are wrong today."

Finally: internal/metrics/proof_rename_test.go on this branch already covers the regression (TestProofRaceRenameWhileRecording drives WithGauge, RecordRequestDuration, RecordEventsReceivedBytes, the EventMetricsRecorder methods and GetAttributes itself, 4 goroutines each, against concurrent SetEnvironmentName). It passes under -race here. It is currently an untracked proof_ file, so it would need committing under a normal name to actually guard this going forward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants