diff --git a/apps/desktop/src/app/DesktopOtlpExport.test.ts b/apps/desktop/src/app/DesktopOtlpExport.test.ts index 6483652d75d5..f495354e310f 100644 --- a/apps/desktop/src/app/DesktopOtlpExport.test.ts +++ b/apps/desktop/src/app/DesktopOtlpExport.test.ts @@ -125,6 +125,20 @@ describe("resolveDesktopOtlpExport", () => { }), ); + it.effect("keeps exporting when T3 Code's own name says to", () => + Effect.gen(function* () { + const resolved = yield* resolve( + { + T3CODE_OTEL_SDK_DISABLED: "false", + OTEL_SDK_DISABLED: "true", + }, + { named: { traces: "http://127.0.0.1:4318/v1/traces" } }, + ); + assert.strictEqual(resolved.traces.url, "http://127.0.0.1:4318/v1/traces"); + assert.deepStrictEqual(resolved.warnings, []); + }), + ); + it.effect("declines only the signal that asked for a protocol T3 Code cannot speak", () => Effect.gen(function* () { const resolved = yield* resolve({ diff --git a/apps/desktop/src/app/DesktopOtlpExport.ts b/apps/desktop/src/app/DesktopOtlpExport.ts index 9b5b78359fc7..5f6b8d1214b3 100644 --- a/apps/desktop/src/app/DesktopOtlpExport.ts +++ b/apps/desktop/src/app/DesktopOtlpExport.ts @@ -126,10 +126,7 @@ export const resolveDesktopOtlpExport = (input: DesktopOtlpExportInput): Desktop metrics: offSignal, logs: offSignal, resource, - warnings: [ - ...otel.warnings, - "OTEL_SDK_DISABLED is set, so the desktop app exports no telemetry; this overrides T3CODE_OTLP_* and Settings too", - ], + warnings: [...otel.warnings], }; } diff --git a/apps/server/src/cli/config.test.ts b/apps/server/src/cli/config.test.ts index 8a3d940c6f3c..bda5fbdbd2d9 100644 --- a/apps/server/src/cli/config.test.ts +++ b/apps/server/src/cli/config.test.ts @@ -791,6 +791,18 @@ it.layer(NodeServices.layer)("cli config resolution", (it) => { }), ); + it.effect("keeps exporting when T3 Code's own name says to, whatever the standard one says", () => + Effect.gen(function* () { + const resolved = yield* resolveWithEnv({ + T3CODE_OTEL_SDK_DISABLED: "false", + OTEL_SDK_DISABLED: "true", + T3CODE_OTLP_TRACES_URL: "http://localhost:4318/v1/traces", + }); + + expect(resolved.otlpTracesUrl).toBe("http://localhost:4318/v1/traces"); + }), + ); + it.effect("falls back to persisted observability settings when env vars are absent", () => Effect.gen(function* () { const fs = yield* FileSystem.FileSystem; diff --git a/apps/server/src/observability/Layers/Observability.ts b/apps/server/src/observability/Layers/Observability.ts index 14e7d240afd4..e8c11eb0671a 100644 --- a/apps/server/src/observability/Layers/Observability.ts +++ b/apps/server/src/observability/Layers/Observability.ts @@ -27,12 +27,6 @@ export const ObservabilityLive = Layer.unwrap( yield* Effect.logWarning(warning); } - if (otel.disabled) { - yield* Effect.logWarning( - "OTEL_SDK_DISABLED is set, so no telemetry is exported; this overrides T3CODE_OTLP_* and Settings too", - ); - } - // One variable can decline every signal, and saying so three times reads // like three separate problems. const declined = new Set( diff --git a/docs/fork/0018-the-standard-otel-variables-are-honored.md b/docs/fork/0018-the-standard-otel-variables-are-honored.md index 74bb36d698df..a0b7aa26a4b2 100644 --- a/docs/fork/0018-the-standard-otel-variables-are-honored.md +++ b/docs/fork/0018-the-standard-otel-variables-are-honored.md @@ -26,7 +26,9 @@ log record arrives. - Turn export off from the environment. `OTEL_SDK_DISABLED=true` stops every export, including one configured in Settings, which is the one switch a - shared machine needs. + shared machine needs. `T3CODE_OTEL_SDK_DISABLED` is the same setting asked of + T3 Code's own name first, so `false` there keeps T3 Code exporting on a + machine whose profile disables every other SDK. - Keep whatever you have. The `T3CODE_OTLP_*` names, the desktop bootstrap envelope, and Settings all still win over the environment, and a setup that never mentioned OpenTelemetry keeps the wire format it always used. @@ -54,8 +56,15 @@ people can actually reach. Auto-enabling from an ambient endpoint is the deliberate part. Every other OpenTelemetry SDK behaves this way, and a telemetry variable that some processes -honor and others quietly ignore is worse than either answer, so `OTEL_SDK_DISABLED` -is the way out rather than a requirement to opt in. +honor and others quietly ignore is worse than either answer, so +`OTEL_SDK_DISABLED` is the way out rather than a requirement to opt in. + +Turning export off is one setting with two names, not two switches, and it is +read in the same order as everything else here: ours, then the standard one. +The ordering is the whole point. Inheriting `OTEL_SDK_DISABLED` from a shell +profile is common, and without a name of our own the only way to get T3 Code's +telemetry back would be to unset a variable the rest of the machine depends +on. ## Upstream considerations diff --git a/docs/fork/0022-the-desktop-app-reports-its-own-work.md b/docs/fork/0022-the-desktop-app-reports-its-own-work.md index 09856b82f10d..cf6ff0d4d499 100644 --- a/docs/fork/0022-the-desktop-app-reports-its-own-work.md +++ b/docs/fork/0022-the-desktop-app-reports-its-own-work.md @@ -18,7 +18,8 @@ Metrics stay off while the main process records none, so a configured metrics endpoint hears from the server and nobody else rather than receiving an empty payload every interval. -- Turn it off the same way. `OTEL_SDK_DISABLED=true` stops both processes. +- Turn it off the same way. `OTEL_SDK_DISABLED=true` stops both processes, and + so does `T3CODE_OTEL_SDK_DISABLED=true`, which is read first. - Tell the two apart without trusting the environment. The main process reports as `t3-desktop`, joining `t3-server` and `t3-web`, and `service.runtime` on it is always `desktop`, so an ambient diff --git a/docs/operations/observability.md b/docs/operations/observability.md index c3ffc646f100..0e09de740486 100644 --- a/docs/operations/observability.md +++ b/docs/operations/observability.md @@ -208,7 +208,9 @@ The base endpoint is a base, not a full URL: traces go to `/v1/traces` Set `OTEL_EXPORTER_OTLP_{TRACES,METRICS,LOGS}_ENDPOINT` when a signal needs a full URL of its own. Ambient `OTEL_*` variables turn export on by themselves. A work collector in your shell profile means -T3 Code exports to it, so use `OTEL_SDK_DISABLED=true` if that is not what you want. +T3 Code exports to it, so use `OTEL_SDK_DISABLED=true` if that is not what you want. If the reverse is +your problem, a profile that disables every other SDK on the machine, `T3CODE_OTEL_SDK_DISABLED=false` +keeps T3 Code exporting. #### Which Processes Export @@ -263,14 +265,18 @@ goes. The three signals are resolved separately, so traces can come from one source and metrics or logs from another. -`OTEL_SDK_DISABLED=true` outranks all four and stops every export, including one configured through -Settings. +Whether anything is exported at all is one setting, read in that same order: `T3CODE_OTEL_SDK_DISABLED` +answers it, and `OTEL_SDK_DISABLED` answers it only when T3 Code's own name is unset. Either way the +answer stops every export, including one configured through Settings, which is the one switch a shared +machine needs. Reading ours first is what lets `T3CODE_OTEL_SDK_DISABLED=false` override an ambient +`OTEL_SDK_DISABLED=true`, so a machine can disable every other SDK and still ask for T3 Code's +telemetry. #### What Is Read | Variable | Effect | | ------------------------------------------------------------------------------------ | ---------------------------------------------------------------------------- | -| `OTEL_SDK_DISABLED` | Stops all export | +| `OTEL_SDK_DISABLED` | Stops all export, unless `T3CODE_OTEL_SDK_DISABLED` answered first | | `OTEL_EXPORTER_OTLP_ENDPOINT` | Base URL for every signal | | `OTEL_EXPORTER_OTLP_{TRACES,METRICS,LOGS}_ENDPOINT` | Full URL for one signal | | `OTEL_EXPORTER_OTLP_HEADERS`, `OTEL_EXPORTER_OTLP_{TRACES,METRICS,LOGS}_HEADERS` | Export headers, per signal overriding the shared ones | @@ -341,7 +347,9 @@ An empty value means the same thing as an unset one, so `OTEL_SERVICE_VERSION=` variable were not there at all. An empty `OTEL_SERVICE_NAME` is not an attempt to rename anything, so it is not warned about either. `OTEL_SDK_DISABLED` follows the specification's one rule for booleans: the case-insensitive string `true` is the only value that switches export off, and -anything else, including `yes` and `1`, leaves it on. +anything else, including `yes` and `1`, leaves it on. `T3CODE_OTEL_SDK_DISABLED` is T3 Code's own +name, so it takes `true`, `1`, `yes`, `on` and their negatives, and a value it cannot read is reported +and then left to `OTEL_SDK_DISABLED` to answer rather than treated as either answer itself. A `OTEL_EXPORTER_OTLP_HEADERS` or `OTEL_RESOURCE_ATTRIBUTES` value that fails to decode is discarded whole rather than partly. A half-parsed credential reaches the collector as the same authentication @@ -725,6 +733,9 @@ OTLP export: - `T3CODE_OTLP_HEADERS`: extra headers for all three exporters, same format as `OTEL_EXPORTER_OTLP_HEADERS`: comma-separated `key=value` pairs with percent-encoded values. - `T3CODE_OTLP_PROTOCOL`: `http/json` (default) or `http/protobuf` +- `T3CODE_OTEL_SDK_DISABLED`: stops every export, whatever configured it, including Settings. Read + before `OTEL_SDK_DISABLED`, so `false` here keeps T3 Code exporting on a machine that sets the + standard name. If the OTLP URLs are unset, local tracing still works, metrics stay in-process only, and logs stay on stdout only. diff --git a/packages/shared/src/otelEnvironment.test.ts b/packages/shared/src/otelEnvironment.test.ts index dda0afb7f6dd..166801f6ead4 100644 --- a/packages/shared/src/otelEnvironment.test.ts +++ b/packages/shared/src/otelEnvironment.test.ts @@ -107,6 +107,57 @@ describe("OtelEnvironment", () => { assert.strictEqual(resolved.disabled, true); assert.strictEqual(resolved.traces.settings, undefined); assert.strictEqual(resolved.metrics.settings, undefined); + // Someone who inherited this from a shell profile has somewhere to go. + assert.include(resolved.warnings.join("\n"), "T3CODE_OTEL_SDK_DISABLED=false"); + }), + ); + + it.effect("lets T3 Code's own name answer before the standard one", () => + Effect.gen(function* () { + const off = yield* OtelEnvironment.load.pipe( + withEnv({ + T3CODE_OTEL_SDK_DISABLED: "true", + OTEL_EXPORTER_OTLP_ENDPOINT: "https://collector.example.com", + }), + ); + assert.isTrue(off.disabled); + assert.strictEqual(off.traces.settings, undefined); + assert.deepStrictEqual(off.warnings, [ + "T3CODE_OTEL_SDK_DISABLED is set, so no telemetry is exported, whatever configured it", + ]); + + // The point of reading ours first: a machine that disables every other + // SDK in its shell profile can still ask for T3 Code's telemetry. + const on = yield* OtelEnvironment.load.pipe( + withEnv({ + T3CODE_OTEL_SDK_DISABLED: "false", + OTEL_SDK_DISABLED: "true", + OTEL_EXPORTER_OTLP_ENDPOINT: "https://collector.example.com", + }), + ); + assert.isFalse(on.disabled); + assert.isDefined(on.traces.settings); + assert.deepStrictEqual(on.warnings, []); + }), + ); + + it.effect("reads T3 Code's own name the way T3 Code reads a boolean", () => + Effect.gen(function* () { + // Ours to define, so it takes the affirmatives people type. The + // specification's single-value rule stays with the OTEL_* name. + const numeric = yield* OtelEnvironment.load.pipe(withEnv({ T3CODE_OTEL_SDK_DISABLED: "1" })); + assert.isTrue(numeric.disabled); + + // A value that answers nothing leaves the source under it to answer. + const nonsense = yield* OtelEnvironment.load.pipe( + withEnv({ + T3CODE_OTEL_SDK_DISABLED: "maybe", + OTEL_SDK_DISABLED: "true", + OTEL_EXPORTER_OTLP_ENDPOINT: "https://collector.example.com", + }), + ); + assert.isTrue(nonsense.disabled); + assert.include(nonsense.warnings.join("\n"), "T3CODE_OTEL_SDK_DISABLED=maybe"); }), ); diff --git a/packages/shared/src/otelEnvironment.ts b/packages/shared/src/otelEnvironment.ts index aef5beb22449..0d623b154089 100644 --- a/packages/shared/src/otelEnvironment.ts +++ b/packages/shared/src/otelEnvironment.ts @@ -9,7 +9,9 @@ * told twice. * * Read by every T3 Code process that exports telemetry, so the server and the - * desktop app cannot disagree about what a variable means. + * desktop app cannot disagree about what a variable means. That is also why + * `T3CODE_OTEL_SDK_DISABLED` is read here: it is the same setting as + * `OTEL_SDK_DISABLED`, asked of T3 Code's own name first. * * Only the variables T3 Code can act on are read. The exporter speaks * OTLP over HTTP, so `grpc` is declined loudly rather than answered with a @@ -85,7 +87,13 @@ export interface OtlpResourceSettings { } export interface OtelEnvironment { - /** `OTEL_SDK_DISABLED`. When set, nothing is exported by any route. */ + /** + * Whether anything is exported at all, by any route. `T3CODE_OTEL_SDK_DISABLED` + * answers it, and `OTEL_SDK_DISABLED` answers it only when T3 Code's own name + * is unset, which is the source order every other setting here follows. So + * `T3CODE_OTEL_SDK_DISABLED=false` is how a machine that exports + * `OTEL_SDK_DISABLED` for everything else keeps T3 Code exporting. + */ readonly disabled: boolean; /** * Settings that were named but could not be used, each already phrased for a @@ -125,6 +133,35 @@ const optionalString = (name: string) => const specBoolean = (name: string) => optionalString(name).pipe(Effect.map((raw) => raw?.toLowerCase() === "true")); +/** + * A `T3CODE_*` name is ours, so it answers to the affirmatives people actually + * type rather than the single value the specification allows. `undefined` means + * the name did not answer, either because it is unset or because its value was + * unreadable, and the source under it decides instead. A typo therefore costs + * that variable and nothing else, the same as everywhere else here. + */ +const t3Boolean = (name: string) => + optionalString(name).pipe( + Effect.map( + (raw): { readonly value: boolean | undefined; readonly warnings: ReadonlyArray } => { + if (raw === undefined) { + return { value: undefined, warnings: [] }; + } + const value = raw.toLowerCase(); + if (["true", "1", "yes", "on"].includes(value)) { + return { value: true, warnings: [] }; + } + if (["false", "0", "no", "off"].includes(value)) { + return { value: false, warnings: [] }; + } + return { + value: undefined, + warnings: [`${name}=${raw} is not a yes or a no and was ignored`], + }; + }, + ), + ); + /** * A number that is not a number is warned about and dropped, which is what the * specification asks for anywhere a value is unrecognized. Letting the read @@ -446,6 +483,17 @@ const resolveResource = Effect.gen(function* () { const UNREADABLE = "the OpenTelemetry environment could not be read"; +/** + * Whichever name switched export off is the one worth naming, because it is + * the one the reader has to go and unset. An ambient `OTEL_SDK_DISABLED` is + * the case where that is not obvious and where the answer is not to unset + * anything, so the message carries the override with it. + */ +const disabledBy = (name: string) => + name === "OTEL_SDK_DISABLED" + ? "OTEL_SDK_DISABLED is set, so no telemetry is exported, whatever configured it; set T3CODE_OTEL_SDK_DISABLED=false to export anyway" + : `${name} is set, so no telemetry is exported, whatever configured it`; + /** * Read the environment. Never fails: a variable T3 Code cannot honor * leaves the corresponding setting unset and is reported through the signal's @@ -453,7 +501,11 @@ const UNREADABLE = "the OpenTelemetry environment could not be read"; * to start. */ export const load: Effect.Effect = Effect.gen(function* () { - const disabled = yield* specBoolean("OTEL_SDK_DISABLED"); + const t3 = yield* t3Boolean("T3CODE_OTEL_SDK_DISABLED"); + const spec = yield* specBoolean("OTEL_SDK_DISABLED"); + // One setting, read the way every other setting here is read: T3 Code's own + // name answers it, and the standard name answers it only when ours is unset. + const disabled = t3.value ?? spec; const protocolDecision = yield* resolveProtocol; const resource = yield* resolveResource; const temporality = yield* resolveMetricsTemporality; @@ -472,6 +524,10 @@ export const load: Effect.Effect = Effect.gen(function* () { // bad value arrives here once per signal and would be logged that often. warnings: [ ...new Set([ + ...t3.warnings, + ...(disabled + ? [disabledBy(t3.value === true ? "T3CODE_OTEL_SDK_DISABLED" : "OTEL_SDK_DISABLED")] + : []), ...protocolDecision.warnings, ...resource.warnings, ...temporality.warnings,