Skip to content

Conversation

@himaschal
Copy link
Collaborator

Summary

This PR integrates the digicert/ctutils shared logging library to enable OpenTelemetry-compliant distributed tracing across the Trillian log server and signer components.

Changes

  • config/config.go: New centralized logging configuration with InitLogging()
  • cmd/trillian_log_server/main.go: Initialize OTEL logging on startup
  • cmd/trillian_log_signer/main.go: Initialize OTEL logging on startup
  • cmd/internal/serverutil/main.go: Add chained gRPC interceptors for trace propagation
  • examples/deployment/docker/log_server/Dockerfile.unified: Docker build with private ctutils dependency
  • README.md: Add Observability section documenting OTEL configuration
  • go.mod/go.sum: Add ctutils v0.1.6 and OTEL dependencies

Configuration

Tracing is controlled via environment variables:

Variable Description Default
OTEL_ENABLED Enable OpenTelemetry false
OTEL_EXPORTER Exporter type (otlp or stdout) stdout
OTEL_COLLECTOR_ENDPOINT OTLP collector URL localhost:4317
OTEL_SERVICE_NAME Service name for traces service-specific
OTEL_SAMPLE_RATIO Sampling ratio (0.0-1.0) 1.0

Related PRs

Testing

Manual testing with local OTEL collector. Full integration testing planned.

Refs: RA-8279

… integration

This change integrates the digicert/ctutils shared logging library to enable
OpenTelemetry-compliant distributed tracing across the Trillian log server and
signer components.

Key changes:
- Add config/config.go with InitLogging() for centralized OTEL configuration
- Update log_server and log_signer main.go to call config.InitLogging()
- Add chained gRPC interceptors for trace context propagation
- Add Dockerfile.unified with SSH access for private ctutils dependency
- Update go.mod/go.sum for ctutils v0.1.6 and OTEL dependencies

The logging configuration is driven by environment variables:
- OTEL_ENABLED: Enable/disable OpenTelemetry (default: false)
- OTEL_EXPORTER: Exporter type ('otlp' or 'stdout')
- OTEL_COLLECTOR_ENDPOINT: OTLP collector URL
- OTEL_SERVICE_NAME: Service name for traces
- OTEL_SAMPLE_RATIO: Sampling ratio (0.0-1.0)

This enables end-to-end request tracing from CTFE through Trillian backends,
allowing operators to correlate logs and traces across the CT infrastructure.

Refs: RA-8279
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR integrates OpenTelemetry distributed tracing into Trillian log server and signer components through the digicert/ctutils shared logging library, enabling end-to-end request tracing and structured logging.

Changes:

  • Added OTEL-compliant distributed tracing via ctutils dependency
  • Configured gRPC and HTTP middleware for trace propagation and logging
  • Added environment-variable based OTEL configuration

Reviewed changes

Copilot reviewed 16 out of 18 changed files in this pull request and generated 13 comments.

