Skip to content

ax-server: set up OpenTelemetry tracing, metrics, events, and JSON logs - #557

Open
erain wants to merge 2 commits into
google:mainfrom
erain:otel-bootstrap
Open

erain wants to merge 2 commits into
google:mainfrom
erain:otel-bootstrap

Conversation

@erain

@erain erain commented Nov 5, 2026 •

Copy link
Copy Markdown
Collaborator

ax-server has no telemetry. It writes plain-text logs and serves nothing to scrape or collect, so an operator can't see where a slow CreateTask spent its time, how often RPCs fail, or which log lines belong together. This PR adds the OpenTelemetry setup that RPC instrumentation and task state events will build on. Part of #13.

The setup follows Agent Substrate's internal/serverboot, including its versions (otel v1.45.0, semconv v1.43.0) and its rules for the OTEL_* variables. ax-server and Substrate telemetry therefore behave the same way and join on the same attributes.

What ax-server now does

  • Traces export over OTLP/gRPC.
    • Sampling: 10% of new traces (ParentBased(TraceIDRatio(0.1))), and a caller's sampling decision is followed.
    • Propagation: only W3C trace context; incoming baggage is not trusted.
  • Metrics are pushed over OTLP, and also served for Prometheus on :9090/metrics.
    • OTEL_METRICS_EXPORTER controls only the push: otlp (the default) is on; prometheus or none is off. The new --metrics-addr flag controls only /metrics, and an empty value turns it off.
    • /metrics also serves Go and process metrics. They are not pushed.
    • The SDK's default cardinality limit applies (2000 attribute sets per instrument); OTEL_GO_X_CARDINALITY_LIMIT overrides it.
  • Events go through the new telemetry.Emit. With OTEL_LOGS_EXPORTER unset or none (the default, as in Substrate), an event is written to stdout as a JSON log line. With otlp, it is sent as an OTLP log record instead. With otlp,console, it goes to both. An event is never dropped because OTLP is off.
  • Logs are now JSON on stdout. A line logged while a span is active carries trace_id, span_id, and trace_flags. Log lines are not exported over OTLP.
  • SDK errors are logged through slog instead of stderr.

Settings ax-server can't use. As the OpenTelemetry specification asks, an unknown or invalid value is logged and ignored, which keeps the default. ax-server resolves OTEL_TRACES_SAMPLER itself, because the SDK falls back to sampling every trace. A typo such as parentbased_traceidratoi, or a ratio sampler whose argument is missing or out of range, keeps the 10% default. An unknown exporter name keeps the default exporter instead of turning export off.

Manifests. Both manifests read the OTEL_* settings from a new ax-otel-config ConfigMap with envFrom. They also set OTEL_RESOURCE_ATTRIBUTES from the downward API: k8s.namespace.name, k8s.pod.name, k8s.pod.uid, and service.instance.id set to the pod UID, as Substrate does.

  • deploy/ax-server.yaml ships with OTLP push off (OTEL_TRACES_EXPORTER: none, OTEL_METRICS_EXPORTER: prometheus). An install without a collector therefore logs no export errors and loses nothing: metrics stay on /metrics and events go to stdout. Connecting a collector means editing the ConfigMap; docs/observability.md shows how.
  • test/e2e/manifests/ax-server.yaml points at the collector that Substrate's kind overlay deploys (opentelemetry-collector.otel-system.svc:4317), with OTEL_LOGS_EXPORTER: otlp,console.
  • Port 9090 is exposed as metrics on the Service; probes and the API stay on :8080.

internal/axattr fixes the attribute names AX will put on spans and events. For actor identity it reuses Substrate's ate.atespace and ate.actor.uid, and it keeps those and the other identity attributes off metrics. docs/observability.md documents all of this for operators.

Nothing records spans, metrics, or events yet; that starts with the RPC instrumentation and task state event PRs.

Behavior changes:

  • ax-server's logs change from slog text to JSON.
  • ax-server also listens on :9090.

