diff --git a/notes/otel/findings.md b/notes/otel/findings.md new file mode 100644 index 000000000..4e913b364 --- /dev/null +++ b/notes/otel/findings.md @@ -0,0 +1,461 @@ +# 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), +the OpenTelemetry .NET SIG's repository for instrumentation of libraries outside the core +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`. + +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` | **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. 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. + +### The net461 target is already notional + +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 + +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 | +| --- | --- | --- | +| `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** (`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 +`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](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 + 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. + +## 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. + +**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:** 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 + +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 new file mode 100644 index 000000000..104c95d09 --- /dev/null +++ b/notes/otel/plan.md @@ -0,0 +1,513 @@ +# 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. + +> 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 +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`. + +## 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, 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. 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 + 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 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 4. + +### 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. + +## 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** — 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 + +`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` | see below | — | + +Version pinned in `Directory.Packages.props` like every other reference. + +### 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 + +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: +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 +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. + +### 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, 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`. +**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 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. **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 + 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.** 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. **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)`)? +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. + +## 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 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 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. 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.