Skip to content

feat(core): add controller log→OTLP bridge (gated OTEL_LOGGING_ENABLED)#2151

Open
braghettos wants to merge 4 commits into
kagent-dev:mainfrom
braghettos:observability-log-bridge
Open

feat(core): add controller log→OTLP bridge (gated OTEL_LOGGING_ENABLED)#2151
braghettos wants to merge 4 commits into
kagent-dev:mainfrom
braghettos:observability-log-bridge

Conversation

@braghettos

Copy link
Copy Markdown

What

Adds an optional controller log → OTLP bridge: the controller's own zap logs can be exported over OTLP alongside the traces already emitted by InitTracerProvider, enabling log/trace correlation in the controller. This is the third and final observability gap proposed in #2148.

Today go/core/internal/telemetry/ only sets up traces; the controller's zap logs are stdout-only, so they can't be shipped to an OTLP backend or correlated with spans.

How

  • New internal/telemetry/logging.go:
    • InitLoggerProvider builds an OTLP LoggerProvider via autoexport.NewLogExporter (same env-driven exporter/endpoint resolution as InitTracerProvider) and registers it as the global OTel logger provider.
    • ControllerZapOpts returns a RawZapOpts(zap.WrapCore(zapcore.NewTee(core, bridgeCore))) option that additively tees the stdout zap core with an otelzap bridge core bound to that provider — stdout logging is preserved, OTLP is added.
  • tracing.go: extracted the shared newTelemetryResource helper so traces and logs carry identical resource attributes (no behaviour change to InitTracerProvider).
  • pkg/app/app.go: initialize the logger provider before building the controller logger (so the bridge binds to the configured global provider), then add the tee via ControllerZapOpts.

Gating (default-OFF, additive)

Enabled only when OTEL_LOGGING_ENABLED=true (the flag already registered in pkg/env). When unset, both InitLoggerProvider and ControllerZapOpts are no-ops and the controller logger is byte-identical to today.

Testing

  • New logging_test.go — asserts ControllerZapOpts() returns no options when disabled, returns a working teed logger when enabled (no panic with the default no-op global provider), and that InitLoggerProvider's shutdown is a safe no-op when disabled.
  • go build ./core/..., go test -race ./core/internal/telemetry/..., and golangci-lint run all pass.

Notes

Refs: #2148

…BLED)

Additively tee the controller's zap logger with an otelzap bridge core so
the controller's own logs can be exported over OTLP alongside the traces
already emitted via InitTracerProvider, without changing stdout logging.

- New internal/telemetry/logging.go:
  - InitLoggerProvider builds an OTLP LoggerProvider via autoexport
    (same env-driven exporter resolution as InitTracerProvider) and sets
    it as the global OTel logger provider.
  - ControllerZapOpts returns a RawZapOpts(zap.WrapCore(NewTee(...)))
    option that tees the stdout core with an otelzap bridge core bound to
    that provider.
- tracing.go: extract the shared newTelemetryResource helper so traces and
  logs carry identical resource attributes.
- app.go: initialize the logger provider before building the controller
  logger (so the bridge binds to it) and add the tee via ControllerZapOpts.

Gated behind OTEL_LOGGING_ENABLED; when unset both functions are no-ops
and the logger is byte-identical to today. Adds otelzap v0.19.0 (matching
otel/log v0.20.0); no other dependency changes.

Refs: kagent-dev#2148
Signed-off-by: Diego Braga <diego.braga86@gmail.com>
@github-actions github-actions Bot added the enhancement New feature or request label Jul 4, 2026
@braghettos braghettos marked this pull request as ready for review July 4, 2026 16:11
@braghettos braghettos requested a review from supreme-gg-gg as a code owner July 4, 2026 16:11
Copilot AI review requested due to automatic review settings July 4, 2026 16:11

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Adds an optional, default-off OpenTelemetry logging pipeline for the controller, enabling controller-runtime zap logs to be exported via OTLP (in addition to existing trace export) for log/trace correlation.

Changes:

  • Introduces InitLoggerProvider to configure and register a global OTel LoggerProvider via env-driven OTLP autoexport (gated by OTEL_LOGGING_ENABLED).
  • Adds ControllerZapOpts to tee the controller’s zap core to an otelzap bridge core while preserving stdout logging.
  • Refactors trace setup to share a common OTel Resource builder so traces and logs carry identical resource attributes.

Reviewed changes

Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
go/go.mod Adds otelzap bridge dependency and promotes otel/log to a direct dependency.
go/go.sum Updates module checksums for the new/now-direct dependencies.
go/core/pkg/app/app.go Initializes OTel log provider before constructing the controller logger and applies the zap tee option.
go/core/internal/telemetry/tracing.go Extracts shared newTelemetryResource helper for consistent resource attributes.
go/core/internal/telemetry/logging.go New OTLP logger provider init + controller zap tee option (gated).
go/core/internal/telemetry/logging_test.go Adds unit tests for gating behavior and no-op shutdown when disabled.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread go/core/pkg/app/app.go
Comment on lines +340 to +344
defer func() {
shutdownCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
defer cancel()
if err := shutdownLogging(shutdownCtx); err != nil {
setupLog.Error(err, "failed to shutdown logging")

@krisztianfekete krisztianfekete left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the PR!
Added some comments, and just like in #2149, we'd like to see docs/examples to set it up, and some screenshots showing that the changes have been tested in a real setup. An OTel collector + debugexporter is perfect for this purpose if you don't want to ingest the logs into a proper logging backend.

if !env.OtelLoggingEnabled.Get() {
return nil
}
bridgeCore := otelzap.NewCore(loggerBridgeName,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think this actually gets us log-trace correlation as-is, which is what you're trying to deliver?

otelzap.Core only adds trace_id/span_id to a record when a context.Context is passed as a zap field. controller-runtime logs through logr -> zapr, and log.FromContext(ctx) only threads values in via WithValues, so I am not sure these records contain spans linked to them.

)
return []crzap.Opts{
crzap.RawZapOpts(zap.WrapCore(func(core zapcore.Core) zapcore.Core {
return zapcore.NewTee(core, bridgeCore)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The OTel log SDK puts severity filtering in a processor, not in the exporter or the bridge, so someone running at error level only expects error+ logs, but when they enable this flag it will ship all their info/debug logs to the backend.

We can fix it via https://pkg.go.dev/go.opentelemetry.io/contrib/processors/minsev

// signal pipelines (traces and logs), keeping their resource attributes
// identical.
func newTelemetryResource(ctx context.Context, serviceVersion string) (*resource.Resource, error) {
instanceID, err := os.Hostname()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Since newTelemetryResource is now called once by InitTracerProvider and once by InitLoggerProvider, and os.Hostname() falls back to a fresh uuid.New() on failure, it's possible that processes can end up with two different service.instance.ids across the two ttelemetry signals.

Let's build the resource once in app.go and pass the same *resource.Resource into both initializers instead.

// TestControllerZapOpts_EnabledTeesLogger verifies that when OTEL_LOGGING_ENABLED
// is set, an otelzap bridge option is returned and the resulting logger is usable
// (the bridge tees onto the stdout core without panicking).
func TestControllerZapOpts_EnabledTeesLogger(t *testing.T) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's also test

  1. a record actually having trace_id when there's an active span, and
  2. severity gating vs the stdout level

…e, docs

Addresses @krisztianfekete's review on kagent-dev#2151:
- Severity filtering now in a minsev processor tied to the controller's zap
  level, so enabling OTLP export at error level no longer ships info/debug.
- Build the OTel resource once (NewTelemetryResource) in app.go and pass it
  into both InitTracerProvider and InitLoggerProvider, so a hostname-lookup
  failure can't yield different service.instance.id across signals.
- Reframe the feature honestly as OTLP controller-log export: otelzap only
  attaches trace_id when a log call carries the request context as a field,
  which controller-runtime's logr->zapr path does not do — documented in code,
  tests, and docs/controller-otlp-logs.md rather than claiming automatic
  correlation.
- Tests: trace context attached when ctx is passed as a field (and absent
  otherwise), the zap-level->min-severity mapping, and severity gating.
- app.go: note that telemetry shutdowns run on graceful termination; fatal
  startup os.Exit paths crash-fast.

Refs: kagent-dev#2148
Signed-off-by: Diego Braga <diego.braga86@gmail.com>
@braghettos braghettos requested a review from a team as a code owner July 9, 2026 15:18
@braghettos

Copy link
Copy Markdown
Author

Thanks @krisztianfekete — great catches. Pushed changes:

  • Log-trace correlation: you're right. otelzap only attaches trace_id/span_id when a log call passes a context.Context as a field, and controller-runtime's logrzapr path doesn't thread the reconcile context into fields — so log.FromContext(ctx) records aren't automatically span-linked. Rather than claim something that doesn't hold, I've reframed the feature as OTLP controller-log export (ship controller logs to an OTLP backend alongside traces, same resource) and documented the correlation caveat in the code, tests, and docs. Automatic correlation for all controller logs would need a separate mechanism — happy to explore it as a follow-up if you think it's worth it. Added a test showing the span is attached when the ctx is passed as a field (and absent otherwise).
  • Severity: moved filtering into a minsev processor tied to the controller's --zap-log-level, so enabling export at error level no longer ships info/debug. Test asserts sub-threshold records are dropped.
  • Duplicate service.instance.id: the resource is now built once (NewTelemetryResource) in app.go and passed into both InitTracerProvider and InitLoggerProvider.
  • Docs: added docs/controller-otlp-logs.md — enabling, severity behaviour, the correlation caveat, and an OTel collector + debug exporter setup for testing. (Screenshot placeholder left in the doc — I don't currently have a cluster/collector wired to capture one; happy to add.)
  • Copilot (os.Exit vs deferred shutdown): the telemetry shutdowns run on graceful termination (manager stop); the fatal startup os.Exit paths crash-fast without flushing (minimal buffered telemetry, error already on stderr). Noted this in a comment. Let me know if you'd prefer a run() error refactor so those paths flush too.

…g exporter

Replace the screenshot TODO with real collector debug output captured by driving
the log bridge against a local OTel collector: controller LogRecords exported
over OTLP with the shared resource (service.name=kagent-controller) and the
github.com/kagent-dev/kagent/go/core scope.

Refs: kagent-dev#2148
Signed-off-by: Diego Braga <diego.braga86@gmail.com>
@braghettos

Copy link
Copy Markdown
Author

Tested with an OTel collector + debug exporter ✅

Drove the log bridge against a local OTel Collector (otlp receiver → debug exporter, per your suggestion) with OTEL_LOGGING_ENABLED=true. The controller's log records arrive over OTLP with the shared resource (service.name=kagent-controller, same as its traces) and the bridge scope:

Resource attributes:
     -> k8s.namespace.name: Str(kagent)
     -> k8s.pod.name: Str(kagent-controller-...)
     -> service.name: Str(kagent-controller)
     -> service.version: Str(v0.0.0-demo)
InstrumentationScope github.com/kagent-dev/kagent/go/core
LogRecord  SeverityText: info   Body: Str(reconciling Agent)             { controller=agent, agent=k8s-agent, namespace=kagent }
LogRecord  SeverityText: info   Body: Str(Agent reconciled successfully) { controller=agent, agent=k8s-agent }
LogRecord  SeverityText: error  Body: Str(example error log)             { reason=demo }

Stdout logging is unchanged (additive tee). Added this to docs/controller-otlp-logs.md.

Signed-off-by: Diego Braga <diego.braga86@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants