From 9031b9e9de006095e0150ce9afd16006e2b2bff5 Mon Sep 17 00:00:00 2001 From: mgravell Date: Thu, 17 Sep 2026 14:07:14 +0100 Subject: [PATCH 1/8] Start a notes/ convention, and write up where OpenTelemetry actually stands notes/ mirrors the split protobuf-net uses: docs/ is the published Jekyll site (so anything there lands in sitemap.xml), notes/ is handover material that is not built and not indexed. Topic per subdirectory, files named for content. First topic is otel, prompted by #1044 being revived: findings.md records what the contrib bridge actually does and what it costs, what the .NET and semconv state of the art is, how far System.Diagnostics.DiagnosticSource reaches across our TFMs, and what go-redis, Lettuce and redis-py do - the short version being that there is no cross-language vocabulary beyond semconv, and adoption of that lags unevenly. plan.md is the proposal that follows from it. --- notes/otel/findings.md | 268 +++++++++++++++++++++++++++++++++++++++++ notes/otel/plan.md | 223 ++++++++++++++++++++++++++++++++++ notes/readme.md | 23 ++++ 3 files changed, 514 insertions(+) create mode 100644 notes/otel/findings.md create mode 100644 notes/otel/plan.md create mode 100644 notes/readme.md diff --git a/notes/otel/findings.md b/notes/otel/findings.md new file mode 100644 index 000000000..a876dc5f4 --- /dev/null +++ b/notes/otel/findings.md @@ -0,0 +1,268 @@ +# OpenTelemetry: where things stand + +Research notes, September 2026. Background reading for [`plan.md`](plan.md); this file is the +evidence, that one is the decision. + +The conversation this came out of is [#1044](https://github.com/StackExchange/StackExchange.Redis/issues/1044) +("Allow to register global profiler", open since 2019, revived by @martincostello on 2026-09-17). + +## 1. What the OpenTelemetry bridge does today, and what it costs + +`OpenTelemetry.Instrumentation.StackExchangeRedis` is a community package in +[opentelemetry-dotnet-contrib](https://github.com/open-telemetry/opentelemetry-dotnet-contrib/tree/main/src/OpenTelemetry.Instrumentation.StackExchangeRedis). +It has exactly one hook into us: `IConnectionMultiplexer.RegisterProfiler`. + +Everything else it does is machinery to reconstruct, after the fact, causality that we already +have in hand at the moment the command completes. From +`StackExchangeRedisConnectionInstrumentation.cs`: + +- a **dedicated background thread per multiplexer**, draining on a timer (`FlushInterval`, + default 10s); +- a `ConcurrentDictionary<(ActivityTraceId, ActivitySpanId), (Activity, ProfilingSession, Baggage)>`, + allocating **one `ProfilingSession` per parent span** so that drained commands can be attributed + back to the right caller; +- `Baggage.Current` captured at session creation and manually restored on the drain thread around + each `StartActivity`, because the thread doing the draining is not the thread that issued the + command; +- `ExecutionContext.SuppressFlow()` around `Thread.Start()`, so the drain thread does not + permanently inherit the baggage of whichever request happened to create the multiplexer; +- spans created **retroactively**, with explicit `startTime` and `SetEndTime`, so they surface up + to `FlushInterval` late and the sampler sees them out of band; +- commands issued with no ambient `Activity` all funnel into a single shared `defaultSession`. + +The last year of their changelog is mostly this machinery being debugged — a `ProfilingSession` +race (contrib #5117), empty `Baggage.Current` on command activities (#4927), draining performance +(#4398), `FlushInterval` validation (#4860), `Enrich` callbacks throwing (#4900), disposal +idempotency (#4905). + +None of that is their fault. It is the shape you are forced into when the only available hook is a +session object that you have to poll. + +### The reflection, precisely + +There is one reflection site: `RedisProfilerEntryToActivityConverter.MessageDataGetter`, and it +only runs when the caller opts into `SetVerboseDatabaseStatements` (default `false`). It: + +1. resolves `StackExchange.Redis.Profiling.ProfiledCommand` by name and emits a `DynamicMethod` + getter for its private `Message` field; +2. uses `PropertyFetcher("CommandAndKey")` against that `Message`; +3. resolves `RedisDatabase+ScriptEvalMessage` and reads its private `script` field, for the + EVAL/EVALSHA text. + +#3211 renamed `ScriptEvalMessage` to `ScriptEvaluateMessage` and reused the old name for the new +pooled-buffer type whose field is `_script`, so (3) silently started returning null in 3.2.0. Their +fix (contrib 1.18.0-beta.3) probes both type names and both field names. The blast radius was +script text on verbose statements only. + +[#3119](https://github.com/StackExchange/StackExchange.Redis/issues/3119) ("Telemetry is gone") was +a separate consumer — Datadog's App Service extension — with the same root cause. + +**The only thing we do not already expose publicly is that statement string.** Everything else the +converter reads comes off `IProfiledCommand`. + +## 2. The .NET state of the art + +`System.Diagnostics.ActivitySource` and `System.Diagnostics.Metrics.Meter` (both in +`System.Diagnostics.DiagnosticSource`) *are* the .NET tracing and metrics API. OpenTelemetry .NET +does not define its own; `AddSource(...)` / `AddMeter(...)` attach an `ActivityListener` / +`MeterListener` to what the runtime already provides. + +Microsoft's guidance to library authors is explicit +([docs](https://learn.microsoft.com/en-us/dotnet/core/diagnostics/distributed-tracing-instrumentation-walkthroughs)): + +> .NET library authors can exclusively rely on APIs in System.Diagnostics.DiagnosticSource, which +> is part of .NET runtime. This ensures that libraries will run in a wide range of .NET apps, +> regardless of the app developer's preferences about which library or vendor to use for +> collecting telemetry. + +OpenTelemetry's own +[library guidelines](https://opentelemetry.io/docs/specs/otel/library-guidelines/) agree that an +instrumented library must remain fully usable with no telemetry SDK present. + +This settles the "what if library X goes out of fashion" worry. `Activity` predates the +OpenTelemetry specification, is owned by the runtime, and Datadog, Application Insights, Elastic +and New Relic already listen to it. Emitting `Activity`/`Meter` couples us to the BCL, not to a +vendor. + +Relevant mechanics: + +- `ActivitySource.StartActivity` returns `null` when nothing is listening, and + `ActivitySource.HasListeners()` is a cheap pre-check. +- `Activity.IsAllDataRequested` says whether any listener intends to read tags — the gate for + building anything expensive, such as the statement text. +- `StartActivity` **sets `Activity.Current`**. That matters for us; see §5. + +### Does it reach our whole TFM base? + +We target `net461; netstandard2.0; net472; net8.0; net10.0`. Package assets, checked against +nuget.org: + +| SE.Redis TFM | how it would resolve | verdict | +| --- | --- | --- | +| `net10.0` | in-box — `System.Diagnostics.DiagnosticSource.dll` is in the `Microsoft.NETCore.App` ref pack | no `PackageReference` needed | +| `net8.0` | in-box, same | no `PackageReference` needed | +| `net472` | package, `lib/net462` asset | fine | +| `netstandard2.0` | package, `lib/netstandard2.0` asset | fine | +| `net461` | package, falls back to `lib/netstandard2.0` | **not supported** | + +`System.Diagnostics.DiagnosticSource` dropped its `net461` asset after 6.0.1 — 8.0.x, 9.0.x and +10.0.x ship `net462` as the lowest .NET Framework target. NuGet *will* hand a net461 project the +`netstandard2.0` asset, but that is the combination Microsoft stopped supporting and the one that +needs the `netstandard.dll` facade plus binding redirects to behave. + +So: telemetry everywhere except `net461`, which compiles the instrumentation out. That matches the +existing `VECTOR_SAFE` / `UNIX_SOCKET` idiom in `StackExchange.Redis.csproj` and costs net461 users +nothing they have today. + +## 3. Semantic conventions + +Two documents apply, both in +[open-telemetry/semantic-conventions](https://github.com/open-telemetry/semantic-conventions): + +**Spans** (`docs/db/redis.md`, over `docs/db/database-spans.md`). Span kind `CLIENT`; span name +follows the general database convention **except** that `db.namespace` is deliberately left out of +the name, because for Redis it is a bare integer and reads as noise. + +| attribute | requirement level | where we get it | +| --- | --- | --- | +| `db.system.name` = `redis` | required | constant | +| `db.operation.name` | required, un-normalised case | `Message.CommandString` | +| `db.namespace` | conditionally required | `Message.Db` | +| `db.response.status_code` | conditionally required (on failure) | Redis error prefix — **we do not expose this today** | +| `error.type` | conditionally required (on failure) | fault type — **we do not expose this today** | +| `server.address`, `server.port` | recommended / cond. required | `ServerEndPoint.EndPoint` | +| `network.peer.address`, `network.peer.port` | recommended | ditto | +| `db.query.text` | recommended | `Message.CommandAndKey` (+ script) — the reflection target | +| `db.operation.batch.size` | recommended | batch / transaction size — **not exposed today** | + +For batches the convention is to prepend `MULTI` or `PIPELINE` to the span name when the +constituent operations share a command. + +**Metrics** (`docs/db/database-metrics.md`). `db.client.operation.duration` is a histogram in +seconds and is **stable**, with recommended buckets +`[0.001, 0.005, 0.01, 0.05, 0.1, 0.5, 1, 5, 10]`; required attribute `db.system.name`, plus +`db.operation.name`, `db.namespace`, `server.address`/`server.port`, `error.type` on failure. The +`db.client.connection.*` family (count, idle.max/min, limit, pending_requests, timeouts, +create_time, wait_time, use_time) is still *development* status. + +Note the shape of that: duration is a **histogram**, so it is O(1) memory regardless of +throughput. Metrics are the half of this that can be left on permanently at Stack Overflow scale; +spans are the half that needs sampling. + +### The migration tax is real + +The contrib package currently declares **three** `ActivitySource`s — one at semconv 1.23.0 emitting +`db.system`/`db.statement`, one at 1.42.0 emitting `db.system.name`/`db.operation.name`/ +`db.query.text`, and one emitting both — driven by `EmitOldAttributes`/`EmitNewAttributes`. If we +own the source, we own that kind of migration. The counter-argument is that the new names are now +the stable ones and the dual-emit window is closing; but it is the strongest single argument for +leaving span production to someone whose job is tracking semconv. + +## 4. What other Redis clients do + +Worth knowing because it tells us (a) whether a client shipping its own telemetry is normal, and +(b) whether there is a cross-language vocabulary we would be breaking by inventing our own. + +**go-redis** — closest to the "utopian" end. `redis.Hook` is a first-class public interface on the +client (`DialHook`, `ProcessHook`, `ProcessPipelineHook`), and `redisotel` is an official module in +the go-redis repo itself. Notable choices: span name is `cmd.FullName()`; connection establishment +gets its own `redis.dial` span; a pipeline is **one** span named `redis.pipeline {summary}` with +`db.redis.num_cmd`. Attributes are still old-semconv — `db.statement`, `db.connection_string` — +alongside `server.address`/`server.port`. + +**Lettuce (Java)** — the "middle ground" in its purest form. Lettuce defines a vendor-neutral +tracing SPI in the client (`io.lettuce.core.tracing.Tracing`: `getTracerProvider()`, +`initialTraceContextProvider()`, `isEnabled()`, `includeCommandArgsInSpanTags()`, +`createEndpoint(SocketAddress)`), explicitly so that tracing libraries are not a mandatory +dependency, and ships `BraveTracing` and `MicrometerTracing` adapters against it. Configured via +`ClientResources`, with endpoint and span customisers and an include/exclude switch for command +arguments. + +**redis-py** — furthest from it. `opentelemetry-instrumentation-redis` monkeypatches +`Redis.execute_command`, `Pipeline.execute`, cluster and asyncio variants via +`wrapt.wrap_function_wrapper`. Carries its own `db.redis.args_length` and +`db.redis.pipeline_length` attributes, and has a semconv opt-in mode for old-vs-new names, same as +.NET. + +**Us** — a contrib package reaching through a profiling API with reflection. + +Takeaways: + +1. A client library shipping or blessing its own telemetry is normal, not novel. go-redis does it + in-repo; Lettuce does it via an SPI plus adapters. +2. **There is no agreed cross-language vocabulary beyond the semantic conventions themselves**, and + adoption of those lags badly and unevenly — go-redis is still on `db.statement`, Python has a + dual-mode switch, .NET has a triple-source switch. Following current semconv puts us *ahead* of + the field, not out of step with it. +3. Where the clients *do* informally agree, it is on things semconv does not mandate: span name is + the bare command name, pipelines/batches collapse to one span with a count attribute, and + argument capture is opt-in because of PII. We should match those three. +4. Both of the good implementations trace **connection establishment** as well as commands + (go-redis `redis.dial`; Lettuce via endpoint resolution). We have much richer material there + than either. + +## 5. Where we would hook, and the one non-obvious wrinkle + +Existing sites, all of which are already the right ones: + +- `ConnectionMultiplexer.cs:~2397` — after server selection, still on the caller's thread. This is + where `ProfiledCommand.NewWithContext` is attached today. +- `Message.SetProfileStorage` / `Message.CreatedTimestamp` — per-message state and start time. +- `Message.Complete(PhysicalConnection?)` — on the reader thread; already has `currBox.Fault` in + hand, which is where `error.type` and `db.response.status_code` come from. +- `Message.SetExceptionAndComplete` — the failure path. +- `Message.PrepareToResend` — `-MOVED` / `-ASK` retransmission, which the contrib converter has a + literal `// TODO: deal with the re-transmission` for. + +**The wrinkle:** do not start the `Activity` on the caller's thread. `ActivitySource.StartActivity` +assigns `Activity.Current`, and for a multiplexed client whose command completes on a different +thread there is no scope to dispose — so the caller's async flow would inherit our span and we +would have to save and restore an `AsyncLocal` on the hot path. + +Instead: + +- **caller thread**, only when `HasListeners()`: capture `Activity.Current?.Context` — an + `ActivityContext` struct, no `AsyncLocal` write — plus the start timestamp we already record. +- **reader thread, at completion**: `StartActivity(name, ActivityKind.Client, parentContext, tags, + startTime)` → `SetEndTime` → `Stop()`. + +Because the parent is supplied as a *context* rather than a parent `Activity`, the started activity +has no in-process parent, so `Stop()` restores `Activity.Current` to null — which is what the +reader thread had. It cleans up after itself. + +This is the same deferred-creation shape the contrib drain thread uses, minus the thread, the +dictionary, the 10s latency and the baggage restoration — because we are already on the completion +path with the caller's context in hand. + +## 6. The two asks on the table + +From [#1044](https://github.com/StackExchange/StackExchange.Redis/issues/1044#issuecomment-5713538582), +@martincostello: + +> In the *ideal* case the OpenTelemetry instrumentation would actually become obsolete because +> it's entirely built-in to the library here. A middle ground where we have the hooks to wrap +> things and produce the telemetry ourselves is also fine (just not the *utopian ideal*). + +So, named: + +- **Utopian** — we emit `Activity` and `Meter` ourselves. Consumers write + `AddSource("StackExchange.Redis")`. The contrib package collapses to a one-line convenience + extension or is retired. #1044 stops existing, because there is nothing to register. +- **Middle ground** — we expose a supported, documented extensibility surface (profiling done + properly: statement text, failure detail, batch size, retransmission, connection events) and + contrib keeps producing the spans, against a contract instead of against `DynamicMethod`. This is + Lettuce's model, and it keeps semconv churn on their side of the line. + +They are not exclusive, and the second is a subset of the work for the first: the data that has to +become reachable is the same data either way. What differs is who calls `StartActivity`. + +## 7. Prior art for the packaging question + +**Npgsql** is the closest analogue and the one to copy. Native `ActivitySource` inside the driver; +a separate, trivial `Npgsql.OpenTelemetry` package whose entire job is an `AddNpgsql()` extension +that calls `AddSource("Npgsql")`. The driver itself has no OpenTelemetry dependency. Their docs +still label the tracing support experimental, tracking the semconv churn above. + +That is the template: **the telemetry is in-box; the OpenTelemetry-shaped convenience wrapper is a +separate, near-empty package** — and in our case that package already exists and already has a +maintainer, so the wrapper need not be ours at all. diff --git a/notes/otel/plan.md b/notes/otel/plan.md new file mode 100644 index 000000000..f3f350731 --- /dev/null +++ b/notes/otel/plan.md @@ -0,0 +1,223 @@ +# OpenTelemetry: the plan + +Decisions and sequencing for in-box telemetry. The evidence behind these is in +[`findings.md`](findings.md); this file is what we intend to do about it. + +Status: **proposed**. Nothing below has shipped. + +## The shape + +Emit `System.Diagnostics.ActivitySource` spans and `System.Diagnostics.Metrics.Meter` instruments +directly from StackExchange.Redis. No OpenTelemetry dependency, no vendor dependency — these types +are the .NET tracing and metrics API, and every collector (OpenTelemetry, Datadog, Application +Insights, Elastic, New Relic) already listens to them. + +Consumer side, in full: + +```csharp +builder.Services.AddOpenTelemetry() + .WithTracing(t => t.AddSource("StackExchange.Redis")) + .WithMetrics(m => m.AddMeter("StackExchange.Redis")); +``` + +No registration call, no connection handed to a builder, no `RegisterProfiler`. That is the point: +**[#1044](https://github.com/StackExchange/StackExchange.Redis/issues/1044) — "allow a global +profiler" — stops existing**, because there is nothing left to register. Same for the seven +`AddRedisInstrumentation` overloads in the contrib package and its DI deferral dance. + +### What is *not* public + +No `public static class RedisTelemetry { public const string ActivitySourceName = ... }`. The name +is a stability contract whether or not a `const` points at it, and a constant is just a second +spelling of a documented string plus a public API entry to maintain forever. Neither ASP.NET Core, +HttpClient, EF Core nor Npgsql ships one; their public surface is either a documented string or a +convenience extension in a *separate* package (`Npgsql.OpenTelemetry`'s `AddNpgsql()`). + +Ours is a documented string in `docs/`, and the convenience extension — if anyone wants one — can +stay in the contrib package, which already owns the name `AddRedisInstrumentation` and already has +a maintainer. + +### Names + +Both are `StackExchange.Redis`, versioned with the package version. One source, one meter, until +we have a concrete case for splitting (pub/sub is the likely first candidate — see phase 5). + +### Configuration + +`ConfigurationOptions`, using the existing `OptionFlags` + `DefaultOptionsProvider` fallback +pattern (`IncludeDetailInExceptions` is the model). Per-multiplexer, not static — the whole +complaint in #1044 is about a hook that is awkward *because* it is per-connection-and-imperative, +not because it is per-connection. + +Initial knob, defaulting off: + +- `EmitQueryText` — whether to populate `db.query.text` / `db.statement`. Off by default because + keys can carry PII. Command **and key**; never argument values. Matches contrib's + `SetVerboseDatabaseStatements` and Lettuce's `includeCommandArgsInSpanTags`. + +## Target frameworks + +`System.Diagnostics.DiagnosticSource` reaches all of our targets except `net461` — the package +dropped its `net461` asset after 6.0.1 and now bottoms out at `net462` (see findings §2). So: + +| TFM | instrumentation | `PackageReference` | +| --- | --- | --- | +| `net10.0` | yes | none — in-box in `Microsoft.NETCore.App` | +| `net8.0` | yes | none — in-box | +| `net472` | yes | `System.Diagnostics.DiagnosticSource` (`lib/net462`) | +| `netstandard2.0` | yes | `System.Diagnostics.DiagnosticSource` (`lib/netstandard2.0`) | +| `net461` | **compiled out** | none | + +Guarded by a `TELEMETRY` compile symbol defined for everything but `net461`, following the existing +`VECTOR_SAFE` / `UNIX_SOCKET` idiom in `StackExchange.Redis.csproj`. Version pinned in +`Directory.Packages.props` like every other reference. + +net461 users lose nothing they have today; the existing profiling API stays where it is on every +target. + +## Pay-for-play + +The cost when nobody is listening must be a predictable, tiny constant: + +- one static `ActivitySource.HasListeners()` check per command on the caller thread; +- one reference field on `Message`, null unless telemetry is active; +- nothing allocated, no `AsyncLocal` written, no `Activity` created — `StartActivity` is never + reached. + +When a listener *is* attached, anything expensive (statement text above all) goes behind +`Activity.IsAllDataRequested`, and the statement itself additionally behind `EmitQueryText`. + +Benchmarks are part of the work, not a follow-up: `tests/StackExchange.Redis.Benchmarks` needs a +listener-off case, a metrics-only case, and a fully-sampled case, because "metrics always on, +traces sampled" is the configuration that actually matters at scale. + +## Where the code goes + +All five sites already exist and are already the right ones: + +| site | role | +| --- | --- | +| `ConnectionMultiplexer.cs:~2397` | caller thread, post server-selection — capture parent context | +| `Message.CreatedTimestamp` | start time (already recorded) | +| `Message.Complete(PhysicalConnection?)` | reader thread — create, tag, stop; `resultBox.Fault` is in hand | +| `Message.SetExceptionAndComplete` | failure path | +| `Message.PrepareToResend` | `-MOVED` / `-ASK` retransmission | + +**Deferred activity creation** (findings §5): capture `Activity.Current?.Context` on the caller +thread — a struct copy, no `AsyncLocal` write — and call `StartActivity(..., parentContext, +startTime)` → `SetEndTime` → `Stop()` on the completion path. Starting the activity on the caller +thread would assign `Activity.Current` with no scope to dispose, leaking our span into the +caller's async flow. + +## Sequencing + +Phases are ordered by risk and by how much of the design they commit us to. Each is independently +shippable. + +### Phase 1 — unblock the reflection, now + +Expose the statement string so the contrib package can stop emitting `DynamicMethod` field getters +against our privates, on 3.x, today. This is worth doing *whatever* we decide about the rest: +contrib has to support 3.x for years regardless. + +`IProfiledCommand` is a public interface, so adding a member to it is a break for implementers +(realistically only test doubles — `ProfiledCommand` is internal sealed — but a break). Preferred +form is therefore an extension method, which is additive and honest about the fact that only our +own implementation can answer: + +```csharp +namespace StackExchange.Redis.Profiling; + +public static class ProfilingExtensions +{ + // null for any IProfiledCommand we did not create + public static string? GetStatement(this IProfiledCommand command); +} +``` + +Returns command + key + script text — the exact payload the reflection reconstructs. Needs a +decision on whether it is gated by `EmitQueryText` or always available to a caller who asks (it is +already opt-in by virtue of being an explicit call). + +*Open:* interface member vs. extension method. Raising rather than assuming, per AGENTS.md. + +### Phase 2 — metrics + +Lowest risk of the real work: no `Activity.Current` semantics, no public API surface beyond a +documented meter name, and bounded memory regardless of throughput. This is the half that can run +permanently at Stack Overflow scale, and connection-level telemetry is the part +[Nick already agreed to in 2022](https://github.com/StackExchange/StackExchange.Redis/issues/1044#issuecomment-1069746402): + +> For things like connections, disconnects, reconnects, errors: yeah sure, that's a lot lower +> volume and reasonable. + +- `db.client.operation.duration` — histogram, seconds, semconv **stable**, buckets + `[0.001, 0.005, 0.01, 0.05, 0.1, 0.5, 1, 5, 10]`. Tags: `db.system.name`, `db.operation.name`, + `db.namespace`, `server.address`, `server.port`, `error.type` on failure. +- connection instruments from the events and counters we already have — + `ConnectionFailed`, `ConnectionRestored`, `ErrorMessage`, and observable gauges over + `GetCounters()` for backlog depth, queue lengths and timeouts. The semconv + `db.client.connection.*` family is still *development* status, so we follow it where it fits and + use our own names where it does not, rather than bending our model to a moving target. + +### Phase 3 — traces + +`ActivitySource` with the deferred-creation pattern. Span kind `CLIENT`, span name = bare command +name (`db.namespace` deliberately excluded from the name per semconv; bare command name is also +what go-redis and Lettuce do). Attribute set per findings §3, emitting **current** semconv names +only — `db.system.name`, `db.operation.name`, `db.query.text` — not the 1.23-era +`db.system`/`db.statement`. Contrib can keep its dual-emit shim for people who need the old names; +we should not carry that migration into a library that has not shipped any of it yet. + +New material that the profiling API cannot express today and that comes free here: +`db.response.status_code` (Redis error prefix — `WRONGTYPE`, `MOVED`, `NOSCRIPT`) and `error.type`. +**Failures are currently invisible to telemetry**, which is the single largest gap. + +### Phase 4 — the things contrib gave up on + +- **Retransmission.** `-MOVED` / `-ASK` resends, which the contrib converter carries a literal + `// TODO: deal with the re-transmission` for. `PrepareToResend` is ours; a child span or an + `ActivityLink` back to the original is straightforward from the inside. +- **Batches and transactions.** One span with `db.operation.batch.size`, name prefixed `MULTI` or + `PIPELINE` per semconv. Both go-redis (`redis.pipeline`, `db.redis.num_cmd`) and redis-py + (`db.redis.pipeline_length`) collapse a pipeline to one span; we should too. +- **Connection establishment.** go-redis traces `redis.dial`; we have far richer material — + handshake, discovery, failover, sentinel — and it is low-volume enough to trace unconditionally. + +### Phase 5 — pub/sub + +Producer/consumer spans with `ActivityLink`, not client spans; message-bus semconv, not database +semconv. Almost certainly wants its own `ActivitySource` so it can be enabled independently. Out of +scope until the above lands. + +## Open questions + +Worth putting to @martincostello directly, since he offered to collaborate: + +1. **Who owns semconv churn?** Contrib currently ships three `ActivitySource`s to straddle 1.23 vs + 1.42. If we emit natively, that migration becomes ours. Is the dual-emit window genuinely + closing, or are we signing up for it permanently? This is the strongest argument for the middle + ground and we should hear it argued before dismissing it. +2. **Does contrib want to become a one-liner, or keep producing spans?** If we go native, does the + package retire, or keep an `AddRedisInstrumentation()` that calls `AddSource` plus the old + `Filter`/`Enrich` knobs? Those two callbacks are the only features that do not obviously survive + the transition. +3. **`Filter` and `Enrich` equivalents.** Do we need them in-box, or is per-command filtering + better done by the listener? Nick's 2022 objection was specifically that *deciding not to + profile* costs something per command; with `ActivityListener.Sample` that decision moves to the + collector, which is where it belongs. +4. **Version/schema pinning.** Should the `ActivitySource` version track the package version, or a + semconv schema version (contrib uses the latter via `ActivitySourceFactory.Create(version)`)? +5. **Does this need to wait for v4?** The IO core rewrite moves all five hook sites. Hooks placed + now survive as *concepts* but not as code. Landing metrics on 3.x and traces on 4.x is a + defensible split; so is landing both on 3.x and accepting the port. + +## Things we are deliberately not doing + +- **Not depending on any OpenTelemetry package.** Library authors depend on + `System.Diagnostics.DiagnosticSource` only; this is both Microsoft's and OpenTelemetry's own + guidance. +- **Not emitting 1.23-era attribute names.** New code, current conventions. +- **Not putting command argument values in spans.** Command and key only, and that opt-in. +- **Not removing or changing the profiling API.** It stays, unchanged, on every target including + `net461`. Native telemetry is additive. diff --git a/notes/readme.md b/notes/readme.md new file mode 100644 index 000000000..7c2a57f49 --- /dev/null +++ b/notes/readme.md @@ -0,0 +1,23 @@ +# Working notes + +This directory holds working material for whoever picks a topic up next: investigations, the +reasoning behind a decision, and snapshots that were expensive to produce and would be expensive +to produce again. + +The split to keep: + +- `docs/` — documentation for people *using* StackExchange.Redis. It is a Jekyll site + (`docs/_config.yml`, published at ), so anything placed there is built into + the site *and* listed in `sitemap.xml`. +- `notes/` — internal handover material. Not built, not published, not indexed. + +Notes are grouped by topic in a subdirectory, and the files inside are named for their content +rather than repeating the topic — `notes/otel/findings.md`, not `notes/otel/otel-findings.md`. + +When something in here graduates into advice for users, write it up in `docs/` and leave the note +as the record of *why*. + +## Topics + +- [`otel/`](otel/) — OpenTelemetry: what the ecosystem does today, and the plan for in-box + traces and metrics. From 6fffbd047a77609fa621ab1be56f89935d44cd9e Mon Sep 17 00:00:00 2001 From: mgravell Date: Thu, 17 Sep 2026 14:11:31 +0100 Subject: [PATCH 2/8] Plan to replace the contrib package, and pin down what that obliges us to mgravell is taking the takeover question to the OTel folks; this records what it would mean for us. The package has never shipped a stable version in 48 releases - their README says that is because the database conventions are still experimental, which is exactly the burden we would be inheriting. The load-bearing correction: the old/new attribute split is driven by OTEL_SEMCONV_STABILITY_OPT_IN, and its default is Old. Unopted users are seeing db.system and db.statement today, so the previous draft's "new names only" would have broken every existing consumer silently. We honour the env var instead, which is also just the correct behaviour for a .NET database library. Second consequence: the timing events are on by default and consume all five profiling timestamps, so a start-and-complete-only span would quietly lose data people already have. findings.md gains the full inventory to match against; plan.md gains the compatibility contract, the two things that cannot be preserved (source name, Filter/Enrich), and the Apache-2.0-into-MIT question - which lands hardest on their test suite, the part actually worth having. --- notes/otel/findings.md | 74 ++++++++++++++++++++++++++ notes/otel/plan.md | 115 ++++++++++++++++++++++++++++++++++++----- 2 files changed, 176 insertions(+), 13 deletions(-) diff --git a/notes/otel/findings.md b/notes/otel/findings.md index a876dc5f4..7898a8269 100644 --- a/notes/otel/findings.md +++ b/notes/otel/findings.md @@ -266,3 +266,77 @@ still label the tracing support experimental, tracking the semconv churn above. That is the template: **the telemetry is in-box; the OpenTelemetry-shaped convenience wrapper is a separate, near-empty package** — and in our case that package already exists and already has a maintainer, so the wrapper need not be ours at all. + +## 8. What contrib emits today — the compatibility baseline + +If we intend to replace the package (see `plan.md`), this is the inventory we have to match. Read +off contrib `main` as of 2026-09-17, package version 1.18.0-beta.3. + +**Release history:** 48 versions since 0.3.0-beta.1, and **not one stable release**. Their README +attributes this to the database semantic conventions still being Experimental, not to the code +being immature. + +**Activity identity** + +| | value | +| --- | --- | +| `ActivitySource.Name` | `OpenTelemetry.Instrumentation.StackExchangeRedis` (the assembly name, via `ActivitySourceFactory`) | +| `ActivitySource.Version` | the package version | +| `TelemetrySchemaUrl` | derived from the semconv version — 1.23.0 for the old source, 1.42.0 for the new, unset when emitting both | +| span name | `command.Command`, i.e. the bare command string; falls back to `"{ActivitySource.Name}.Execute"` if empty | +| span kind | `Client` | +| start / end | `command.CommandCreated` and `+ command.ElapsedTime` — explicit, because the span is built after the fact | + +**The old/new attribute switch.** `EmitOldAttributes` / `EmitNewAttributes` are *internal*, not +user-facing knobs; they are set in the options constructor from the standard +`OTEL_SEMCONV_STABILITY_OPT_IN` environment variable via OTel's shared +`DatabaseSemanticConventionHelper`: + +- `database/dup` → both +- `database` → new only +- **anything else, including unset → `Old`** + +That default is the important part. **Unless a user has opted in, what they are seeing in +production today is the 1.23-era names.** + +| mode | attributes emitted | +| --- | --- | +| Old (default) | `db.system` = `redis`; `db.statement`; `db.redis.database_index` | +| New | `db.system.name` = `redis`; `db.operation.name`; `db.namespace`; `db.query.text` | +| Dupe | both sets | + +Note `db.redis.database_index` — contrib's own invention, not in any semantic convention, and +present only in old mode. + +**Always emitted, in every mode**, from `command.EndPoint`: + +- `IPEndPoint` → `server.address`, `server.port`, `network.peer.address`, `network.peer.port` +- `DnsEndPoint` → `server.address`, `server.port` +- `UnixDomainSocketEndPoint` (net only) → `server.address`, `network.peer.address` + +**Activity events** — `EnrichActivityWithTimingEvents`, default **`true`**: `Enqueued`, `Sent`, +`ResponseReceived`, timestamped by accumulating `CreationToEnqueued`, `EnqueuedToSending`, +`SentToResponse` onto `CommandCreated`. + +This matters for implementation, not just for parity: **it consumes all five profiling timestamps**, +so any replacement that captures only start-and-complete silently drops data that people have +today. + +**`db.statement` / `db.query.text` content** — the command string by default; with +`SetVerboseDatabaseStatements` (default `false`), `CommandAndKey` plus, for EVAL/EVALSHA, a space +and the script text. This is the reflection payload from §1. + +**Not emitted, at all:** + +- any error or failure information — no `error.type`, no `db.response.status_code`, and + `ActivityStatusCode` is never set. A Redis command that fails produces a span indistinguishable + from one that succeeded. +- `db.operation.batch.size` +- anything about retransmission (`// TODO: deal with the re-transmission`) +- metrics of any kind — the package is traces only + +**User-facing options:** `FlushInterval` (default 10s), `SetVerboseDatabaseStatements` (false), +`EnrichActivityWithTimingEvents` (true), `Filter`, `Enrich`. + +**Licensing:** contrib is Apache-2.0 (SPDX headers on every file); this repo is MIT. Code owner is +@matt-hensley. diff --git a/notes/otel/plan.md b/notes/otel/plan.md index f3f350731..507dac1c0 100644 --- a/notes/otel/plan.md +++ b/notes/otel/plan.md @@ -55,6 +55,87 @@ Initial knob, defaulting off: keys can carry PII. Command **and key**; never argument values. Matches contrib's `SetVerboseDatabaseStatements` and Lettuce's `includeCommandArgsInSpanTags`. +## Subsuming the contrib package + +The intent is to **replace** `OpenTelemetry.Instrumentation.StackExchangeRedis`, not to sit +alongside it. That needs agreement from its owners — code owner is @matt-hensley, with +@martincostello active on it — and mgravell is raising it with them. + +It should not be especially controversial. In 48 releases since 2020 the package has **never +shipped a stable version**; every one is `-beta` or `-rc`. Their README says why, and the reason +matters to us: + +> This component is based on the OpenTelemetry semantic conventions for traces. These conventions +> are Experimental, and hence, this package is a pre-release. Until a stable version is released, +> there can be breaking changes. + +So the reason it is not GA is semconv instability, not immaturity — which is precisely the thing we +inherit by taking it over. See open question 1. + +### The compatibility contract + +**Default to matching their output byte-for-byte; deviate only deliberately, and write the +deviation down here.** People have dashboards, alerts and saved queries built on these spans, and +"we rewrote it and your error rate graph went flat" is not an acceptable upgrade story. + +Concretely, from findings §8: + +- **Honour `OTEL_SEMCONV_STABILITY_OPT_IN`**, with the same three modes and the same default. This + is the single most important item: the default today is `Old`, so unopted users are seeing + `db.system` and `db.statement`, *not* `db.system.name` and `db.query.text`. An earlier draft of + this plan said "new names only" — that was wrong, and would have silently broken every existing + consumer. Honouring the env var is also simply the correct behaviour for any .NET library + emitting database telemetry, and it gives a clean exit: when the conventions go stable, our + default flips in lockstep with the rest of the ecosystem rather than on our own schedule. +- **Keep `db.redis.database_index`** in old mode. It is contrib-specific, not semconv, and it is in + people's dashboards. +- **Keep the timing events** — `Enqueued`, `Sent`, `ResponseReceived` — and keep them on by + default, as `EnrichActivityWithTimingEvents` is. This has a design consequence: a + compatibility-preserving implementation must capture **all five** profiling timestamps when data + is requested, not just create-and-complete. Cheaper span construction is not worth losing data + people already have. +- **Keep the span name rule**: the bare command string. +- **Set `TelemetrySchemaUrl`** on the `ActivitySource` from the semconv version, as they do. + +### Where we intend to be better + +Additive is always fine. Changing the value or meaning of something that already exists is not. + +- Failure attribution — `error.type`, `db.response.status_code`, and an actual + `ActivityStatusCode.Error`. Contrib sets **none** of these; failures are currently invisible. + This is a deliberate deviation: error rates that were flat will stop being flat. Call it out in + release notes. +- `-MOVED` / `-ASK` retransmission, which they have a `// TODO` for. +- Batch and transaction shape (`db.operation.batch.size`). +- Connection lifecycle: establishment, failover, sentinel, discovery. +- Metrics at all — they ship traces only. +- Statement text without reflection, and correct for every message type rather than the handful + the reflection knows about. + +### Two things that cannot be preserved + +1. **The `ActivitySource` name changes.** Theirs is the assembly name, + `OpenTelemetry.Instrumentation.StackExchangeRedis`; ours will be `StackExchange.Redis`. We are + not squatting their name. Anyone calling `AddRedisInstrumentation()` sees nothing — the + extension just changes which source it adds — but anyone who hand-wrote + `AddSource("OpenTelemetry.Instrumentation.StackExchangeRedis")` has to edit one string. Document + it prominently; consider asking contrib to add *both* names for a transition window. +2. **`Filter` and `Enrich`.** Callbacks on their options object with no obvious in-box equivalent. + `ActivityListener.Sample` is the better home for filtering — see open question 3. + +### On starting from their implementation + +Their code is Apache-2.0; this repo is MIT. Apache-2.0 is permissive and can be incorporated, but +§4 has real obligations (retain notices, state changes, carry the license text for the derived +portions) — so this is a decision to make on purpose, with their blessing, not a quiet copy-paste. + +The practical recommendation is to **use it as a specification rather than a source**: most of the +implementation is machinery for the drain thread, the session cache and the baggage restoration, +all of which we are deleting, and the rest is written against internals we will not have. What is +genuinely worth having is (a) the exact attribute/event/default inventory, which is behaviour and +is captured in findings §8, and (b) their **test suite**, which is the compatibility oracle and is +the part where the licensing question actually bites. Ask about the tests explicitly. + ## Target frameworks `System.Diagnostics.DiagnosticSource` reaches all of our targets except `net461` — the package @@ -164,10 +245,9 @@ permanently at Stack Overflow scale, and connection-level telemetry is the part `ActivitySource` with the deferred-creation pattern. Span kind `CLIENT`, span name = bare command name (`db.namespace` deliberately excluded from the name per semconv; bare command name is also -what go-redis and Lettuce do). Attribute set per findings §3, emitting **current** semconv names -only — `db.system.name`, `db.operation.name`, `db.query.text` — not the 1.23-era -`db.system`/`db.statement`. Contrib can keep its dual-emit shim for people who need the old names; -we should not carry that migration into a library that has not shipped any of it yet. +what go-redis and Lettuce do). Attribute set per findings §3, honouring +`OTEL_SEMCONV_STABILITY_OPT_IN` exactly as contrib does — see "Subsuming the contrib package" +below, which is what fixes the attribute names, the defaults, and the timing events. New material that the profiling API cannot express today and that comes free here: `db.response.status_code` (Redis error prefix — `WRONGTYPE`, `MOVED`, `NOSCRIPT`) and `error.type`. @@ -194,21 +274,28 @@ scope until the above lands. Worth putting to @martincostello directly, since he offered to collaborate: -1. **Who owns semconv churn?** Contrib currently ships three `ActivitySource`s to straddle 1.23 vs - 1.42. If we emit natively, that migration becomes ours. Is the dual-emit window genuinely - closing, or are we signing up for it permanently? This is the strongest argument for the middle - ground and we should hear it argued before dismissing it. +1. **Who owns semconv churn?** Contrib ships three `ActivitySource`s to straddle 1.23 vs 1.42, and + has never shipped a stable version in six years *because* the conventions are experimental. If + we emit natively, that becomes our problem — and StackExchange.Redis does not have the option of + shipping perpetual betas. Do we mark the telemetry `[Experimental]` (we already have the + `SER00x` machinery in `src/RESPite/Shared/Experiments.cs`), or do we accept that our stable + package emits attributes that may be renamed under us? This is the strongest argument for the + middle ground and we should hear it argued before dismissing it. 2. **Does contrib want to become a one-liner, or keep producing spans?** If we go native, does the package retire, or keep an `AddRedisInstrumentation()` that calls `AddSource` plus the old `Filter`/`Enrich` knobs? Those two callbacks are the only features that do not obviously survive - the transition. -3. **`Filter` and `Enrich` equivalents.** Do we need them in-box, or is per-command filtering + the transition. Either way, ask them to add our source name — and ideally to keep adding their + own for a window, so existing hand-written `AddSource` calls do not break. +3. **Can we have the tests?** Their test suite is the compatibility oracle, and it is Apache-2.0 + going into an MIT repo. Explicit blessing, an agreed attribution form, or a clean-room rewrite + from the behaviour inventory — but decided up front, not discovered in review. +4. **`Filter` and `Enrich` equivalents.** Do we need them in-box, or is per-command filtering better done by the listener? Nick's 2022 objection was specifically that *deciding not to profile* costs something per command; with `ActivityListener.Sample` that decision moves to the collector, which is where it belongs. -4. **Version/schema pinning.** Should the `ActivitySource` version track the package version, or a +5. **Version/schema pinning.** Should the `ActivitySource` version track the package version, or a semconv schema version (contrib uses the latter via `ActivitySourceFactory.Create(version)`)? -5. **Does this need to wait for v4?** The IO core rewrite moves all five hook sites. Hooks placed +6. **Does this need to wait for v4?** The IO core rewrite moves all five hook sites. Hooks placed now survive as *concepts* but not as code. Landing metrics on 3.x and traces on 4.x is a defensible split; so is landing both on 3.x and accepting the port. @@ -217,7 +304,9 @@ Worth putting to @martincostello directly, since he offered to collaborate: - **Not depending on any OpenTelemetry package.** Library authors depend on `System.Diagnostics.DiagnosticSource` only; this is both Microsoft's and OpenTelemetry's own guidance. -- **Not emitting 1.23-era attribute names.** New code, current conventions. +- **Not picking our own attribute names or our own default.** We follow + `OTEL_SEMCONV_STABILITY_OPT_IN` like every other .NET database instrumentation, including its + current `Old` default, however much we might prefer the new names. - **Not putting command argument values in spans.** Command and key only, and that opt-in. - **Not removing or changing the profiling API.** It stays, unchanged, on every target including `net461`. Native telemetry is additive. From 7d0d1c0252f16555b283a906e7d98416548b2971 Mon Sep 17 00:00:00 2001 From: mgravell Date: Thu, 17 Sep 2026 14:15:24 +0100 Subject: [PATCH 3/8] Sketch the public API impact, and answer the one-assembly question It was not in the plan; it is now, phase by phase, in PublicAPI.Unshipped.txt terms. Eight lines total - one static extension class for the statement text, and two ConfigurationOptions/DefaultOptionsProvider pairs. Nothing on IConnectionMultiplexer: IncludeDetailInExceptions is the cautionary tale, six API lines for one bool and an interface member that can never be added to. Read Npgsql's own PublicAPI.Shipped.txt rather than guessing: 14 lines, all knobs, ActivitySource internal, no name constants, all instrumentation in the core assembly. Their separate package is four lines and exists only because the OTel-typed extension methods need OTel types. We do not even need that much - contrib already owns AddRedisInstrumentation. So: one assembly. The hook sites have to be in core either way (the data is produced on the reader thread), ActivitySource already is the indirection a sink assembly would re-implement, and cross-package IVT reintroduces the reflection's failure mode - version skew, discovered at runtime - behind a compiler check. The narrow case for splitting is a netstandard2.0/net472 build with no added reference, and that is all it buys. Also recorded against myself: Npgsql shipped Filter/Enrich/span-name providers in-box, which is direct counter-evidence to my "ActivityListener.Sample is enough" position. Two implementations converging beats an instinct. --- notes/otel/findings.md | 55 ++++++++++++++++++++ notes/otel/plan.md | 113 +++++++++++++++++++++++++++++++++++++++-- 2 files changed, 164 insertions(+), 4 deletions(-) diff --git a/notes/otel/findings.md b/notes/otel/findings.md index 7898a8269..f1113a95b 100644 --- a/notes/otel/findings.md +++ b/notes/otel/findings.md @@ -340,3 +340,58 @@ and the script text. This is the reflection payload from §1. **Licensing:** contrib is Apache-2.0 (SPDX headers on every file); this repo is MIT. Code owner is @matt-hensley. + +## 9. Npgsql's public surface, in full + +Since Npgsql is the model (§7), it is worth being exact about what "native telemetry" cost them in +public API. Read off `npgsql/main`; they use the same `PublicApiAnalyzers` we do, so this is their +own `PublicAPI.Shipped.txt`, not a reading of the source. + +**In the core `Npgsql` package** — 14 API lines, all of them knobs: + +```text +Npgsql.NpgsqlTracingOptionsBuilder +Npgsql.NpgsqlTracingOptionsBuilder.ConfigureCommandFilter(System.Func? commandFilter) -> Npgsql.NpgsqlTracingOptionsBuilder! +Npgsql.NpgsqlTracingOptionsBuilder.ConfigureCommandEnrichmentCallback(System.Action? commandEnrichmentCallback) -> Npgsql.NpgsqlTracingOptionsBuilder! +Npgsql.NpgsqlTracingOptionsBuilder.ConfigureCommandSpanNameProvider(System.Func? commandSpanNameProvider) -> Npgsql.NpgsqlTracingOptionsBuilder! + ... the same three again for Batch, and again for CopyOperation ... +Npgsql.NpgsqlTracingOptionsBuilder.EnableFirstResponseEvent(bool enable = true) -> Npgsql.NpgsqlTracingOptionsBuilder! +Npgsql.NpgsqlTracingOptionsBuilder.EnablePhysicalOpenTracing(bool enable = true) -> Npgsql.NpgsqlTracingOptionsBuilder! +Npgsql.NpgsqlDataSourceBuilder.ConfigureTracing(System.Action! configureAction) -> Npgsql.NpgsqlDataSourceBuilder! +Npgsql.NpgsqlSlimDataSourceBuilder.ConfigureTracing(...) -> Npgsql.NpgsqlSlimDataSourceBuilder! +Npgsql.NpgsqlMetricsOptions +Npgsql.NpgsqlMetricsOptions.NpgsqlMetricsOptions() -> void +``` + +What is *not* there: `NpgsqlActivitySource` is **internal**. No public source-name constant, no +public meter-name constant, no public telemetry types beyond the options. `NpgsqlMetricsOptions` is +a bare class with a default constructor — a placeholder so the metrics extension has something to +take. + +**All the instrumentation lives in the core assembly.** There is no IVT-ed sink library. + +**The `Npgsql.OpenTelemetry` package is four lines of code**, in two files: + +```csharp +public static TracerProviderBuilder AddNpgsql(this TracerProviderBuilder builder) + => builder.AddSource("Npgsql"); + +public static MeterProviderBuilder AddNpgsqlInstrumentation( + this MeterProviderBuilder builder, Action? options = null) + => builder.AddMeter("Npgsql"); +``` + +That is the entire package. Note the `options` parameter on the metrics one is accepted and +ignored. The package exists for exactly one reason: those two extension methods need types from +`OpenTelemetry`, and the core driver must not depend on `OpenTelemetry`. + +Two further observations worth carrying into our own design: + +- **They shipped `Filter` and `Enrich` in-box after all** — filters, enrichment callbacks *and* + span-name providers, for each of commands, batches and copy operations. That is direct + counter-evidence to the position that `ActivityListener.Sample` makes in-box filtering + unnecessary. Someone who has run this in production for years concluded otherwise. +- **`EnablePhysicalOpenTracing` and `EnableFirstResponseEvent` are opt-in, defaulting off** — + connection-establishment spans and extra timing events are not free enough to be on by default. + Contrib, by contrast, has its timing events **on** by default, which is a compatibility + constraint for us (§8) and not an endorsement. diff --git a/notes/otel/plan.md b/notes/otel/plan.md index 507dac1c0..b66f5a6a4 100644 --- a/notes/otel/plan.md +++ b/notes/otel/plan.md @@ -136,6 +136,107 @@ genuinely worth having is (a) the exact attribute/event/default inventory, which is captured in findings §8, and (b) their **test suite**, which is the compatibility oracle and is the part where the licensing question actually bites. Ask about the tests explicitly. +## Public API impact + +Modelled on Npgsql, whose full telemetry surface is inventoried in findings §9: 14 API lines in the +core package, all knobs, with the `ActivitySource` itself kept internal and no public name +constants. Ours is smaller, because `ConfigurationOptions` + `DefaultOptionsProvider` is a cheaper +place to hang a setting than a data-source builder. + +In `PublicAPI.Unshipped.txt` terms, phase by phase: + +**Phase 1 — statement text** (2 lines) + +```text +StackExchange.Redis.Profiling.ProfilingExtensions +static StackExchange.Redis.Profiling.ProfilingExtensions.GetStatement(this StackExchange.Redis.Profiling.IProfiledCommand! command) -> string? +``` + +**Phase 2 — metrics** (0 lines) + +Nothing. The meter name is a documented string; there is no knob worth having on day one. If we +ever need one, Npgsql's placeholder `NpgsqlMetricsOptions` is the shape — and the fact that theirs +is still an empty class with a default constructor suggests waiting. + +**Phase 3 — traces** (3 lines) + +```text +StackExchange.Redis.ConfigurationOptions.EmitQueryText.get -> bool +StackExchange.Redis.ConfigurationOptions.EmitQueryText.set -> void +virtual StackExchange.Redis.Configuration.DefaultOptionsProvider.EmitQueryText.get -> bool +``` + +The connection-string keyword is free: `OptionKeys` is a `private static class` inside +`ConfigurationOptions`, so parsing support adds no public API. + +**Deliberately not mirrored onto `IConnectionMultiplexer`.** `IncludeDetailInExceptions` is the +cautionary example — it costs six API lines because it appears on `ConfigurationOptions`, +`ConnectionMultiplexer` *and* `IConnectionMultiplexer`, and that last one means the property can +never be added to without breaking implementers. Telemetry settings are read from `RawConfig` on +the command path; nothing needs them on the multiplexer interface. + +**Phase 4 — connection tracing** (3 lines, same shape) + +```text +StackExchange.Redis.ConfigurationOptions.EmitConnectionTracing.get -> bool +StackExchange.Redis.ConfigurationOptions.EmitConnectionTracing.set -> void +virtual StackExchange.Redis.Configuration.DefaultOptionsProvider.EmitConnectionTracing.get -> bool +``` + +Opt-in and defaulting off, following Npgsql's `EnablePhysicalOpenTracing`. + +**Running total: eight lines**, no new public types beyond one static extension class, no new +interfaces, no change to any existing interface. That is the whole cost of the in-box option and it +is worth weighing against the alternative below. + +**Not in the total, because it is a separate decision:** `Filter` / `Enrich` / span-name-provider +equivalents. Npgsql shipped all three, for commands, batches *and* copy operations — nine methods +plus a builder type, i.e. the bulk of their surface — which is real counter-evidence to the +"`ActivityListener.Sample` is enough" position (open question 4). If we follow them, the surface +roughly triples. + +There is a genuine design problem underneath it, which is why it is not sketched here: *what do we +hand the callback?* Npgsql passes the live `NpgsqlCommand`. Our equivalent would be +`IProfiledCommand` — but the whole point of the deferred-creation design is that we do **not** +build a `ProfiledCommand` unless someone registered a profiler. Handing one to a filter means +allocating the thing we were avoiding. Likely answers are a `readonly ref struct` view over the +`Message`, or accepting the allocation only when a callback is configured. Needs deciding before +any of this is sketched as API. + +## One assembly, or hooks plus an IVT sink? + +Recommendation: **one assembly.** Not strongly held on the packaging, but the technical argument is +fairly one-sided. + +1. **The hook sites have to be in core either way.** The data is produced on the reader thread + inside `Message.Complete`, and the parent context is captured on the caller thread in + `ConnectionMultiplexer`. A second assembly cannot reach those. It can only be *called* — which + means an indirection in core (a per-command virtual dispatch, plus a registration mechanism that + is itself near-public API) — or it can *poll*, which is exactly the `ProfilingSession` design we + are trying to delete. +2. **`ActivitySource` and `Meter` already are the indirection.** That is the entire point of them + living in the BCL. A second assembly binding over internals would be re-implementing + `ActivityListener`, worse and privately. +3. **Cross-package IVT is lockstep with a runtime failure mode.** `StackExchange.Redis` 3.5 plus + `StackExchange.Redis.Telemetry` 3.2 is a perfectly resolvable NuGet graph that throws + `MissingMethodException` on first use. That is the same failure class as the reflection this + whole exercise is meant to retire — moved behind a compiler check, but not removed. +4. **The reference is free where it matters.** `System.Diagnostics.DiagnosticSource` is in-box on + `net8.0` and `net10.0`; only `net472` and `netstandard2.0` gain an actual package reference, and + `net461` gains nothing because it compiles out. +5. **Npgsql — the model — put everything in core.** Their separate package holds only the two + OpenTelemetry-typed extension methods, because *those* need `OpenTelemetry` types and the driver + must not. It contains no instrumentation: four lines total. + +The honest case for the split is narrow: it buys a `netstandard2.0`/`net472` build with no new +package reference, and independent versioning of the telemetry. If we decide that reference is +unacceptable, the split is what buys it — and nothing else. + +And on the OpenTelemetry-typed wrapper specifically: Npgsql needed a second package because nobody +else was going to write `AddNpgsql()` for them. **We do not have that problem** — contrib already +owns `AddRedisInstrumentation`, already has a maintainer, and would be reduced to precisely those +four lines. That is the conversation to have with them. + ## Target frameworks `System.Diagnostics.DiagnosticSource` reaches all of our targets except `net461` — the package @@ -289,10 +390,14 @@ Worth putting to @martincostello directly, since he offered to collaborate: 3. **Can we have the tests?** Their test suite is the compatibility oracle, and it is Apache-2.0 going into an MIT repo. Explicit blessing, an agreed attribution form, or a clean-room rewrite from the behaviour inventory — but decided up front, not discovered in review. -4. **`Filter` and `Enrich` equivalents.** Do we need them in-box, or is per-command filtering - better done by the listener? Nick's 2022 objection was specifically that *deciding not to - profile* costs something per command; with `ActivityListener.Sample` that decision moves to the - collector, which is where it belongs. +4. **`Filter` and `Enrich` equivalents.** My instinct was that `ActivityListener.Sample` makes + in-box filtering unnecessary — Nick's 2022 objection was precisely that *deciding not to + profile* costs something per command, and `Sample` moves that decision to the collector. + **Npgsql's experience says otherwise**: they ship filters, enrichment callbacks and span-name + providers for commands, batches and copy operations, and that is the bulk of their public + surface (findings §9). Contrib ships `Filter`/`Enrich` too. Two independent implementations + converging on the same thing is worth more than my instinct. If we adopt them we need to settle + what gets handed to the callback first — see "Public API impact". 5. **Version/schema pinning.** Should the `ActivitySource` version track the package version, or a semconv schema version (contrib uses the latter via `ActivitySourceFactory.Create(version)`)? 6. **Does this need to wait for v4?** The IO core rewrite moves all five hook sites. Hooks placed From a16adb599500648528c7f159814b3c75c775cc4b Mon Sep 17 00:00:00 2001 From: mgravell Date: Thu, 17 Sep 2026 14:19:00 +0100 Subject: [PATCH 4/8] Say what "contrib" means before using it 33 times It was never defined in plan.md at all - the document most likely to be read standalone - and findings.md only linked the repo in passing. Worse, it was doing double duty: "contrib already owns AddRedisInstrumentation" means the package, but "ask contrib to add both names" means the people. Both files now define the shorthand at first use and name the maintainers where people are meant. Same definition added to the PR body. --- notes/otel/findings.md | 12 +++++++++--- notes/otel/plan.md | 23 +++++++++++++++-------- 2 files changed, 24 insertions(+), 11 deletions(-) diff --git a/notes/otel/findings.md b/notes/otel/findings.md index f1113a95b..4ab9f34d0 100644 --- a/notes/otel/findings.md +++ b/notes/otel/findings.md @@ -9,7 +9,13 @@ The conversation this came out of is [#1044](https://github.com/StackExchange/St ## 1. What the OpenTelemetry bridge does today, and what it costs `OpenTelemetry.Instrumentation.StackExchangeRedis` is a community package in -[opentelemetry-dotnet-contrib](https://github.com/open-telemetry/opentelemetry-dotnet-contrib/tree/main/src/OpenTelemetry.Instrumentation.StackExchangeRedis). +[opentelemetry-dotnet-contrib](https://github.com/open-telemetry/opentelemetry-dotnet-contrib/tree/main/src/OpenTelemetry.Instrumentation.StackExchangeRedis), +the OpenTelemetry .NET SIG's repository for instrumentation of libraries outside the core +distribution. **These notes call that package "the contrib package" throughout** — the OpenTelemetry +project uses "contrib" for the repository, and it is shorter than writing the package name out +thirty times. Where the *people* are meant rather than the package, they are named as its +maintainers. + It has exactly one hook into us: `IConnectionMultiplexer.RegisterProfiler`. Everything else it does is machinery to reconstruct, after the fact, causality that we already @@ -267,7 +273,7 @@ That is the template: **the telemetry is in-box; the OpenTelemetry-shaped conven separate, near-empty package** — and in our case that package already exists and already has a maintainer, so the wrapper need not be ours at all. -## 8. What contrib emits today — the compatibility baseline +## 8. What the contrib package emits today — the compatibility baseline If we intend to replace the package (see `plan.md`), this is the inventory we have to match. Read off contrib `main` as of 2026-09-17, package version 1.18.0-beta.3. @@ -338,7 +344,7 @@ and the script text. This is the reflection payload from §1. **User-facing options:** `FlushInterval` (default 10s), `SetVerboseDatabaseStatements` (false), `EnrichActivityWithTimingEvents` (true), `Filter`, `Enrich`. -**Licensing:** contrib is Apache-2.0 (SPDX headers on every file); this repo is MIT. Code owner is +**Licensing:** the contrib repository is Apache-2.0 (SPDX headers on every file); this repo is MIT. Code owner is @matt-hensley. ## 9. Npgsql's public surface, in full diff --git a/notes/otel/plan.md b/notes/otel/plan.md index b66f5a6a4..f71a28e9b 100644 --- a/notes/otel/plan.md +++ b/notes/otel/plan.md @@ -25,6 +25,13 @@ No registration call, no connection handed to a builder, no `RegisterProfiler`. profiler" — stops existing**, because there is nothing left to register. Same for the seven `AddRedisInstrumentation` overloads in the contrib package and its DI deferral dance. +> Throughout these notes, **"the contrib package"** means +> [`OpenTelemetry.Instrumentation.StackExchangeRedis`](https://github.com/open-telemetry/opentelemetry-dotnet-contrib/tree/main/src/OpenTelemetry.Instrumentation.StackExchangeRedis), +> which lives in `opentelemetry-dotnet-contrib` — the OpenTelemetry .NET SIG's repository for +> instrumenting libraries outside the core distribution. It is the package that instruments us +> today, from the outside, via `RegisterProfiler` and reflection. Where the *people* are meant, they +> are named as its maintainers. + ### What is *not* public No `public static class RedisTelemetry { public const string ActivitySourceName = ... }`. The name @@ -119,9 +126,9 @@ Additive is always fine. Changing the value or meaning of something that already not squatting their name. Anyone calling `AddRedisInstrumentation()` sees nothing — the extension just changes which source it adds — but anyone who hand-wrote `AddSource("OpenTelemetry.Instrumentation.StackExchangeRedis")` has to edit one string. Document - it prominently; consider asking contrib to add *both* names for a transition window. + it prominently; consider asking its maintainers to add *both* names for a transition window. 2. **`Filter` and `Enrich`.** Callbacks on their options object with no obvious in-box equivalent. - `ActivityListener.Sample` is the better home for filtering — see open question 3. + `ActivityListener.Sample` is the better home for filtering — see open question 4. ### On starting from their implementation @@ -233,9 +240,9 @@ package reference, and independent versioning of the telemetry. If we decide tha unacceptable, the split is what buys it — and nothing else. And on the OpenTelemetry-typed wrapper specifically: Npgsql needed a second package because nobody -else was going to write `AddNpgsql()` for them. **We do not have that problem** — contrib already -owns `AddRedisInstrumentation`, already has a maintainer, and would be reduced to precisely those -four lines. That is the conversation to have with them. +else was going to write `AddNpgsql()` for them. **We do not have that problem** — the contrib +package already owns `AddRedisInstrumentation`, already has a maintainer, and would be reduced to +precisely those four lines. That is the conversation to have with them. ## Target frameworks @@ -300,7 +307,7 @@ shippable. Expose the statement string so the contrib package can stop emitting `DynamicMethod` field getters against our privates, on 3.x, today. This is worth doing *whatever* we decide about the rest: -contrib has to support 3.x for years regardless. +the contrib package has to support 3.x for years regardless. `IProfiledCommand` is a public interface, so adding a member to it is a break for implementers (realistically only test doubles — `ProfiledCommand` is internal sealed — but a break). Preferred @@ -382,8 +389,8 @@ Worth putting to @martincostello directly, since he offered to collaborate: `SER00x` machinery in `src/RESPite/Shared/Experiments.cs`), or do we accept that our stable package emits attributes that may be renamed under us? This is the strongest argument for the middle ground and we should hear it argued before dismissing it. -2. **Does contrib want to become a one-liner, or keep producing spans?** If we go native, does the - package retire, or keep an `AddRedisInstrumentation()` that calls `AddSource` plus the old +2. **Do its maintainers want the package to become a one-liner, or to keep producing spans?** If we + go native, does it retire, or keep an `AddRedisInstrumentation()` that calls `AddSource` plus the old `Filter`/`Enrich` knobs? Those two callbacks are the only features that do not obviously survive the transition. Either way, ask them to add our source name — and ideally to keep adding their own for a window, so existing hand-written `AddSource` calls do not break. From a46640fa3c5831c69c81993517ad6f7470f6d000 Mon Sep 17 00:00:00 2001 From: mgravell Date: Thu, 17 Sep 2026 14:21:04 +0100 Subject: [PATCH 5/8] Cut the side arcs protobuf-net was only ever in the PR body, but it was off topic there: how another project lays out its notes is not something a reader of this one needs. The convention stands on its own description. Also removed three tangents that were about the documents rather than about OpenTelemetry: a pointer at AGENTS.md, and two passages narrating my own reasoning - "my instinct was...", "an earlier draft of this plan said...". A future reader has not seen the earlier draft and does not need the autobiography; what they need is the trap itself, so the semconv default is now stated as the trap most likely to catch an implementation, and the Filter/Enrich question as two implementations landing on the opposite answer. --- notes/otel/findings.md | 6 ++---- notes/otel/plan.md | 30 +++++++++++++++--------------- 2 files changed, 17 insertions(+), 19 deletions(-) diff --git a/notes/otel/findings.md b/notes/otel/findings.md index 4ab9f34d0..78aae966f 100644 --- a/notes/otel/findings.md +++ b/notes/otel/findings.md @@ -11,10 +11,8 @@ The conversation this came out of is [#1044](https://github.com/StackExchange/St `OpenTelemetry.Instrumentation.StackExchangeRedis` is a community package in [opentelemetry-dotnet-contrib](https://github.com/open-telemetry/opentelemetry-dotnet-contrib/tree/main/src/OpenTelemetry.Instrumentation.StackExchangeRedis), the OpenTelemetry .NET SIG's repository for instrumentation of libraries outside the core -distribution. **These notes call that package "the contrib package" throughout** — the OpenTelemetry -project uses "contrib" for the repository, and it is shorter than writing the package name out -thirty times. Where the *people* are meant rather than the package, they are named as its -maintainers. +distribution. **These notes call that package "the contrib package" throughout**; where the +*people* are meant rather than the package, they are named as its maintainers. It has exactly one hook into us: `IConnectionMultiplexer.RegisterProfiler`. diff --git a/notes/otel/plan.md b/notes/otel/plan.md index f71a28e9b..d209b4bcc 100644 --- a/notes/otel/plan.md +++ b/notes/otel/plan.md @@ -88,12 +88,13 @@ deviation down here.** People have dashboards, alerts and saved queries built on Concretely, from findings §8: - **Honour `OTEL_SEMCONV_STABILITY_OPT_IN`**, with the same three modes and the same default. This - is the single most important item: the default today is `Old`, so unopted users are seeing - `db.system` and `db.statement`, *not* `db.system.name` and `db.query.text`. An earlier draft of - this plan said "new names only" — that was wrong, and would have silently broken every existing - consumer. Honouring the env var is also simply the correct behaviour for any .NET library - emitting database telemetry, and it gives a clean exit: when the conventions go stable, our - default flips in lockstep with the rest of the ecosystem rather than on our own schedule. + is the single most important item, and the easiest to get wrong: the default today is `Old`, so + unopted users are seeing `db.system` and `db.statement`, *not* `db.system.name` and + `db.query.text`. Emitting only the current names would look like the modern, correct choice and + would silently break every existing consumer. Honouring the env var is also simply the correct + behaviour for any .NET library emitting database telemetry, and it gives a clean exit: when the + conventions go stable, our default flips in lockstep with the rest of the ecosystem rather than + on our own schedule. - **Keep `db.redis.database_index`** in old mode. It is contrib-specific, not semconv, and it is in people's dashboards. - **Keep the timing events** — `Enqueued`, `Sent`, `ResponseReceived` — and keep them on by @@ -328,7 +329,7 @@ Returns command + key + script text — the exact payload the reflection reconst decision on whether it is gated by `EmitQueryText` or always available to a caller who asks (it is already opt-in by virtue of being an explicit call). -*Open:* interface member vs. extension method. Raising rather than assuming, per AGENTS.md. +*Open:* interface member vs. extension method. ### Phase 2 — metrics @@ -397,14 +398,13 @@ Worth putting to @martincostello directly, since he offered to collaborate: 3. **Can we have the tests?** Their test suite is the compatibility oracle, and it is Apache-2.0 going into an MIT repo. Explicit blessing, an agreed attribution form, or a clean-room rewrite from the behaviour inventory — but decided up front, not discovered in review. -4. **`Filter` and `Enrich` equivalents.** My instinct was that `ActivityListener.Sample` makes - in-box filtering unnecessary — Nick's 2022 objection was precisely that *deciding not to - profile* costs something per command, and `Sample` moves that decision to the collector. - **Npgsql's experience says otherwise**: they ship filters, enrichment callbacks and span-name - providers for commands, batches and copy operations, and that is the bulk of their public - surface (findings §9). Contrib ships `Filter`/`Enrich` too. Two independent implementations - converging on the same thing is worth more than my instinct. If we adopt them we need to settle - what gets handed to the callback first — see "Public API impact". +4. **`Filter` and `Enrich` equivalents.** The case against is that `ActivityListener.Sample` already + does this at the collector — which answers Nick's 2022 objection that *deciding not to profile* + costs something per command. The case for is that both existing implementations shipped them + anyway: Npgsql has filters, enrichment callbacks and span-name providers for commands, batches + and copy operations, which is the bulk of their public surface (findings §9), and the contrib + package has `Filter`/`Enrich`. If we adopt them, what gets handed to the callback has to be + settled first — see "Public API impact". 5. **Version/schema pinning.** Should the `ActivitySource` version track the package version, or a semconv schema version (contrib uses the latter via `ActivitySourceFactory.Create(version)`)? 6. **Does this need to wait for v4?** The IO core rewrite moves all five hook sites. Hooks placed From cb8d0a597b034a2d4a5a1a4ca100c66eadcdcf86 Mon Sep 17 00:00:00 2001 From: mgravell Date: Thu, 17 Sep 2026 14:25:45 +0100 Subject: [PATCH 6/8] Link the semantic conventions, and ask whether the convention choice should be explicit The semconv docs were named as paths but never linked; they are now linked properly, using the two @martincostello pointed at in #1044 (db/redis.md over db/database-spans.md) plus db/database-metrics.md for the metrics half. New open question 5: OTEL_SEMCONV_STABILITY_OPT_IN is ambient and process-wide, and a library changing the attribute names it emits because of an environment variable is how you get a baffled bug report. Proposes an explicit enum whose Default still follows the variable, so a service-wide setting keeps every instrumentation in agreement - which is the point of the variable, and the reason not to just ignore it. Per mgravell: individual enum members can carry [Experimental] - verified, ExperimentalAttribute includes AttributeTargets.Field in both the runtime's version and our down-level polyfill, and enum members are fields. That scopes the semconv churn risk to the members that track unstable conventions, leaving Default/Old as plain stable API, and the exit is clean: deleting the attribute later is neither a source nor a binary break. Much better than gating the whole feature, so question 1 now points at it. --- notes/otel/findings.md | 29 +++++++++++++------ notes/otel/plan.md | 64 +++++++++++++++++++++++++++++++++++++++--- 2 files changed, 80 insertions(+), 13 deletions(-) diff --git a/notes/otel/findings.md b/notes/otel/findings.md index 78aae966f..d256ca6ee 100644 --- a/notes/otel/findings.md +++ b/notes/otel/findings.md @@ -120,12 +120,22 @@ nothing they have today. ## 3. Semantic conventions -Two documents apply, both in -[open-telemetry/semantic-conventions](https://github.com/open-telemetry/semantic-conventions): - -**Spans** (`docs/db/redis.md`, over `docs/db/database-spans.md`). Span kind `CLIENT`; span name -follows the general database convention **except** that `db.namespace` is deliberately left out of -the name, because for Redis it is a bare integer and reads as noise. +Three documents apply, all in +[open-telemetry/semantic-conventions](https://github.com/open-telemetry/semantic-conventions). The +first two are the ones @martincostello pointed at in +[#1044](https://github.com/StackExchange/StackExchange.Redis/issues/1044#issuecomment-5713538582) +as what the contrib package is implementing: + +- [`docs/db/redis.md`](https://github.com/open-telemetry/semantic-conventions/blob/main/docs/db/redis.md) + — Redis client spans +- [`docs/db/database-spans.md`](https://github.com/open-telemetry/semantic-conventions/blob/main/docs/db/database-spans.md) + — the general database span conventions the Redis one builds on +- [`docs/db/database-metrics.md`](https://github.com/open-telemetry/semantic-conventions/blob/main/docs/db/database-metrics.md) + — database client metrics + +**Spans.** Span kind `CLIENT`; span name follows the general database convention **except** that +`db.namespace` is deliberately left out of the name, because for Redis it is a bare integer and +reads as noise. | attribute | requirement level | where we get it | | --- | --- | --- | @@ -142,7 +152,7 @@ the name, because for Redis it is a bare integer and reads as noise. For batches the convention is to prepend `MULTI` or `PIPELINE` to the span name when the constituent operations share a command. -**Metrics** (`docs/db/database-metrics.md`). `db.client.operation.duration` is a histogram in +**Metrics** (`database-metrics.md` above). `db.client.operation.duration` is a histogram in seconds and is **stable**, with recommended buckets `[0.001, 0.005, 0.01, 0.05, 0.1, 0.5, 1, 5, 10]`; required attribute `db.system.name`, plus `db.operation.name`, `db.namespace`, `server.address`/`server.port`, `error.type` on failure. The @@ -194,8 +204,9 @@ Takeaways: 1. A client library shipping or blessing its own telemetry is normal, not novel. go-redis does it in-repo; Lettuce does it via an SPI plus adapters. -2. **There is no agreed cross-language vocabulary beyond the semantic conventions themselves**, and - adoption of those lags badly and unevenly — go-redis is still on `db.statement`, Python has a +2. **There is no agreed cross-language vocabulary beyond the + [semantic conventions](https://github.com/open-telemetry/semantic-conventions/blob/main/docs/db/redis.md) + themselves**, and adoption of those lags badly and unevenly — go-redis is still on `db.statement`, Python has a dual-mode switch, .NET has a triple-source switch. Following current semconv puts us *ahead* of the field, not out of step with it. 3. Where the clients *do* informally agree, it is on things semconv does not mandate: span name is diff --git a/notes/otel/plan.md b/notes/otel/plan.md index d209b4bcc..2ce996a33 100644 --- a/notes/otel/plan.md +++ b/notes/otel/plan.md @@ -94,7 +94,8 @@ Concretely, from findings §8: would silently break every existing consumer. Honouring the env var is also simply the correct behaviour for any .NET library emitting database telemetry, and it gives a clean exit: when the conventions go stable, our default flips in lockstep with the rest of the ecosystem rather than - on our own schedule. + on our own schedule. Honouring it does not mean it has to be the *only* way to choose — see open + question 5 on making the selection an explicit enum as well. - **Keep `db.redis.database_index`** in old mode. It is contrib-specific, not semconv, and it is in people's dashboards. - **Keep the timing events** — `Enqueued`, `Sent`, `ResponseReceived` — and keep them on by @@ -389,7 +390,9 @@ Worth putting to @martincostello directly, since he offered to collaborate: shipping perpetual betas. Do we mark the telemetry `[Experimental]` (we already have the `SER00x` machinery in `src/RESPite/Shared/Experiments.cs`), or do we accept that our stable package emits attributes that may be renamed under us? This is the strongest argument for the - middle ground and we should hear it argued before dismissing it. + middle ground and we should hear it argued before dismissing it. **See question 5 for a way to + make this much less binary** — gating individual enum members rather than the whole feature, so + only the callers who opted into unstable conventions carry the diagnostic. 2. **Do its maintainers want the package to become a one-liner, or to keep producing spans?** If we go native, does it retire, or keep an `AddRedisInstrumentation()` that calls `AddSource` plus the old `Filter`/`Enrich` knobs? Those two callbacks are the only features that do not obviously survive @@ -405,9 +408,62 @@ Worth putting to @martincostello directly, since he offered to collaborate: and copy operations, which is the bulk of their public surface (findings §9), and the contrib package has `Filter`/`Enrich`. If we adopt them, what gets handed to the callback has to be settled first — see "Public API impact". -5. **Version/schema pinning.** Should the `ActivitySource` version track the package version, or a +5. **Should the semconv choice be an explicit enum rather than only an environment variable?** + `OTEL_SEMCONV_STABILITY_OPT_IN` is ambient, process-wide, and read once at construction — a + library silently changing the attribute names it emits based on an environment variable is + exactly the sort of action-at-a-distance that produces a baffled bug report. An explicit setting + means nobody is surprised: + + ```csharp + // sketch only + public enum RedisSemanticConventions { Default, Old, New, Both } + + options.SemanticConventions = RedisSemanticConventions.New; + ``` + + `Default` would mean "follow `OTEL_SEMCONV_STABILITY_OPT_IN`, and its `Old` default if unset", so + an operator who sets the variable once still gets every instrumentation in the service agreeing + — which is the whole point of the variable, and a strong reason *not* to simply ignore it. + Anything else is an explicit override that wins. + + Costs three or four more public API lines than the sketch above (the enum, the property pair, the + `DefaultOptionsProvider` virtual). Probably worth it: this is the single setting most likely to + produce "why did my dashboard go blank", and the one place where being explicit is cheap. + + Two details to settle if we do it: whether `Default` resolves at multiplexer construction (as the + contrib package does) or per command, and whether it is connection-string-parsable like the rest + of `ConfigurationOptions`. + + **This is also a better answer to open question 1 than gating the whole feature.** + `ExperimentalAttribute` includes `AttributeTargets.Field` — in the runtime's version *and* in the + down-level polyfill in `src/RESPite/Shared/Experiments.cs`, so it works on our netfx targets too + — and enum members are fields. So individual members can be gated: + + ```csharp + public enum RedisSemanticConventions + { + Default, + Old, + [Experimental(Experiments.SemanticConventions, UrlFormat = Experiments.UrlFormat)] New, + [Experimental(Experiments.SemanticConventions, UrlFormat = Experiments.UrlFormat)] Both, + } + ``` + + The stable half — `Default` and `Old`, which is what everyone gets today — stays plain stable + API. Only the members that actually track still-experimental conventions carry the gate, so the + churn risk is scoped to the people who opted into churn. Marking the *whole* telemetry feature + `[Experimental]` is all-or-nothing and puts a diagnostic in front of people who only ever wanted + the default behaviour. + + And the exit is clean: when the database conventions go stable, delete the attribute from the + member. That is neither a source nor a binary break — the diagnostic simply stops firing. + + Repo convention is `[Experimental(Experiments.X, UrlFormat = Experiments.UrlFormat)]` with a + `docs/exp/SERxxx.md` page; `SER001`–`SER009` are taken (retired IDs stay reserved), so this would + be `SER010`. +6. **Version/schema pinning.** Should the `ActivitySource` version track the package version, or a semconv schema version (contrib uses the latter via `ActivitySourceFactory.Create(version)`)? -6. **Does this need to wait for v4?** The IO core rewrite moves all five hook sites. Hooks placed +7. **Does this need to wait for v4?** The IO core rewrite moves all five hook sites. Hooks placed now survive as *concepts* but not as code. Landing metrics on 3.x and traces on 4.x is a defensible split; so is landing both on 3.x and accepting the port. From 76c7ba1fe3842b8c993351059d16f4d8f8a27ec2 Mon Sep 17 00:00:00 2001 From: mgravell Date: Thu, 17 Sep 2026 14:28:56 +0100 Subject: [PATCH 7/8] Get the net461/netstandard2.0 story right I had written that net461-on-netstandard2.0 is "the combination Microsoft stopped supporting", which is the wrong framing - nothing was withdrawn. The official .NET Standard table does list .NET Framework 4.6.1 under .NET Standard 2.0, but footnote 2 says that is the rule NuGet uses to decide applicability, that there are several issues consuming such libraries from 4.6.1 projects, and that the recommendation is to move to 4.7.2 or higher. Restore succeeds; whether it works depends on the netstandard.dll facade and binding redirects. Footnote now quoted in place, so the next person does not have to re-derive it. The conclusion is unchanged and in fact better supported: there are two independent reasons to leave net461 out, since the package has no net461 asset at all after 6.0.1 and the only fallback is the path Microsoft advises against. plan.md needed no change - it only ever cited the missing asset. --- notes/otel/findings.md | 25 +++++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/notes/otel/findings.md b/notes/otel/findings.md index d256ca6ee..33dd493ca 100644 --- a/notes/otel/findings.md +++ b/notes/otel/findings.md @@ -107,12 +107,29 @@ nuget.org: | `net8.0` | in-box, same | no `PackageReference` needed | | `net472` | package, `lib/net462` asset | fine | | `netstandard2.0` | package, `lib/netstandard2.0` asset | fine | -| `net461` | package, falls back to `lib/netstandard2.0` | **not supported** | +| `net461` | package, falls back to `lib/netstandard2.0` | **don't** — see below | `System.Diagnostics.DiagnosticSource` dropped its `net461` asset after 6.0.1 — 8.0.x, 9.0.x and -10.0.x ship `net462` as the lowest .NET Framework target. NuGet *will* hand a net461 project the -`netstandard2.0` asset, but that is the combination Microsoft stopped supporting and the one that -needs the `netstandard.dll` facade plus binding redirects to behave. +10.0.x ship `net462` as the lowest .NET Framework target. That leaves the `netstandard2.0` asset as +the only candidate for a net461 project, and **net461 consuming netstandard2.0 is a NuGet +resolution rule rather than a support statement.** The +[.NET Standard table](https://learn.microsoft.com/en-us/dotnet/standard/net-standard) does list +.NET Framework 4.6.1 under .NET Standard 2.0, but with this footnote attached: + +> The versions listed here represent the rules that NuGet uses to determine whether a given .NET +> Standard library is applicable. While NuGet considers .NET Framework 4.6.1 as supporting .NET +> Standard 1.5 through 2.0, there are several issues with consuming .NET Standard libraries that +> were built for those versions from .NET Framework 4.6.1 projects. For .NET Framework projects +> that need to use such libraries, we recommend that you upgrade the project to target .NET +> Framework 4.7.2 or higher. + +So the restore succeeds and the build may well succeed; whether it *works* depends on the +`netstandard.dll` facade and binding redirects being right. 4.7.2 is the floor Microsoft actually +stands behind — which is also why `net472` is in our target list and `net461` is the one that +cannot be made comfortable here. + +Two independent reasons to leave net461 out, then: the package has no net461 asset, and the only +fallback is the path Microsoft explicitly advises against. So: telemetry everywhere except `net461`, which compiles the instrumentation out. That matches the existing `VECTOR_SAFE` / `UNIX_SOCKET` idiom in `StackExchange.Redis.csproj` and costs net461 users From 16c17d58e5354f0d45a4dfc804224944ee4b6bca Mon Sep 17 00:00:00 2001 From: mgravell Date: Thu, 17 Sep 2026 14:30:41 +0100 Subject: [PATCH 8/8] net461 is the question, not the special case mgravell's suggestion - drop net461 or move to net462 - is right that the TELEMETRY conditional was the wrong answer, and checking it out makes the case stronger than either option. net461 went out of support on 27 April 2022, retired with 4.5.2 and 4.6 for being SHA-1 signed. And every Microsoft package we already depend on bottoms out at net462: logging abstractions, hashing, pipelines, async interfaces, channels, not one with a net461 asset. So the net461 build already resolves all of them through netstandard2.0 - the exact path the .NET Standard footnote warns about. Carving telemetry out of net461 would have been protecting a target that has nothing left to protect. On net462 specifically: supported only until 13 January 2027, so bumping the floor onto it buys four months. net472 has no end-of-support date, is what the footnote itself recommends, and is already a target - so "drop net461" and "move to net472" are the same change. Dropping is also not a hard break; net461 consumers fall back to our netstandard2.0 asset, as they already do for every dependency above. Recorded with the knock-on tidying (VECTOR_SAFE becomes unconditional, three conditional PackageReferences and an analyzer branch go), but explicitly left undecided: this belongs to the package, not to telemetry. --- notes/otel/findings.md | 46 +++++++++++++++++++++++++++++++++------ notes/otel/plan.md | 49 +++++++++++++++++++++++++++++++++++------- 2 files changed, 80 insertions(+), 15 deletions(-) diff --git a/notes/otel/findings.md b/notes/otel/findings.md index 33dd493ca..4e913b364 100644 --- a/notes/otel/findings.md +++ b/notes/otel/findings.md @@ -125,15 +125,47 @@ resolution rule rather than a support statement.** The So the restore succeeds and the build may well succeed; whether it *works* depends on the `netstandard.dll` facade and binding redirects being right. 4.7.2 is the floor Microsoft actually -stands behind — which is also why `net472` is in our target list and `net461` is the one that -cannot be made comfortable here. +stands behind. -Two independent reasons to leave net461 out, then: the package has no net461 asset, and the only -fallback is the path Microsoft explicitly advises against. +### The net461 target is already notional -So: telemetry everywhere except `net461`, which compiles the instrumentation out. That matches the -existing `VECTOR_SAFE` / `UNIX_SOCKET` idiom in `StackExchange.Redis.csproj` and costs net461 users -nothing they have today. +Special-casing telemetry out of net461 would be protecting a target that is *already* built +entirely on the fallback above. Checked against nuget.org, every Microsoft package +`StackExchange.Redis` already depends on: + +| package | lowest .NET Framework asset | +| --- | --- | +| `Microsoft.Extensions.Logging.Abstractions` 10.0.5 | `net462` | +| `System.IO.Hashing` 10.0.12 | `net462` | +| `System.IO.Pipelines` 10.0.12 | `net462` | +| `Microsoft.Bcl.AsyncInterfaces` 10.0.12 | `net462` | +| `System.Threading.Channels` 10.0.12 | `net462` | + +**Not one of them ships a net461 asset.** Our net461 build already resolves every one of them +through `netstandard2.0` — the exact path the footnote warns about. `System.Diagnostics.DiagnosticSource` +would not be introducing a new problem; it would be joining a queue. + +### And net461 has been out of support since 2022 + +From the [.NET Framework lifecycle](https://learn.microsoft.com/en-us/lifecycle/products/microsoft-net-framework): + +| version | end of support | +| --- | --- | +| 4.6.1 | **27 April 2022** — retired alongside 4.5.2 and 4.6 | +| 4.6.2 | **13 January 2027** | +| 4.7, 4.7.1, 4.7.2, 4.8, 4.8.1 | none | + +4.5.2, 4.6 and 4.6.1 were retired specifically because they were signed with SHA-1 certificates. + +Note the middle row: **net462 is supported for about four more months** from these notes. Bumping a +floor onto a version that expires within the year is not a bump worth doing — which is what points +at `net472`, the oldest .NET Framework with no end-of-support date, the version Microsoft's own +footnote names ("upgrade the project to target .NET Framework 4.7.2 or higher"), and a target +`StackExchange.Redis` already ships. + +So the question is not "how do we compile telemetry out of net461" but "why is net461 still a +target". See `plan.md`, "The net461 question", which is a repo-level decision this topic merely +surfaced. ## 3. Semantic conventions diff --git a/notes/otel/plan.md b/notes/otel/plan.md index 2ce996a33..104c95d09 100644 --- a/notes/otel/plan.md +++ b/notes/otel/plan.md @@ -257,14 +257,44 @@ dropped its `net461` asset after 6.0.1 and now bottoms out at `net462` (see find | `net8.0` | yes | none — in-box | | `net472` | yes | `System.Diagnostics.DiagnosticSource` (`lib/net462`) | | `netstandard2.0` | yes | `System.Diagnostics.DiagnosticSource` (`lib/netstandard2.0`) | -| `net461` | **compiled out** | none | +| `net461` | see below | — | -Guarded by a `TELEMETRY` compile symbol defined for everything but `net461`, following the existing -`VECTOR_SAFE` / `UNIX_SOCKET` idiom in `StackExchange.Redis.csproj`. Version pinned in -`Directory.Packages.props` like every other reference. +Version pinned in `Directory.Packages.props` like every other reference. -net461 users lose nothing they have today; the existing profiling API stays where it is on every -target. +### The net461 question + +An earlier version of this section proposed a `TELEMETRY` compile symbol defined for everything but +`net461`, on the `VECTOR_SAFE` / `UNIX_SOCKET` model. **Don't.** The evidence in findings §2 says +the conditional would be protecting a target that does not need protecting: + +- **net461 has been out of support since 27 April 2022** — retired with 4.5.2 and 4.6 because they + were SHA-1 signed. +- **Every Microsoft package we already depend on bottoms out at `net462`** — logging abstractions, + hashing, pipelines, async interfaces, channels. Not one ships a net461 asset. Our net461 build is + *already* resolving all of them through `netstandard2.0`, which is precisely the combination the + .NET Standard footnote warns about. Adding `System.Diagnostics.DiagnosticSource` to that list + changes nothing about the risk profile; it joins a queue. + +So the honest options are to drop `net461`, or to raise the floor — and if raising it, **`net472`, +not `net462`**: 4.6.2 itself loses support on 13 January 2027, so bumping onto it buys a few months. +`net472` has no end-of-support date, is the version Microsoft's own footnote names, and is a target +we already ship — so "drop net461" and "move to net472" are the same change. + +What net461 consumers would actually experience: not a hard break. NuGet still considers net461 +compatible with `netstandard2.0`, so they would fall back to our `netstandard2.0` asset — the same +fallback they already get for every dependency listed above. + +Knock-on tidying if net461 goes: `VECTOR_SAFE` becomes unconditional, the +`System.Runtime.InteropServices.RuntimeInformation` reference disappears, the `net461` arms of the +`Microsoft.Bcl.AsyncInterfaces` / `System.Threading.Channels` / `System.IO.Compression` conditions +go, and `Directory.Build.targets` loses its net461 analyzer branch. Same for `src/RESPite` and +`toys/TestConsoleBaseline`. + +**This is a repo-level decision that telemetry merely surfaced, not a telemetry decision** — it +affects the whole package and wants deciding on its own terms, probably alongside v4. Recorded here +because this is where the evidence turned up. If net461 stays for reasons outside this topic, the +`TELEMETRY` symbol is the fallback and costs net461 users nothing they have today; the profiling +API stays on every target regardless. ## Pay-for-play @@ -476,5 +506,8 @@ Worth putting to @martincostello directly, since he offered to collaborate: `OTEL_SEMCONV_STABILITY_OPT_IN` like every other .NET database instrumentation, including its current `Old` default, however much we might prefer the new names. - **Not putting command argument values in spans.** Command and key only, and that opt-in. -- **Not removing or changing the profiling API.** It stays, unchanged, on every target including - `net461`. Native telemetry is additive. +- **Not removing or changing the profiling API.** It stays, unchanged, on every target we ship. + Native telemetry is additive. +- **Not deciding the net461 question on telemetry's behalf.** The evidence is recorded above + because this is where it surfaced, but the target list belongs to the package, not to this + feature.