diff --git a/.gitignore b/.gitignore index 373333a..7d82e60 100644 --- a/.gitignore +++ b/.gitignore @@ -9,3 +9,7 @@ rossoctl-cli # Editor / OS .DS_Store + +# Local MLflow backing store, created in the working directory by +# `mlflow server` — the tracing backend `rossoctl otel collect` forwards to. +/mlflow.db diff --git a/README.md b/README.md index 5a729ca..b3c54c9 100644 --- a/README.md +++ b/README.md @@ -68,6 +68,35 @@ forward proxy, plus `HTTPS_PROXY` and the CA trust variables bridge runs. Variables already set in your environment are left alone. Everything is shut down when the command exits or on SIGINT/SIGTERM. +`--with-claude-otel` additionally exports the variables that make Claude Code send +traces to the local collector — `CLAUDE_CODE_ENABLE_TELEMETRY=1`, +`CLAUDE_CODE_ENHANCED_TELEMETRY_BETA=1`, `OTEL_EXPORTER_OTLP_PROTOCOL=http/json`, +`OTEL_TRACES_EXPORT_INTERVAL=1`, `OTEL_TRACES_EXPORTER=otlp`, +`OTEL_LOGS_EXPORTER=none`, `OTEL_METRICS_EXPORTER=none`, and +`OTEL_EXPORTER_OTLP_ENDPOINT` built from `httpEndpoint` in +`~/.config/rossoctl/otel-config.yaml`: + +```sh +rossoctl otel collect # writes otel-config.yaml +rossoctl authbridge exec --with-claude-otel --config ./authbridge.yaml -- claude +``` + +It reads that record rather than assuming a port, so it fails — naming +`rossoctl otel collect` — when no collector has been started on this host, instead +of pointing the command at an endpoint that is not listening. + +`--otel-endpoint` overrides that address with a full URL, for a collector this host +did not start. The record is then not read at all, so it needs no local collector: + +```sh +rossoctl authbridge exec --with-claude-otel \ + --otel-endpoint https://otel.example.com:4318 --config ./authbridge.yaml -- claude +``` + +The value must begin with `http://` or `https://`, and it sets one of the variables +`--with-claude-otel` turns on — so passing it without that flag is an error rather +than an implied opt-in. + Authbridge's own log output goes to `--logfile` (default `/tmp/authbridge.log`) rather than stderr, so it does not interleave with the hosted command's output. The path is printed at startup; pass `--logfile ""` to log to stderr instead. @@ -273,9 +302,11 @@ rossoctl otel collect --traces_endpoint http://host.containers.internal:5002/v1/ # Both flags matter: MLflow otherwise binds loopback, which a container cannot # reach, and rejects requests whose Host header is host.containers.internal. # -# The generated config's path and the receiver's HTTP endpoint are recorded in -# ~/.config/rossoctl/otel-config.yaml. The container runs detached; stop it with -# `podman stop` (or `docker stop`) using the name printed on start. +# The generated config's path and the address a client uses to reach the receiver +# (httpEndpoint: 127.0.0.1:4318 — the dialable form, not the 0.0.0.0 the receiver +# binds inside the container) are recorded in ~/.config/rossoctl/otel-config.yaml. +# The container runs detached; stop it with `podman stop` (or `docker stop`) using +# the name printed on start. # Send one mock span to that collector, to check the path from here to MLflow # without an instrumented workload. Random trace and span IDs each run, so every diff --git a/cmd/authbridge_exec.go b/cmd/authbridge_exec.go index 5c794c0..2c82345 100644 --- a/cmd/authbridge_exec.go +++ b/cmd/authbridge_exec.go @@ -33,6 +33,7 @@ import ( "github.com/rossoctl/cortex/authbridge/authlib/tlsbridge" "github.com/rossoctl/rossoctl-cli/internal/instances" + "github.com/rossoctl/rossoctl-cli/internal/otelcollect" ) // execArgs holds the `authbridge exec` flags. @@ -44,6 +45,8 @@ var execArgs struct { instanceName string namespace string inboundProtocol string + withClaudeOtel bool + otelEndpoint string } // defaultSessionServer is where the session API listens when --sessionServer is @@ -167,6 +170,29 @@ runs. A variable already set in rossoctl's own environment is left alone. The reverse proxy adds nothing: it is where callers reach the command, not where the command sends its own traffic. +--with-claude-otel additionally exports the variables that make Claude Code send +traces to a local collector: + + CLAUDE_CODE_ENABLE_TELEMETRY=1 + CLAUDE_CODE_ENHANCED_TELEMETRY_BETA=1 + OTEL_EXPORTER_OTLP_PROTOCOL=http/json + OTEL_TRACES_EXPORT_INTERVAL=1 + OTEL_TRACES_EXPORTER=otlp + OTEL_LOGS_EXPORTER=none + OTEL_METRICS_EXPORTER=none + OTEL_EXPORTER_OTLP_ENDPOINT=http:// + +The endpoint is read from httpEndpoint in ~/.config/rossoctl/otel-config.yaml, +which "rossoctl otel collect" writes — so the flag fails, rather than exporting a +guess, when no collector has been started on this host. As with the proxy +variables, one already set in rossoctl's own environment is left alone. + +--otel-endpoint overrides that address with a full URL (http:// or https://), for a +collector this host did not start — one already running, or on another machine. The +record is then not read at all, so it works with no local collector. It sets one of +the variables --with-claude-otel turns on, so passing it alone is an error rather +than an implied --with-claude-otel. + With --proxyContainerImage the pipeline runs in a container from that image instead of in this process. The config is mounted read-only at /tmp/config.yaml and passed as --config; ports 8000, 8081, 9093, and 9094 are published on @@ -246,6 +272,13 @@ func runCortexExec(cmd *cobra.Command, args []string) error { return fmt.Errorf("--config is required (a local YAML file path or an http/https URL)") } + // Here rather than where the endpoint is resolved: a flag combination that + // cannot work should be rejected before a remote --config is fetched and the + // logfile is opened. + if err := validateClaudeOtelFlags(cmd); err != nil { + return err + } + // exec ignores the context, but an explicit --context naming one that does // not exist is still reported rather than silently accepted: it is almost // certainly a typo, and staying quiet would hide it. The lookup is read-only @@ -398,7 +431,142 @@ func execWithPipeline(cmd *cobra.Command, argv []string) error { } defer unregisterInstance(cmd, rec) - return runPassthrough(cmd, argv, host.env, host.serveErr) + childEnvironment := host.env + if execArgs.withClaudeOtel { + childEnvironment, err = withClaudeOtelEnv(cmd, childEnvironment) + if err != nil { + return err + } + } + + return runPassthrough(cmd, argv, childEnvironment, host.serveErr) +} + +// claudeOtelEndpoint resolves the OTEL_EXPORTER_OTLP_ENDPOINT for the child, and +// reports where the value came from for the --verbose line. +// +// --otel-endpoint wins, and when it is given the record is not read at all: naming +// an endpoint explicitly is how a collector that `otel collect` did not start — one +// on another host, or already running — is pointed at, and requiring a local record +// alongside it would defeat that. +func claudeOtelEndpoint() (endpointURL, source string, err error) { + if execArgs.otelEndpoint != "" { + return execArgs.otelEndpoint, "from --otel-endpoint", nil + } + + recordPath, err := otelCollectRecordPath() + if err != nil { + return "", "", err + } + rec, err := otelcollect.ReadRecord(recordPath) + if err != nil { + if os.IsNotExist(err) { + // The fix is one command away, so name it rather than reporting a bare + // missing file for a path the user never typed. --otel-endpoint is + // offered too, since it is the way through without a local collector. + return "", "", fmt.Errorf("--with-claude-otel needs a collector: %s does not exist.\n"+ + "Start one with `rossoctl otel collect`, or name one with --otel-endpoint", recordPath) + } + return "", "", err + } + url, err := rec.OTLPEndpointURL() + if err != nil { + return "", "", fmt.Errorf("--with-claude-otel: %s: %w", recordPath, err) + } + return url, "from " + recordPath, nil +} + +// validateClaudeOtelFlags rejects --otel-endpoint without --with-claude-otel, and a +// value that is not a full URL. +// +// Checked before anything is started, so a mistyped invocation fails immediately +// rather than after the pipeline is up and the child has been launched. +// +// --otel-endpoint alone is an error rather than an implied --with-claude-otel: it +// sets one variable out of the set the other flag turns on, so on its own it would +// name an endpoint for telemetry that is switched off — almost certainly not what +// was meant, and silently doing nothing is the worse of the two ways to handle it. +func validateClaudeOtelFlags(cmd *cobra.Command) error { + if !cmd.Flags().Changed("otel-endpoint") { + return nil + } + if !execArgs.withClaudeOtel { + return fmt.Errorf("--otel-endpoint requires --with-claude-otel") + } + + // A full URL, as OTEL_EXPORTER_OTLP_ENDPOINT itself expects. + // + // The scheme is checked by prefix *before* url.Parse, rather than reading + // u.Scheme afterwards, because the common mistake is a bare host:port — and + // url.Parse rejects that with "first path segment in URL cannot contain colon", + // which describes its own internals instead of telling the user to add http://. + // Testing the prefix first means the message names the fix. + endpoint := execArgs.otelEndpoint + if !strings.HasPrefix(endpoint, "http://") && !strings.HasPrefix(endpoint, "https://") { + return fmt.Errorf("--otel-endpoint %q must be a full URL beginning with http:// or https:// "+ + "(for example http://127.0.0.1:%d)", endpoint, otelcollect.HTTPPort) + } + u, err := url.Parse(endpoint) + if err != nil { + return fmt.Errorf("--otel-endpoint %q: %w", endpoint, err) + } + if u.Host == "" { + return fmt.Errorf("--otel-endpoint %q names no host", endpoint) + } + return nil +} + +// withClaudeOtelEnv adds the Claude Code telemetry variables to env, pointing at +// the collector recorded by `rossoctl otel collect`, or at --otel-endpoint when +// that is given. +// +// env is the environment the host built, which may be nil to mean "inherit +// rossoctl's" (see exec.Cmd.Env). A nil is expanded to the real environment first: +// appending to it would otherwise hand the child *only* these variables, with no +// PATH or HOME. +// +// An inherited variable is left alone and reported, matching how childEnv treats +// the proxy and CA settings — someone who exported OTEL_EXPORTER_OTLP_ENDPOINT +// before running this has said where they want spans to go. +func withClaudeOtelEnv(cmd *cobra.Command, env []string) ([]string, error) { + errOut := cmd.ErrOrStderr() + + endpointURL, source, err := claudeOtelEndpoint() + if err != nil { + return nil, err + } + + if env == nil { + env = os.Environ() + } + + // Index what is already present by name, so an inherited setting can be + // detected without depending on how the slice was ordered. + present := make(map[string]string, len(env)) + for _, kv := range env { + if k, v, ok := strings.Cut(kv, "="); ok { + present[k] = v + } + } + + for _, kv := range otelcollect.ClaudeTelemetryEnv(endpointURL) { + k, v, ok := strings.Cut(kv, "=") + if !ok { + continue + } + if existing, taken := present[k]; taken { + if existing != v { + fmt.Fprintf(errOut, "keeping inherited %s=%s (not overriding with %s)\n", k, existing, v) + } + continue + } + env = append(env, kv) + } + + if verbose { + fmt.Fprintf(errOut, "Claude Code telemetry -> %s (%s)\n", endpointURL, source) + } + return env, nil } // instanceName returns the name to record for this run: --instanceName when @@ -1554,6 +1722,10 @@ func init() { "namespace to record this instance in; defaults to the current context's namespace, or \""+instances.DefaultNamespace+"\"") f.StringVar(&execArgs.inboundProtocol, "inboundProtocol", string(instances.DefaultProtocol), `protocol the inbound listener fronts, recorded in the instance file: "a2a" or "mcp"`) + f.BoolVar(&execArgs.withClaudeOtel, "with-claude-otel", false, + "give the command Claude Code telemetry variables pointing at the collector recorded by `rossoctl otel collect`") + f.StringVar(&execArgs.otelEndpoint, "otel-endpoint", "", + "full URL for OTEL_EXPORTER_OTLP_ENDPOINT, replacing the recorded collector address (requires --with-claude-otel)") // The authbridge group deliberately has no --cortex flag. exec is configured // entirely by --config and never resolves a context, so the flag it used to diff --git a/cmd/authbridge_exec_otel_test.go b/cmd/authbridge_exec_otel_test.go new file mode 100644 index 0000000..ec3ccb9 --- /dev/null +++ b/cmd/authbridge_exec_otel_test.go @@ -0,0 +1,420 @@ +package cmd + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/rossoctl/rossoctl-cli/internal/otelcollect" +) + +// writeOtelRecord writes an otel-config.yaml under the isolated home, as +// `otel collect` would, and returns its path. +func writeOtelRecord(t *testing.T, httpEndpoint string) string { + t.Helper() + path, err := otelCollectRecordPath() + if err != nil { + t.Fatalf("otelCollectRecordPath: %v", err) + } + if err := otelcollect.WriteRecord(path, otelcollect.Record{ + ConfigFile: filepath.Join(filepath.Dir(path), "otel", "collector-test.yaml"), + HTTPEndpoint: httpEndpoint, + }); err != nil { + t.Fatalf("WriteRecord: %v", err) + } + return path +} + +// childEnvFor runs `authbridge exec --with-claude-otel` with a command that prints +// its environment, and returns that environment as a name->value map. +// +// The child is `env`, so what is asserted is the environment the command actually +// received rather than an intermediate slice. +func childEnvFor(t *testing.T, extraArgs ...string) map[string]string { + t.Helper() + cfg := writeConfig(t, pipelineOnlyConfig(t)) + + args := append([]string{"authbridge", "exec", "--config", cfg}, extraArgs...) + args = append(args, "--", "env") + + out, code := execExitCode(t, args...) + if code != 0 { + t.Fatalf("exit code = %d, want 0\n%s", code, out) + } + + env := map[string]string{} + for line := range strings.SplitSeq(out, "\n") { + if k, v, ok := strings.Cut(strings.TrimRight(line, "\r"), "="); ok { + env[k] = v + } + } + return env +} + +// TestExecWithClaudeOtelSetsEnv verifies the command receives every documented +// variable, with the endpoint built from the record's httpEndpoint. +func TestExecWithClaudeOtelSetsEnv(t *testing.T) { + isolateHome(t) + writeOtelRecord(t, "127.0.0.1:4318") + + env := childEnvFor(t, "--with-claude-otel") + + for k, want := range map[string]string{ + "CLAUDE_CODE_ENABLE_TELEMETRY": "1", + "CLAUDE_CODE_ENHANCED_TELEMETRY_BETA": "1", + "OTEL_EXPORTER_OTLP_PROTOCOL": "http/json", + "OTEL_TRACES_EXPORT_INTERVAL": "1", + "OTEL_TRACES_EXPORTER": "otlp", + "OTEL_LOGS_EXPORTER": "none", + "OTEL_METRICS_EXPORTER": "none", + "OTEL_EXPORTER_OTLP_ENDPOINT": "http://127.0.0.1:4318", + } { + if got, ok := env[k]; !ok { + t.Errorf("%s was not set on the command", k) + } else if got != want { + t.Errorf("%s = %q, want %q", k, got, want) + } + } +} + +// TestExecWithClaudeOtelKeepsInheritedEnvironment verifies the child still gets the +// ordinary environment. +// +// The substance: the host used here runs no forward proxy, so host.env is nil, which +// exec.Cmd reads as "inherit". Appending to that nil without expanding it first would +// hand the child *only* the telemetry variables — no PATH, no HOME. +func TestExecWithClaudeOtelKeepsInheritedEnvironment(t *testing.T) { + isolateHome(t) + writeOtelRecord(t, "127.0.0.1:4318") + t.Setenv("ROSSOCTL_OTEL_CANARY", "still-here") + + env := childEnvFor(t, "--with-claude-otel") + + if env["ROSSOCTL_OTEL_CANARY"] != "still-here" { + t.Errorf("an inherited variable was lost: ROSSOCTL_OTEL_CANARY = %q", env["ROSSOCTL_OTEL_CANARY"]) + } + if env["PATH"] == "" { + t.Error("PATH was lost; the inherited environment must be preserved") + } +} + +// TestExecWithClaudeOtelUsesRecordedEndpoint verifies the endpoint tracks the record +// rather than being hardcoded. +func TestExecWithClaudeOtelUsesRecordedEndpoint(t *testing.T) { + isolateHome(t) + writeOtelRecord(t, "127.0.0.1:14318") + + env := childEnvFor(t, "--with-claude-otel") + + if got, want := env["OTEL_EXPORTER_OTLP_ENDPOINT"], "http://127.0.0.1:14318"; got != want { + t.Errorf("OTEL_EXPORTER_OTLP_ENDPOINT = %q, want %q from the record", got, want) + } +} + +// TestExecWithClaudeOtelRewritesWildcardEndpoint verifies a record carrying the +// receiver's wildcard bind address still yields a dialable URL. +// +// Covers a record written before that was corrected, or edited by hand: an exporter +// told to send to http://0.0.0.0:4318 relies on the platform reinterpreting it. +func TestExecWithClaudeOtelRewritesWildcardEndpoint(t *testing.T) { + isolateHome(t) + writeOtelRecord(t, "0.0.0.0:4318") + + env := childEnvFor(t, "--with-claude-otel") + + if got, want := env["OTEL_EXPORTER_OTLP_ENDPOINT"], "http://127.0.0.1:4318"; got != want { + t.Errorf("OTEL_EXPORTER_OTLP_ENDPOINT = %q, want the rewritten %q", got, want) + } +} + +// TestExecWithoutClaudeOtelSetsNothing verifies the variables appear only when the +// flag is given. +func TestExecWithoutClaudeOtelSetsNothing(t *testing.T) { + isolateHome(t) + writeOtelRecord(t, "127.0.0.1:4318") + + env := childEnvFor(t) + + for _, k := range []string{ + "CLAUDE_CODE_ENABLE_TELEMETRY", + "CLAUDE_CODE_ENHANCED_TELEMETRY_BETA", + "OTEL_EXPORTER_OTLP_ENDPOINT", + "OTEL_TRACES_EXPORTER", + } { + if v, ok := env[k]; ok { + t.Errorf("%s = %q was set without --with-claude-otel", k, v) + } + } +} + +// TestExecWithClaudeOtelKeepsInheritedOtelVars verifies an already-exported telemetry +// variable is left alone and reported, as the proxy and CA variables are. +// +// Someone who exported OTEL_EXPORTER_OTLP_ENDPOINT before running this has said where +// they want spans to go. +func TestExecWithClaudeOtelKeepsInheritedOtelVars(t *testing.T) { + isolateHome(t) + writeOtelRecord(t, "127.0.0.1:4318") + t.Setenv("OTEL_EXPORTER_OTLP_ENDPOINT", "http://collector.example.com:4318") + t.Setenv("OTEL_METRICS_EXPORTER", "otlp") + + cfg := writeConfig(t, pipelineOnlyConfig(t)) + out, code := execExitCode(t, "authbridge", "exec", "--config", cfg, + "--with-claude-otel", "--", "env") + if code != 0 { + t.Fatalf("exit code = %d, want 0\n%s", code, out) + } + + env := map[string]string{} + for line := range strings.SplitSeq(out, "\n") { + if k, v, ok := strings.Cut(strings.TrimRight(line, "\r"), "="); ok { + env[k] = v + } + } + + if got := env["OTEL_EXPORTER_OTLP_ENDPOINT"]; got != "http://collector.example.com:4318" { + t.Errorf("OTEL_EXPORTER_OTLP_ENDPOINT = %q, want the inherited value kept", got) + } + if got := env["OTEL_METRICS_EXPORTER"]; got != "otlp" { + t.Errorf("OTEL_METRICS_EXPORTER = %q, want the inherited value kept", got) + } + // The override is announced, so a user is not left wondering why spans went + // somewhere else. + if !strings.Contains(out, "keeping inherited OTEL_EXPORTER_OTLP_ENDPOINT") { + t.Errorf("the kept variable should be reported:\n%s", out) + } + // A variable the environment did not already set is still applied. + if got := env["OTEL_TRACES_EXPORTER"]; got != "otlp" { + t.Errorf("OTEL_TRACES_EXPORTER = %q, want otlp", got) + } +} + +// TestExecWithClaudeOtelRequiresRecord verifies the flag fails, naming the command +// that creates the record, when no collector has been started. +// +// Failing rather than exporting a guessed endpoint is the point: a child pointed at a +// collector that is not there buffers and drops spans silently. +func TestExecWithClaudeOtelRequiresRecord(t *testing.T) { + isolateHome(t) + cfg := writeConfig(t, pipelineOnlyConfig(t)) + + out, err := execute(t, "authbridge", "exec", "--config", cfg, + "--with-claude-otel", "--", "true") + if err == nil { + t.Fatalf("expected an error when no otel record exists\n%s", out) + } + if !strings.Contains(err.Error(), "otel collect") { + t.Errorf("error should name the command that starts a collector: %v", err) + } + if !strings.Contains(err.Error(), otelcollect.RecordName) { + t.Errorf("error should name the missing record file: %v", err) + } +} + +// TestExecWithClaudeOtelRejectsEmptyRecordEndpoint verifies a record with no +// httpEndpoint fails rather than exporting "http://". +func TestExecWithClaudeOtelRejectsEmptyRecordEndpoint(t *testing.T) { + isolateHome(t) + recordPath, err := otelCollectRecordPath() + if err != nil { + t.Fatalf("otelCollectRecordPath: %v", err) + } + if err := os.MkdirAll(filepath.Dir(recordPath), 0o700); err != nil { + t.Fatalf("mkdir: %v", err) + } + if err := os.WriteFile(recordPath, []byte("configFile: /tmp/c.yaml\n"), 0o600); err != nil { + t.Fatalf("writing fixture: %v", err) + } + + cfg := writeConfig(t, pipelineOnlyConfig(t)) + out, execErr := execute(t, "authbridge", "exec", "--config", cfg, + "--with-claude-otel", "--", "true") + if execErr == nil { + t.Fatalf("expected an error for a record with no httpEndpoint\n%s", out) + } + if !strings.Contains(execErr.Error(), "httpEndpoint") { + t.Errorf("error should name the missing field: %v", execErr) + } +} + +// TestExecWithClaudeOtelFlagSurface verifies the flag is registered as a bool and +// documented. +func TestExecWithClaudeOtelFlagSurface(t *testing.T) { + f := authbridgeExecCmd.Flags().Lookup("with-claude-otel") + if f == nil { + t.Fatal("authbridge exec has no --with-claude-otel flag") + } + if f.Value.Type() != "bool" { + t.Errorf("--with-claude-otel is a %s, want bool", f.Value.Type()) + } + if f.DefValue != "false" { + t.Errorf("--with-claude-otel default = %q, want false", f.DefValue) + } + + out, err := execute(t, "authbridge", "exec", "--help") + if err != nil { + t.Fatalf("exec --help: %v", err) + } + // The help has to name the variables, since they are the reason to use the flag. + for _, want := range []string{ + "--with-claude-otel", + "CLAUDE_CODE_ENABLE_TELEMETRY", + "OTEL_EXPORTER_OTLP_ENDPOINT", + otelcollect.RecordName, + } { + if !strings.Contains(out, want) { + t.Errorf("exec --help does not mention %q", want) + } + } +} + +// TestExecOtelEndpointOverridesRecord verifies --otel-endpoint replaces the endpoint +// the record names, leaving the other telemetry variables alone. +func TestExecOtelEndpointOverridesRecord(t *testing.T) { + isolateHome(t) + writeOtelRecord(t, "127.0.0.1:4318") + + env := childEnvFor(t, "--with-claude-otel", "--otel-endpoint", "http://collector.example.com:4318") + + if got, want := env["OTEL_EXPORTER_OTLP_ENDPOINT"], "http://collector.example.com:4318"; got != want { + t.Errorf("OTEL_EXPORTER_OTLP_ENDPOINT = %q, want %q from --otel-endpoint", got, want) + } + // The rest of the set is unaffected: the flag names an endpoint, not a policy. + if got := env["OTEL_TRACES_EXPORTER"]; got != "otlp" { + t.Errorf("OTEL_TRACES_EXPORTER = %q, want otlp", got) + } + if got := env["CLAUDE_CODE_ENABLE_TELEMETRY"]; got != "1" { + t.Errorf("CLAUDE_CODE_ENABLE_TELEMETRY = %q, want 1", got) + } +} + +// TestExecOtelEndpointWithoutRecord verifies the override needs no local collector. +// +// This is the case it exists for: pointing at a collector this host did not start, +// where insisting on a record would defeat the flag. +func TestExecOtelEndpointWithoutRecord(t *testing.T) { + isolateHome(t) + // Deliberately no writeOtelRecord. + + env := childEnvFor(t, "--with-claude-otel", "--otel-endpoint", "https://otel.example.com") + + if got, want := env["OTEL_EXPORTER_OTLP_ENDPOINT"], "https://otel.example.com"; got != want { + t.Errorf("OTEL_EXPORTER_OTLP_ENDPOINT = %q, want %q", got, want) + } +} + +// TestExecOtelEndpointIsSentVerbatim verifies the value is exported exactly as typed. +// +// No scheme rewriting, no trailing-slash normalization, and above all no path +// appended: OTEL_EXPORTER_OTLP_ENDPOINT is a base URL the SDK adds the signal path +// to, so anything added here would be sent to /v1/traces/v1/traces. +func TestExecOtelEndpointIsSentVerbatim(t *testing.T) { + isolateHome(t) + + for _, endpoint := range []string{ + "http://127.0.0.1:14318", + "https://otel.example.com:443", + "http://otel.internal", + // A path is a legitimate thing to name — a collector behind a prefix — and + // must survive as given. + "http://gateway.example.com/otlp", + } { + t.Run(endpoint, func(t *testing.T) { + env := childEnvFor(t, "--with-claude-otel", "--otel-endpoint", endpoint) + if got := env["OTEL_EXPORTER_OTLP_ENDPOINT"]; got != endpoint { + t.Errorf("OTEL_EXPORTER_OTLP_ENDPOINT = %q, want %q verbatim", got, endpoint) + } + }) + } +} + +// TestExecOtelEndpointRequiresWithClaudeOtel verifies the flag is rejected on its own. +func TestExecOtelEndpointRequiresWithClaudeOtel(t *testing.T) { + isolateHome(t) + writeOtelRecord(t, "127.0.0.1:4318") + cfg := writeConfig(t, pipelineOnlyConfig(t)) + + out, err := execute(t, "authbridge", "exec", "--config", cfg, + "--otel-endpoint", "http://127.0.0.1:4318", "--", "true") + if err == nil { + t.Fatalf("expected an error for --otel-endpoint without --with-claude-otel\n%s", out) + } + if !strings.Contains(err.Error(), "--with-claude-otel") { + t.Errorf("error should name the flag it requires: %v", err) + } +} + +// TestExecOtelEndpointRejectsNonURL verifies a value that is not a full URL fails. +// +// A bare host:port is the one worth pinning: url.Parse accepts it as a URL whose +// scheme is the host, so without the scheme check it would be exported unusable. +func TestExecOtelEndpointRejectsNonURL(t *testing.T) { + isolateHome(t) + cfg := writeConfig(t, pipelineOnlyConfig(t)) + + for _, tc := range []struct { + name string + endpoint string + }{ + {"bare host and port", "127.0.0.1:4318"}, + {"host only", "collector.example.com"}, + {"no scheme, leading slashes", "//collector.example.com:4318"}, + {"unsupported scheme", "grpc://127.0.0.1:4317"}, + {"scheme but no host", "http://"}, + } { + t.Run(tc.name, func(t *testing.T) { + out, err := execute(t, "authbridge", "exec", "--config", cfg, + "--with-claude-otel", "--otel-endpoint", tc.endpoint, "--", "true") + if err == nil { + t.Fatalf("expected an error for --otel-endpoint %q\n%s", tc.endpoint, out) + } + if !strings.Contains(err.Error(), "otel-endpoint") { + t.Errorf("error should name the flag: %v", err) + } + }) + } +} + +// TestExecOtelEndpointEmptyValueIsRejected verifies an explicitly empty value is an +// error rather than silently falling back to the record. +// +// `--otel-endpoint ""` is a stated intention that cannot be honored; treating it as +// "unset" would quietly point the child somewhere the user did not name. +func TestExecOtelEndpointEmptyValueIsRejected(t *testing.T) { + isolateHome(t) + writeOtelRecord(t, "127.0.0.1:4318") + cfg := writeConfig(t, pipelineOnlyConfig(t)) + + out, err := execute(t, "authbridge", "exec", "--config", cfg, + "--with-claude-otel", "--otel-endpoint", "", "--", "true") + if err == nil { + t.Fatalf("expected an error for an empty --otel-endpoint\n%s", out) + } + if !strings.Contains(err.Error(), "otel-endpoint") { + t.Errorf("error should name the flag: %v", err) + } +} + +// TestExecOtelEndpointFlagSurface verifies the flag's registration and that the help +// documents it. +func TestExecOtelEndpointFlagSurface(t *testing.T) { + f := authbridgeExecCmd.Flags().Lookup("otel-endpoint") + if f == nil { + t.Fatal("authbridge exec has no --otel-endpoint flag") + } + if f.Value.Type() != "string" { + t.Errorf("--otel-endpoint is a %s, want string", f.Value.Type()) + } + if f.DefValue != "" { + t.Errorf("--otel-endpoint default = %q, want empty", f.DefValue) + } + + out, err := execute(t, "authbridge", "exec", "--help") + if err != nil { + t.Fatalf("exec --help: %v", err) + } + if !strings.Contains(out, "--otel-endpoint") { + t.Errorf("exec --help does not document --otel-endpoint:\n%s", out) + } +} diff --git a/cmd/otel_collect_test.go b/cmd/otel_collect_test.go index 9e04ba2..b7ceac6 100644 --- a/cmd/otel_collect_test.go +++ b/cmd/otel_collect_test.go @@ -154,8 +154,11 @@ func TestOtelCollectRunsContainer(t *testing.T) { if rec.ConfigFile != cfgPath { t.Errorf("record configFile = %q, want %q", rec.ConfigFile, cfgPath) } - if rec.HTTPEndpoint != "0.0.0.0:"+strconv.Itoa(otelcollect.HTTPPort) { - t.Errorf("record httpEndpoint = %q, want the receiver's HTTP endpoint", rec.HTTPEndpoint) + // The dialable address, not the wildcard the receiver binds: this value is read + // back and turned into a URL (an OTEL_EXPORTER_OTLP_ENDPOINT), and + // http://0.0.0.0:4318 is not a destination. + if rec.HTTPEndpoint != "127.0.0.1:"+strconv.Itoa(otelcollect.HTTPPort) { + t.Errorf("record httpEndpoint = %q, want the loopback address a client dials", rec.HTTPEndpoint) } // The stop instruction has to name the container, since the run is detached diff --git a/internal/otelcollect/otelcollect.go b/internal/otelcollect/otelcollect.go index 8fbcb8f..014fa99 100644 --- a/internal/otelcollect/otelcollect.go +++ b/internal/otelcollect/otelcollect.go @@ -24,6 +24,7 @@ import ( "net/url" "os" "path/filepath" + "slices" "strconv" "strings" "time" @@ -259,10 +260,51 @@ func NewConfig(tracesEndpoint string) *Config { } } -// HTTPEndpoint returns the config's receivers.otlp.protocols.http.endpoint, which -// is the value the record file carries. +// HTTPEndpoint returns the address a client on this host uses to reach the +// collector's OTLP HTTP receiver — the record file's httpEndpoint, and the basis of +// the OTEL_EXPORTER_OTLP_ENDPOINT given to a child by `authbridge exec +// --with-claude-otel`. +// +// The receiver's own endpoint is a *bind* address, 0.0.0.0 inside the container, +// which is a wildcard rather than a destination: an exporter told to send to +// http://0.0.0.0:4318 is relying on the platform to reinterpret it, which Linux +// does and other systems need not. The published side is on loopback, so +// unspecifiedHost is rewritten to 127.0.0.1 while the port is carried through from +// the receiver. +// +// A host that is already specific is left alone, so a config edited to bind one +// interface still reports the address that was chosen. func (c *Config) HTTPEndpoint() string { - return c.Receivers.OTLP.Protocols.HTTP.Endpoint + return dialableHost(c.Receivers.OTLP.Protocols.HTTP.Endpoint) +} + +// unspecifiedHost is the IPv4 wildcard bind address, and unspecifiedHostV6 its +// IPv6 counterpart. Neither is a usable destination. +const ( + unspecifiedHost = "0.0.0.0" + unspecifiedHostV6 = "::" +) + +// loopbackHost is what an unspecified bind address is reported as: the collector +// publishes its ports on this host, so loopback is where a local client reaches it. +const loopbackHost = "127.0.0.1" + +// dialableHost rewrites a wildcard host in a "host:port" address to loopback, +// leaving the port and any specific host untouched. +// +// An address it cannot split is returned unchanged rather than guessed at: this +// feeds a record file and an environment variable, and passing a malformed value +// through unaltered keeps the eventual error about the value the user can see. +func dialableHost(endpoint string) string { + host, port, err := net.SplitHostPort(endpoint) + if err != nil { + return endpoint + } + switch host { + case unspecifiedHost, unspecifiedHostV6, "": + return net.JoinHostPort(loopbackHost, port) + } + return endpoint } // Marshal renders the config as YAML. @@ -369,11 +411,53 @@ type Record struct { // ConfigFile is the generated collector config on this host. ConfigFile string `yaml:"configFile"` - // HTTPEndpoint is the config's receivers.otlp.protocols.http.endpoint, as - // bound inside the container. + // HTTPEndpoint is the "host:port" a client on this host uses to reach the + // collector's OTLP HTTP receiver, e.g. 127.0.0.1:4318. + // + // Deliberately the dialable form rather than the receiver's own bind address: + // this value is read back and turned into a URL — an + // OTEL_EXPORTER_OTLP_ENDPOINT for a child process — and the wildcard a + // receiver binds is not a destination. See Config.HTTPEndpoint. HTTPEndpoint string `yaml:"httpEndpoint"` } +// ReadRecord reads the record written by `otel collect` at path. +// +// A missing file is reported as-is rather than as an empty Record: the only caller +// needs the endpoint, and "no collector has been started here" is what it has to +// tell the user, not a zero value that would become a nonsense URL. +func ReadRecord(path string) (*Record, error) { + data, err := os.ReadFile(path) + if err != nil { + return nil, err + } + var rec Record + if err := yaml.Unmarshal(data, &rec); err != nil { + return nil, fmt.Errorf("parsing %s: %w", path, err) + } + return &rec, nil +} + +// OTLPEndpointURL renders the record's HTTPEndpoint as the base URL an OTLP +// exporter is configured with. +// +// Just the scheme and authority: OTEL_EXPORTER_OTLP_ENDPOINT is a *base*, onto which +// an SDK appends the signal path (/v1/traces), so including a path here would send +// spans to /v1/traces/v1/traces. +// +// Reports an error when the record carries no endpoint, which is what a truncated or +// hand-edited file looks like. +func (r *Record) OTLPEndpointURL() (string, error) { + endpoint := strings.TrimSpace(r.HTTPEndpoint) + if endpoint == "" { + return "", fmt.Errorf("no httpEndpoint recorded") + } + // Rewritten again on read, not only on write: a record written before the + // wildcard was corrected, or edited by hand, would otherwise hand a child an + // OTEL_EXPORTER_OTLP_ENDPOINT of http://0.0.0.0:4318. + return "http://" + dialableHost(endpoint), nil +} + // WriteRecord writes rec as YAML to path, creating the parent directory. func WriteRecord(path string, rec Record) error { data, err := yaml.Marshal(rec) @@ -391,6 +475,44 @@ func WriteRecord(path string, rec Record) error { return nil } +// EndpointEnvVar is the variable an OTLP exporter reads for the collector's base +// URL. Named because it is the one entry of ClaudeTelemetryEnv whose value is not +// static. +const EndpointEnvVar = "OTEL_EXPORTER_OTLP_ENDPOINT" + +// ClaudeTelemetryEnv returns the environment that makes Claude Code export traces +// to the collector at endpointURL, as "K=V" strings sorted by name. +// +// Everything but the endpoint is fixed. The two CLAUDE_CODE_* variables turn +// Claude's telemetry on; the OTEL_* ones select the transport and restrict it to +// traces. +// +// http/json rather than the OTLP default of http/protobuf because the receiver this +// points at is the one `otel collect` publishes, whose JSON path is what the rest of +// this package exercises. Logs and metrics are switched off explicitly: the +// generated collector config declares a traces pipeline only, so an SDK exporting +// them would retry against a receiver that discards them. +// +// Sorted so the child's environment is byte-identical between runs, which keeps a +// diff of two invocations to what actually differs. +func ClaudeTelemetryEnv(endpointURL string) []string { + env := []string{ + "CLAUDE_CODE_ENABLE_TELEMETRY=1", + "CLAUDE_CODE_ENHANCED_TELEMETRY_BETA=1", + "OTEL_EXPORTER_OTLP_PROTOCOL=http/json", + // Export every second rather than on the SDK's minute-scale default, so a + // span shows up in MLflow while the command that produced it is still on + // screen. Unitless: Claude Code reads this as seconds. + "OTEL_TRACES_EXPORT_INTERVAL=1", + "OTEL_TRACES_EXPORTER=otlp", + "OTEL_LOGS_EXPORTER=none", + "OTEL_METRICS_EXPORTER=none", + EndpointEnvVar + "=" + endpointURL, + } + slices.Sort(env) + return env +} + // EndpointPort returns the port named by a traces endpoint URL, supplying the // scheme's default when the URL has none. // diff --git a/internal/otelcollect/otelcollect_test.go b/internal/otelcollect/otelcollect_test.go index 4a34ab0..501e3fb 100644 --- a/internal/otelcollect/otelcollect_test.go +++ b/internal/otelcollect/otelcollect_test.go @@ -9,6 +9,7 @@ import ( "net/http/httptest" "os" "path/filepath" + "slices" "strconv" "strings" "testing" @@ -144,11 +145,51 @@ func TestNewConfigServicePipeline(t *testing.T) { } } -// TestHTTPEndpoint verifies the accessor reports the receiver endpoint the record -// file carries, rather than the gRPC one beside it. +// TestHTTPEndpoint verifies the accessor reports an address a client can dial, +// rather than the wildcard the receiver binds. +// +// The rewrite is the point. receivers.otlp.protocols.http.endpoint is 0.0.0.0:4318 +// — correct as a bind address inside the container, and useless as a destination: +// this value becomes an OTEL_EXPORTER_OTLP_ENDPOINT, and an exporter told to send to +// http://0.0.0.0:4318 depends on the platform reinterpreting it. The port must +// still come from the receiver rather than being restated here. func TestHTTPEndpoint(t *testing.T) { - if got := NewConfig(DefaultTracesEndpoint).HTTPEndpoint(); got != "0.0.0.0:4318" { - t.Errorf("HTTPEndpoint() = %q, want 0.0.0.0:4318", got) + cfg := NewConfig(DefaultTracesEndpoint) + if got, want := cfg.HTTPEndpoint(), "127.0.0.1:"+strconv.Itoa(HTTPPort); got != want { + t.Errorf("HTTPEndpoint() = %q, want %q", got, want) + } + // The config itself keeps binding the wildcard: a container's loopback is + // reachable only from inside it, so a receiver bound there would refuse every + // connection arriving through the published port. + if got := cfg.Receivers.OTLP.Protocols.HTTP.Endpoint; got != "0.0.0.0:"+strconv.Itoa(HTTPPort) { + t.Errorf("receiver endpoint = %q, want the wildcard bind address unchanged", got) + } +} + +// TestDialableHost verifies which hosts are rewritten and which are preserved. +func TestDialableHost(t *testing.T) { + for _, tc := range []struct { + name string + endpoint string + want string + }{ + {"ipv4 wildcard", "0.0.0.0:4318", "127.0.0.1:4318"}, + {"ipv6 wildcard", "[::]:4318", "127.0.0.1:4318"}, + {"host omitted", ":4318", "127.0.0.1:4318"}, + // A config edited to bind one interface still reports what was chosen. + {"specific host", "192.168.1.5:4318", "192.168.1.5:4318"}, + {"loopback already", "127.0.0.1:4318", "127.0.0.1:4318"}, + {"named host", "localhost:4318", "localhost:4318"}, + // Not splittable, so returned unchanged rather than guessed at: the eventual + // error should name the value the user can see. + {"no port", "0.0.0.0", "0.0.0.0"}, + {"empty", "", ""}, + } { + t.Run(tc.name, func(t *testing.T) { + if got := dialableHost(tc.endpoint); got != tc.want { + t.Errorf("dialableHost(%q) = %q, want %q", tc.endpoint, got, tc.want) + } + }) } } @@ -369,7 +410,7 @@ func TestWriteRecord(t *testing.T) { path := filepath.Join(t.TempDir(), "nested", RecordName) rec := Record{ ConfigFile: "/home/u/.config/rossoctl/otel/collector-20260817-143045.yaml", - HTTPEndpoint: "0.0.0.0:4318", + HTTPEndpoint: "127.0.0.1:4318", } if err := WriteRecord(path, rec); err != nil { t.Fatalf("WriteRecord: %v", err) @@ -769,3 +810,146 @@ func TestSendTraceUnreachable(t *testing.T) { t.Errorf("error = %v, want it to name %q", err, url) } } + +// TestReadRecordRoundTrip verifies a written record reads back unchanged. +func TestReadRecordRoundTrip(t *testing.T) { + path := filepath.Join(t.TempDir(), RecordName) + want := Record{ConfigFile: "/home/u/.config/rossoctl/otel/c.yaml", HTTPEndpoint: "127.0.0.1:4318"} + if err := WriteRecord(path, want); err != nil { + t.Fatalf("WriteRecord: %v", err) + } + + got, err := ReadRecord(path) + if err != nil { + t.Fatalf("ReadRecord: %v", err) + } + if *got != want { + t.Errorf("ReadRecord = %+v, want %+v", *got, want) + } +} + +// TestReadRecordMissing verifies a missing file is reported as such, so a caller can +// tell "no collector started here" from a parse failure. +func TestReadRecordMissing(t *testing.T) { + _, err := ReadRecord(filepath.Join(t.TempDir(), "absent.yaml")) + if err == nil { + t.Fatal("expected an error for a missing record") + } + if !os.IsNotExist(err) { + t.Errorf("error = %v, want one that satisfies os.IsNotExist", err) + } +} + +// TestReadRecordMalformed verifies unparseable YAML is an error naming the file. +func TestReadRecordMalformed(t *testing.T) { + path := filepath.Join(t.TempDir(), RecordName) + if err := os.WriteFile(path, []byte("httpEndpoint: [unclosed\n"), 0o600); err != nil { + t.Fatalf("writing fixture: %v", err) + } + _, err := ReadRecord(path) + if err == nil { + t.Fatal("expected an error for malformed YAML") + } + if !strings.Contains(err.Error(), path) { + t.Errorf("error = %v, want it to name %q", err, path) + } +} + +// TestOTLPEndpointURL verifies the URL handed to an exporter. +// +// The no-path assertion is the substance: OTEL_EXPORTER_OTLP_ENDPOINT is a base URL +// onto which an SDK appends the signal path, so a "/v1/traces" here would send spans +// to /v1/traces/v1/traces. +func TestOTLPEndpointURL(t *testing.T) { + for _, tc := range []struct { + name string + endpoint string + want string + }{ + {"loopback", "127.0.0.1:4318", "http://127.0.0.1:4318"}, + // A record written before the wildcard was corrected, or edited by hand, is + // rewritten on read rather than passed through as a useless destination. + {"wildcard is rewritten", "0.0.0.0:4318", "http://127.0.0.1:4318"}, + {"ipv6 wildcard is rewritten", "[::]:4318", "http://127.0.0.1:4318"}, + {"specific host preserved", "192.168.1.5:4318", "http://192.168.1.5:4318"}, + {"surrounding whitespace", " 127.0.0.1:4318\n", "http://127.0.0.1:4318"}, + } { + t.Run(tc.name, func(t *testing.T) { + got, err := (&Record{HTTPEndpoint: tc.endpoint}).OTLPEndpointURL() + if err != nil { + t.Fatalf("OTLPEndpointURL: %v", err) + } + if got != tc.want { + t.Errorf("OTLPEndpointURL() = %q, want %q", got, tc.want) + } + if strings.Contains(strings.TrimPrefix(got, "http://"), "/") { + t.Errorf("OTLPEndpointURL() = %q, want no path: an SDK appends the signal path", got) + } + }) + } +} + +// TestOTLPEndpointURLEmpty verifies a record with no endpoint is an error rather than +// yielding the nonsense URL "http://". +func TestOTLPEndpointURLEmpty(t *testing.T) { + for _, endpoint := range []string{"", " "} { + if _, err := (&Record{HTTPEndpoint: endpoint}).OTLPEndpointURL(); err == nil { + t.Errorf("OTLPEndpointURL() with %q succeeded; want an error", endpoint) + } + } +} + +// TestClaudeTelemetryEnv verifies the exact set of variables, including the endpoint. +func TestClaudeTelemetryEnv(t *testing.T) { + got := ClaudeTelemetryEnv("http://127.0.0.1:4318") + + want := []string{ + "CLAUDE_CODE_ENABLE_TELEMETRY=1", + "CLAUDE_CODE_ENHANCED_TELEMETRY_BETA=1", + "OTEL_EXPORTER_OTLP_ENDPOINT=http://127.0.0.1:4318", + "OTEL_EXPORTER_OTLP_PROTOCOL=http/json", + "OTEL_LOGS_EXPORTER=none", + "OTEL_METRICS_EXPORTER=none", + "OTEL_TRACES_EXPORTER=otlp", + "OTEL_TRACES_EXPORT_INTERVAL=1", + } + if len(got) != len(want) { + t.Fatalf("got %d variables, want %d:\n%v", len(got), len(want), got) + } + for i := range want { + if got[i] != want[i] { + t.Errorf("variable %d = %q, want %q", i, got[i], want[i]) + } + } +} + +// TestClaudeTelemetryEnvIsSorted verifies the output is sorted, so a child's +// environment is byte-identical between runs. +func TestClaudeTelemetryEnvIsSorted(t *testing.T) { + got := ClaudeTelemetryEnv("http://127.0.0.1:4318") + if !slices.IsSorted(got) { + t.Errorf("ClaudeTelemetryEnv is not sorted: %v", got) + } +} + +// TestClaudeTelemetryEnvNamesAreUnique verifies no variable is set twice, which would +// leave which one wins up to the receiving process. +func TestClaudeTelemetryEnvNamesAreUnique(t *testing.T) { + seen := map[string]bool{} + for _, kv := range ClaudeTelemetryEnv("http://127.0.0.1:4318") { + k, _, ok := strings.Cut(kv, "=") + if !ok { + t.Errorf("%q is not a K=V pair", kv) + continue + } + if seen[k] { + t.Errorf("%s is set more than once", k) + } + seen[k] = true + } + // EndpointEnvVar must be one of them, since it is the name the caller checks + // against the inherited environment. + if !seen[EndpointEnvVar] { + t.Errorf("%s is not among the variables", EndpointEnvVar) + } +}