Validation

  • Unit tests: go test ./internal/telemetry (also with -race -count=3) covers:

    • spans, a metric, and an event delivered over OTLP to an in-process gRPC collector, with the ax-server resource;
    • OTEL_*_EXPORTER=none sending nothing, while spans are still sampled and /metrics still serves;
    • events written to stdout by default, OTLP only with otlp, and both with otlp,console;
    • the default sampler: about 10% of 4,000 root spans sampled, and children follow a sampled or unsampled parent;
    • invalid sampler settings (a typo, a missing ratio, a ratio of 10) keeping the 10% default, plus a table of sampler and exporter values;
    • only traceparent/tracestate propagated;
    • /metrics serving an instrument, Go and process metrics, and the resource; 300 attribute sets with no overflow under the default limit, and 100 series with the overflow series when OTEL_GO_X_CARDINALITY_LIMIT=100;
    • the log handler adding trace_id/span_id/trace_flags only when a span is active.

    go test ./..., go vet ./..., gofmt, and go mod tidy pass.

  • Live cluster:

    • Setup: a GKE 1.37 cluster with Agent Substrate v1.0.0-rc.1 and a gVisor worker pool, running ax-server from this branch with the E2E manifest. The ConfigMap pointed it at an OpenTelemetry Collector with a debug exporter, with a 10 s metric push interval.
    • Result: the full E2E suite passed (8/8). All 86 log lines ax-server wrote during the run were JSON, with no SDK errors or warnings, while it pushed metrics every 10 s. The downward-API resource reached the SDK:
    {"time":"2026-11-06T21:14:02.543509374Z","level":"INFO","msg":"tracing initialized","export":true}
    {"time":"2026-11-06T21:14:02.543842658Z","level":"INFO","msg":"metrics initialized","export":true}
    {"time":"2026-11-06T21:14:02.544556911Z","level":"INFO","msg":"OTLP logs export initialized","exporter":"console,otlp"}
    
    $ curl -s localhost:9090/metrics | grep ^target_info
    target_info{k8s_namespace_name="ax-system",k8s_pod_name="ax-server-5cf64fb5f-lzm9g",k8s_pod_uid="7918ee31-…",service_instance_id="7918ee31-…",service_name="ax-server",telemetry_sdk_language="go",telemetry_sdk_name="opentelemetry",telemetry_sdk_version="1.45.0"} 1
    

    Nothing records data yet, so the collector received no data points. OTLP delivery of each signal is covered by the unit test against the in-process collector, not by the live run.

This change was written with help from an AI coding assistant.

@erain
erain requested a review from JeffLuoo November 5, 2026 21:28
@erain
erain marked this pull request as ready for review November 5, 2026 21:29

@JeffLuoo JeffLuoo left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I compared this PR with the OpenTelemetry setup in Agent Substrate (internal/serverboot at 94e0a26a). Most of the SDK setup is the same: the versions, the resource, the default sampler, the propagator, the error handler, the batch processors, the JSON logs with trace IDs, and the separate metrics listener. The tests are good, and the log handler calls r.Clone() before it adds attributes, as the log/slog docs say.

This PR is part of #13, but later PRs will build on the configuration, the manifests, and the Providers API that it adds. Thus I recommend four changes before merge:

  1. Keep the 10% default when OTEL_TRACES_SAMPLER has an unknown value (telemetry.go L161).
  2. Send metrics over OTLP, as #13 asks (telemetry.go L179).
  3. Put the OTEL_* settings and the resource attributes in the manifests (deploy/ax-server.yaml).
  4. Decide the default of the logs exporter, and a stdout fallback for events (telemetry.go L202).

The other comments do not block the merge.

Validation at 2b1f206: go build ./cmd/ax-server, go vet and gofmt on the changed packages, and go mod tidy show no problems. go test -race -count=3 ./internal/telemetry passes. I did not run the e2e suite.