Show a summary per file
File Description
config/config.go New centralized logging initialization with OTEL setup
cmd/trillian_log_server/main.go Initialize logging on server startup
cmd/trillian_log_signer/main.go Initialize logging on signer startup
cmd/internal/serverutil/main.go Add gRPC/HTTP interceptors for trace propagation
go.mod/go.sum Add ctutils v0.1.13-test and updated OTEL dependencies
examples/deployment/docker//Dockerfile Docker build configuration for private ctutils dependency
.github/workflows/*.yaml CI authentication for private ctutils repository
README.md Documentation for OTEL configuration
experimental/batchmap/batchmap.shims.go Auto-generated code formatting updates

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

Comment on lines +9 to +17
func InitLogging() {
// Initialize OpenTelemetry with config struct
logging.InitOpenTelemetry(logging.TelemetryConfigFromEnv())

// Example: select logger backend via config, env var, or flag
logCfg := logging.Config{Format: "json"}
adapter := adapters.NewLogrusAdapter(logCfg)
logging.SetLoggerAdapter(adapter)
}
Copy link

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

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

The new config package and InitLogging() function lack test coverage. Given that this is a critical initialization function that sets up OpenTelemetry and logging adapters for the entire application, it should have unit tests to verify proper initialization, error handling, and configuration loading from environment variables.

Copilot uses AI. Check for mistakes.
Comment on lines +146 to +147
http.Handle("/metrics", logging.Middleware(promhttp.Handler()))
http.HandleFunc("/healthz", logging.MiddlewareFunc(m.healthz))
Copy link

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

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

The HTTP middleware wrapping (logging.Middleware and logging.MiddlewareFunc) adds observability to HTTP endpoints, but there's no error handling if these functions fail or return errors. Consider adding error checking or at least documenting the error handling behavior of these middleware functions.

Copilot uses AI. Check for mistakes.
# Use Go proxy for faster module downloads (can be overridden at build time)
ENV GOPROXY=https://proxy.golang.org,direct

# Always copy main repo and ctutils
Copy link

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

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

The Dockerfile expects both trillian/. and ctutils/. directories to exist in the build context (lines 17-18), but this assumes a specific directory structure that may not be clear to users. Consider documenting this requirement in the README or adding a comment explaining the expected build context structure and how to set it up.

Suggested change
# Always copy main repo and ctutils
# Always copy main repo and ctutils.
# NOTE: The Docker build context must contain `trillian/` and `ctutils/` directories
# at its root. Typically this means running `docker build` from the repository root,
# e.g.: `docker build -f examples/deployment/docker/log_server/Dockerfile.unified .`

Copilot uses AI. Check for mistakes.
Comment on lines 8 to 17
# Download dependencies first - this should be cacheable.
COPY go.mod go.sum ./
ARG GITHUB_TOKEN
ENV GITHUB_TOKEN=${GITHUB_TOKEN}
ENV GOPRIVATE=github.com/digicert/ctutils
ENV GONOSUMDB=github.com/digicert/ctutils
RUN git config --global url."https://${GITHUB_TOKEN}:x-oauth-basic@github.com/".insteadOf "https://github.com/"

# Download dependencies first - this should be cacheable.
COPY go.mod go.sum ./
Copy link

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

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

Lines 8-9 and lines 16-17 are duplicated. The comment "Download dependencies first - this should be cacheable." and the COPY go.mod go.sum ./ command appear twice. The first occurrence (lines 8-9) should be removed since the authentication setup (lines 10-14) needs to happen before copying the go.mod/go.sum files for the authentication to be effective during go mod download.

Copilot uses AI. Check for mistakes.
toolchain go1.23.4

require (
// require github.com/digicert/ctutils v0.1.2 // commented out for localreplace: use local ctutils for development
Copy link

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

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

The comment on line 8 is misleading. It says "commented out for localreplace: use local ctutils for development" but the actual require statement for ctutils is on line 54 and is not commented out. Either remove this comment or clarify the intended development workflow. Additionally, having a commented require statement can be confusing - consider using go.mod replace directives for local development instead.

Suggested change
// require github.com/digicert/ctutils v0.1.2 // commented out for localreplace: use local ctutils for development
// For local development of github.com/digicert/ctutils, use a go.mod replace directive instead of a commented-out require.

Copilot uses AI. Check for mistakes.
logging.InitOpenTelemetry(logging.TelemetryConfigFromEnv())

// Example: select logger backend via config, env var, or flag
logCfg := logging.Config{Format: "json"}
Copy link

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

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

The hardcoded logger configuration Format: "json" is not aligned with the environment-based configuration approach mentioned in the PR description. Consider making the log format configurable via environment variables (e.g., LOG_FORMAT) to maintain consistency with the OTEL configuration pattern and allow flexibility in different deployment environments.

Copilot uses AI. Check for mistakes.
Comment on lines 9 to 14
ARG GITHUB_TOKEN
ENV GITHUB_TOKEN=${GITHUB_TOKEN}
ENV GOPRIVATE=github.com/digicert/ctutils
ENV GONOSUMDB=github.com/digicert/ctutils
RUN git config --global url."https://${GITHUB_TOKEN}:x-oauth-basic@github.com/".insteadOf "https://github.com/"

Copy link

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

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

The token is stored in an environment variable GITHUB_TOKEN and then used in git config. This approach exposes the token in the environment and in the git config. Consider using --mount=type=secret to handle the token more securely, especially since this is a private repository token that should be protected.

Copilot uses AI. Check for mistakes.
Comment on lines 10 to 14
ARG GITHUB_TOKEN
ENV GITHUB_TOKEN=${GITHUB_TOKEN}
ENV GOPRIVATE=github.com/digicert/ctutils
ENV GONOSUMDB=github.com/digicert/ctutils
RUN git config --global url."https://${GITHUB_TOKEN}:x-oauth-basic@github.com/".insteadOf "https://github.com/"
Copy link

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

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

The token is stored in an environment variable GITHUB_TOKEN and then used in git config. This approach exposes the token in the environment and in the git config. Consider using --mount=type=secret to handle the token more securely, especially since this is a private repository token that should be protected.

Copilot uses AI. Check for mistakes.

// Set up logging adapter via config logic
// This can be extended to use env vars, flags, or config files
config.InitLogging()
Copy link

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

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

The config.InitLogging() call has no error handling. If the initialization fails (e.g., due to invalid configuration, missing environment variables, or OTEL setup issues), the server will continue running with potentially incorrect logging/tracing configuration. Consider either: 1) Having InitLogging() return an error and handle it here, or 2) Documenting that InitLogging() will panic/exit on critical errors.

Suggested change
config.InitLogging()
if err := config.InitLogging(); err != nil {
klog.Exitf("Failed to initialize logging: %v", err)
}

Copilot uses AI. Check for mistakes.
Comment on lines +117 to +119
// Initialize OpenTelemetry for the Trillian server
//config.InitLogging()

Copy link

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

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

The OpenTelemetry initialization is commented out. If this initialization is not needed here (because it's already called in the main functions), the commented code should be removed. If it is needed, it should be uncommented. Leaving commented code can cause confusion about whether this initialization is intentional or was forgotten.

Suggested change
// Initialize OpenTelemetry for the Trillian server
//config.InitLogging()

Copilot uses AI. Check for mistakes.
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.

1 participant