Comment thread internal/telemetry/telemetry.go Outdated
func newTracerProvider(ctx context.Context, res *resource.Resource) (*sdktrace.TracerProvider, error) {
opts := []sdktrace.TracerProviderOption{sdktrace.WithResource(res)}
// Without WithSampler, the SDK reads OTEL_TRACES_SAMPLER itself.
if strings.TrimSpace(os.Getenv("OTEL_TRACES_SAMPLER")) == "" {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before merge. When an operator sets OTEL_TRACES_SAMPLER, this code sets no sampler, so the SDK reads the variables. I measured the result on this branch with 2,000 root spans:

OTEL_TRACES_SAMPLER OTEL_TRACES_SAMPLER_ARG Share sampled
not set not set 0.10
parentbased_traceidratoi (typo) 0.1 1.00
parentbased_traceidratio not set 1.00
parentbased_traceidratio 10 1.00

The typo row is a defect. The OTel specification says that an implementation must warn about an unknown value and ignore the setting. For ax-server, to ignore the setting means to use the ax-server default of 10%. But the SDK uses its own default, parentbased_always_on, so a typo samples every trace.

The other two rows follow the specification, which reads a missing or invalid ratio as 1.0. Substrate keeps its default in these cases too, because then one missing value multiplies the trace volume by 10. I recommend the same.

Substrate's resolveTraceSampling implements these rules. Apply the same rules here.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in b2ff786. resolveSampler in internal/telemetry/config.go applies the same rules as Substrate's resolveTraceSampling. ax-server always passes its own sampler now, so the SDK's env handling never runs. An unknown name, or a ratio sampler whose argument is missing, unparseable, or outside [0, 1], logs a warning and keeps ParentBased(TraceIDRatio(0.1)). An empty value reads as unset.

TestInvalidSamplerSettingKeepsDefault runs your three rows through Init and checks that about 10% of 2,000 root spans are sampled. TestParseSampler covers the valid names and the error cases.

Comment thread docs/observability.md Outdated
|---|---|
| `OTEL_EXPORTER_OTLP_ENDPOINT` | Collector address, for example `http://otel-collector.observability.svc:4317`. The SDK default is `localhost:4317`. An `http://` endpoint is reached without TLS. |
| `OTEL_TRACES_EXPORTER`, `OTEL_LOGS_EXPORTER` | `otlp` (default) or `none`. With `none`, traces are still sampled and propagated, only not sent. |
| `OTEL_TRACES_SAMPLER`, `OTEL_TRACES_SAMPLER_ARG` | Replace the default sampler, `parentbased_traceidratio` at `0.1`. A caller's sampling decision is always followed. |

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"A caller's sampling decision is always followed" is true only for the parentbased_* samplers. It is false for always_on, always_off, and traceidratio.

Suggested change
| `OTEL_TRACES_SAMPLER`, `OTEL_TRACES_SAMPLER_ARG` | Replace the default sampler, `parentbased_traceidratio` at `0.1`. A caller's sampling decision is always followed. |
| `OTEL_TRACES_SAMPLER`, `OTEL_TRACES_SAMPLER_ARG` | Replace the default sampler, `parentbased_traceidratio` at `0.1`. With a `parentbased_*` sampler, ax-server always follows the sampling decision of the caller. |

If you apply the comment on telemetry.go line 161, also say here that a value that is not valid keeps the default.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied in b2ff786, with the sentence about invalid values added: "An unknown sampler, or a ratio sampler whose argument is missing or outside [0, 1], keeps the default."

Comment thread internal/telemetry/telemetry.go Outdated
return tp, nil
}

func newMeterProvider(res *resource.Resource) (*sdkmetric.MeterProvider, *prometheus.Registry, error) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before merge. Issue #13 asks for metrics over OTLP, but this function installs only the Prometheus reader. Substrate's InitMetrics installs an OTLP periodic reader and also serves /metrics.

With this PR, traces and events go to an OTLP collector, but metrics reach a backend only through a separate Prometheus scrape of port 9090. Thus an operator needs two collection paths for one process. This PR is part of #13, but it sets the metrics configuration that later PRs build on, so add the OTLP reader here.

I propose the same behavior as ateapi (metricsExporters):

  • OTEL_METRICS_EXPORTER controls only the OTLP push. otlp (the default in the OTel specification) turns it on, and prometheus or none turns it off.
  • --metrics-addr controls only /metrics, for operators who scrape.
  • As in ateapi, the Go and process metrics can stay on /metrics only.

The periodic reader exports at each interval (60 s by default). Without a collector, each export fails, and the SDK logs an error. Thus decide the behavior without a collector (see the comment on deploy/ax-server.yaml) in the same change.

Also add OTEL_METRICS_EXPORTER to docs/observability.md.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in b2ff786, with the same split as ateapi:

  • OTEL_METRICS_EXPORTER controls only the OTLP push. otlp (or unset) installs an otlpmetricgrpc periodic reader. prometheus or none leaves it out, and an unknown value keeps the default.
  • --metrics-addr controls only /metrics. The Prometheus reader is always installed.
  • The Go and process metrics stay on /metrics only.

otlpmetricgrpc is pinned at v1.45.0, the same as the other exporters. The behavior without a collector is set in the manifest (see the reply on deploy/ax-server.yaml), and docs/observability.md now documents OTEL_METRICS_EXPORTER and OTEL_METRIC_EXPORT_INTERVAL. TestInitExportsAllSignalsOverOTLP checks that a counter reaches the in-process collector, and TestInitHonorsExporterNone checks that none sends nothing while /metrics still serves.

Comment thread deploy/ax-server.yaml
ports:
- containerPort: 8080
name: http
- containerPort: 9090

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before merge. The manifest sets no OTEL_* variable, so the exporters send to the SDK default, localhost:4317. Nothing listens there in the pod. Nothing records spans yet, so no error occurs now. After the first PR that records spans, ax-server will log an SDK error for each export that fails.

Each cluster has a different collector address, so the manifest cannot contain a fixed address. Substrate keeps all its OTLP settings in one ConfigMap, ate-otel-config, and each component reads it with envFrom. ax-server runs in ax-system, so it cannot read that ConfigMap in ate-system. I propose:

  • Add an ax-otel-config ConfigMap for all OTEL_* settings, and read it with envFrom. Then an operator changes one object to connect ax-server to their collector. On a cluster that runs Substrate, the endpoint can be the same as in ate-otel-config.
  • Decide what happens when the operator sets no endpoint. One option: the ConfigMap sets the OTLP exporters to none, and docs/observability.md tells how to turn them on. The other option: the operator must supply the endpoint, for example with a kustomize patch.
  • Add the resource attributes from the downward API, as Substrate does (ate-api-server.yaml). Kubernetes expands $(VAR) only for a variable that comes earlier in the list:
- name: POD_NAME
  valueFrom:
    fieldRef:
      fieldPath: metadata.name
- name: POD_NAMESPACE
  valueFrom:
    fieldRef:
      fieldPath: metadata.namespace
- name: POD_UID
  valueFrom:
    fieldRef:
      fieldPath: metadata.uid
- name: OTEL_RESOURCE_ATTRIBUTES
  value: k8s.namespace.name=$(POD_NAMESPACE),k8s.pod.name=$(POD_NAME),k8s.pod.uid=$(POD_UID),service.instance.id=$(POD_UID)

These are OTel semantic convention attributes, so all backends can use them. Now the code sets service.instance.id to the hostname, which is the pod name, and sends no k8s.* attributes. With the pod UID, ax-server uses the same convention as Substrate.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in b2ff786:

  • Added an ax-otel-config ConfigMap in ax-system, read with envFrom. All OTEL_* settings go there.
  • No endpoint: I took your first option. deploy/ax-server.yaml ships with OTEL_TRACES_EXPORTER: none and OTEL_METRICS_EXPORTER: prometheus, and the logs exporter already defaults to none. A fresh install therefore logs no export errors and loses nothing: metrics stay on /metrics and events go to stdout. The ConfigMap has a commented endpoint line, and docs/observability.md has a "Connect a collector" section showing the edit and the rollout restart. The install docs don't assume GKE, so I didn't hard-code the GKE managed collector the way Substrate's base config does. The doc says that on a Substrate cluster the endpoint can be the one in ate-otel-config.
  • Added POD_NAME, POD_NAMESPACE, and POD_UID from the downward API, and OTEL_RESOURCE_ATTRIBUTES with k8s.namespace.name, k8s.pod.name, k8s.pod.uid, and service.instance.id=$(POD_UID), in that order. On the live cluster the pod got k8s.namespace.name=ax-system,k8s.pod.name=ax-server-5cf64fb5f-lzm9g,k8s.pod.uid=7918ee31-…,service.instance.id=7918ee31-…. The code still falls back to the hostname outside the manifests.

ports:
- containerPort: 8080
name: http
- containerPort: 9090

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apply the changes from the comment on deploy/ax-server.yaml here too. For the endpoint: hack/setup-e2e-kind.sh installs Substrate with --kind, and the kind overlay deploys an OpenTelemetry Collector at http://opentelemetry-collector.otel-system.svc:4317 (kind/ate-otel-config.yaml). That collector has pipelines for traces, metrics, and logs. Set OTEL_EXPORTER_OTLP_ENDPOINT to that address here. Then the e2e runs send to a real collector when the instrumentation PRs merge.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in b2ff786. The E2E manifest has the same ConfigMap, envFrom, and downward-API attributes. It sets OTEL_EXPORTER_OTLP_ENDPOINT: http://opentelemetry-collector.otel-system.svc:4317, which is the same address in the kind overlay at the go.mod version (v1.0.0-rc.1). It also sets OTEL_LOGS_EXPORTER: otlp,console, like Substrate's kind config, so events show up in kubectl logs too. I recorded the dependency on that address in the address table in skills/substrate/SKILL.md, so a Substrate bump checks it.

Comment thread internal/telemetry/telemetry.go Outdated
// exporterSelected reports whether the exporter list in the environment
// variable key selects otlp. Unset selects otlp. ax-server knows only otlp and
// none; other names are logged and ignored.
func exporterSelected(ctx context.Context, key string) bool {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: the OTel specification says: "if the user provides a value the implementation does not recognize, the implementation MUST generate a warning and gracefully ignore the setting." This function warns, but then it turns export off. For example, OTEL_TRACES_EXPORTER=otpl or OTEL_TRACES_EXPORTER=console stops the trace export. To ignore the setting, keep the default, otlp. Substrate's resolveExporters does this.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in b2ff786. The exporter variables now go through resolveExporters, a port of Substrate's rules. Unknown names are skipped with a warning. If no known name is left (otpl, console for traces), the default is kept. none combined with an exporter also keeps the default. Covered by TestResolveExporters.

Comment thread internal/telemetry/telemetry.go Outdated
mp := sdkmetric.NewMeterProvider(
sdkmetric.WithResource(res),
sdkmetric.WithReader(reader),
sdkmetric.WithCardinalityLimit(CardinalityLimit),

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This option also overrides OTEL_GO_X_CARDINALITY_LIMIT. I set that variable to 2000 and still got 100 series. Substrate keeps the SDK default of 2000 (metrics best practices).

The AX service has 14 RPCs. Thus RPC duration metrics by method and status code can go above 100 series. Then the SDK puts all new series into the overflow series and gives no error.

I recommend the SDK default. In SDK v1.45, a view cannot set a limit. The narrowest option is a limit for each instrument kind, with WithCardinalityLimitSelector on a reader.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. In b2ff786 I removed WithCardinalityLimit, so the SDK default of 2000 applies and OTEL_GO_X_CARDINALITY_LIMIT works again. TestCardinalityLimitFollowsEnvironment sets it to 100 and checks for 100 series plus the overflow series. The other test checks that 300 sets produce no overflow under the default. I left out the per-kind selector until a real instrument needs one. The docs now give 2000 and the variable.

Comment thread internal/telemetry/log.go Outdated
func (h traceHandler) Handle(ctx context.Context, r slog.Record) error {
if sc := trace.SpanContextFromContext(ctx); sc.IsValid() {
r = r.Clone()
r.AddAttrs(slog.String("trace_id", sc.TraceID().String()), slog.String("span_id", sc.SpanID().String()))

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The OTel specification for trace context in non-OTLP logs defines three optional fields: trace_id, span_id, and trace_flags. Substrate writes all three. To add the third: slog.String("trace_flags", sc.TraceFlags().String()).

For a later PR: the 15 slog calls in internal/server, internal/controller, and internal/substrate have no context. When these paths get spans, change the calls to the *Context functions. Otherwise their lines get no trace ID.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added trace_flags in b2ff786, and the log handler test checks it. Noted on the *Context calls: the RPC instrumentation PR will switch those 15 call sites when their paths get spans.

Comment thread internal/axattr/axattr.go Outdated
// identify an actor reuse Agent Substrate's names, so AX and Substrate records
// join on them.
//
// The identity attributes (task ID and name, actor UID, exit code, signal) are

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Add ate.atespace to this list. Each task has an atespace that the user chooses, so the set of values is not fixed. Substrate's no-actor-identity rule also forbids it on metrics.
  • The exit code and the signal each have a fixed set of values, so "unbounded" is not correct for them. If they must stay off metrics, give the reason. For example: ax-server records them only with the task identity.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both fixed in b2ff786. The package doc now lists ate.atespace with the identity attributes and cites Substrate's no-actor-identity rule. It says the exit code and the signal have bounded values but stay off metrics because ax-server records them only beside the task identity.

Comment thread internal/telemetry/telemetry_test.go Outdated
// receives.
type fakeCollector struct {
coltracepb.UnimplementedTraceServiceServer
collogspb.UnimplementedLogsServiceServer

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: the test registers fakeCollector only as the trace service, and logsService handles logs. Thus this embed has no use. Remove it.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed in b2ff786. The collector now serves metrics too, through a metricsService adapter like logsService.

@JeffLuoo JeffLuoo left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked b2ff786 against each reply. All 11 comments are fixed, and the four items that I asked to fix before merge are done. At b2ff786, go build, go vet ./..., gofmt, go mod tidy, and go test ./... pass, and go test -race -count=3 ./internal/telemetry passes too. I also tested the new behavior: a sampler typo keeps 10%, the OTLP metric push exports at each interval when nothing records (as the doc says), and an event goes to stdout when OTLP is off.

The two new comments do not block the merge.

A correction to the reply on the sampler: the SDK still parses OTEL_TRACES_SAMPLER in NewTracerProvider, before it applies WithSampler. With a typo, the sampled share is 10%, but the SDK also logs unsupported sampler through the error handler. Thus an operator sees two warnings, and the SDK warning does not say that ax-server kept the default. Substrate has the same second warning. No change is necessary.

Comment thread docs/observability.md Outdated
Comment on lines +43 to +45
The manifests also set `OTEL_RESOURCE_ATTRIBUTES` from the downward API:
`k8s.namespace.name`, `k8s.pod.name`, `k8s.pod.uid`, and `service.instance.id`
set to the pod UID, as Substrate does.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

env overrides envFrom for the same key. The Kubernetes API says: "Values defined by an Env with a duplicate key will take precedence." Thus if an operator adds OTEL_RESOURCE_ATTRIBUTES to ax-otel-config, for example to set deployment.environment.name, Kubernetes ignores it, and no error shows. Line 21 also says that the manifests read "every OTEL_* setting" from the ConfigMap. Substrate's manifests have the same behavior.

Suggested change
The manifests also set `OTEL_RESOURCE_ATTRIBUTES` from the downward API:
`k8s.namespace.name`, `k8s.pod.name`, `k8s.pod.uid`, and `service.instance.id`
set to the pod UID, as Substrate does.
The manifests also set `OTEL_RESOURCE_ATTRIBUTES` from the downward API:
`k8s.namespace.name`, `k8s.pod.name`, `k8s.pod.uid`, and `service.instance.id`
set to the pod UID, as Substrate does. This variable is in the Deployment's
`env`, which overrides `envFrom`, so the same key in `ax-otel-config` has no
effect. Add other resource attributes to the value in the Deployment.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied in 345d8d5 (along with rebasing onto main).


// attrValue converts an slog value to the attribute kind slog's JSON handler
// writes, so the two forms of an event match.
func attrValue(v slog.Value) attribute.Value {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: the comment says that the two forms of an event match, but they do not for a time or a group. I compared attrValue with the JSON handler:

slog value stdout (JSON handler) OTLP (attrValue)
slog.Time("at", t) 2026-11-06T21:00:00.000000123Z 2026-11-06 21:00:00.000000123 +0000 UTC
slog.Group("g", slog.String("k", "v")) {"k":"v"} [k=v]

The JSON handler formats a time as time.RFC3339Nano, and in v1.45 attribute.MapValue can hold a group:

case slog.KindTime:
	return attribute.StringValue(v.Time().Format(time.RFC3339Nano))
case slog.KindGroup:
	kvs := make([]attribute.KeyValue, 0, len(v.Group()))
	for _, a := range v.Group() {
		kvs = append(kvs, attribute.KeyValue{Key: attribute.Key(a.Key), Value: attrValue(a.Value)})
	}
	return attribute.MapValue(kvs...)

Also add these two cases to TestAttrValueMatchesJSONKinds. A struct in slog.Any also gives different forms (fmt against json.Marshal). If an event needs a struct value, convert it here too.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added slog.KindTime (time.RFC3339Nano) and slog.KindGroup (attribute.MapValue) in attrValue and covered both in TestAttrValueMatchesJSONKinds in 345d8d5.

@JeffLuoo JeffLuoo left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also rebase

ax-server had no telemetry: plain-text logs and nothing to scrape or
collect. Set up the OpenTelemetry SDK in a new internal/telemetry package
and wire it into ax-server.

- Traces export over OTLP/gRPC. New traces are sampled at 10%
  (ParentBased(TraceIDRatio)), a caller's decision is followed, and only
  W3C trace context is propagated, not baggage.
- A logger provider exports structured events over OTLP/gRPC.
  ax-server's own log lines stay on stdout.
- Metrics are served for Prometheus on :9090/metrics (--metrics-addr),
  with Go and process metrics and a cap of 100 attribute sets per
  instrument. Probes and the API stay on :8080.
- Logs are JSON, with trace_id and span_id when a span is active.
- SDK errors go to slog instead of stderr.

Exporters and sampling follow the standard OTEL_* environment
variables. internal/axattr names the attributes AX emits, reusing
Substrate's ate.atespace and ate.actor.uid so records join on them. The
OTel versions match Agent Substrate's (otel v1.45.0, semconv v1.43.0).

Nothing records spans or emits events yet; RPC instrumentation and task
state events build on this.
…d events to stdout

Address review on the OpenTelemetry setup:

- Resolve OTEL_TRACES_SAMPLER and OTEL_*_EXPORTER in ax-server, as Agent
  Substrate does. An unknown or invalid value is logged and keeps the
  default, so a typo no longer samples every trace or turns export off.
- Add an OTLP periodic metric reader. OTEL_METRICS_EXPORTER controls only
  the push; --metrics-addr still controls /metrics.
- Default OTEL_LOGS_EXPORTER to none and accept console. Add
  telemetry.Emit, which writes an event to stdout when OTLP logs are off,
  or beside OTLP with console, so events are not dropped.
- Keep the SDK's default cardinality limit (2000), so
  OTEL_GO_X_CARDINALITY_LIMIT applies again.
- Add trace_flags to log lines.
- Read OTEL_* settings from an ax-otel-config ConfigMap, and set the k8s.*
  resource attributes and service.instance.id from the downward API. The
  production manifest ships with OTLP push off; the E2E manifest points at
  the collector from Substrate's kind overlay.
- List ate.atespace among the attributes kept off metrics.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants