Skip to content

feat(tracing): add tracing support and configuration options for BBR #2518

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
k8s-ci-robot merged 2 commits into from
Mar 11, 2026
Merged

feat(tracing): add tracing support and configuration options for BBR #2518

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
141157c
feat(tracing): add tracing support and configuration options for BBR
AtharvaPakade Mar 8, 2026
bd60ece
refactor(tracing): make InitTracing generic and unify instrumentation…
AtharvaPakade Mar 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions cmd/bbr/runner/runner.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,10 @@ import (
"sigs.k8s.io/gateway-api-inference-extension/pkg/bbr/metrics"
"sigs.k8s.io/gateway-api-inference-extension/pkg/bbr/plugins"
runserver "sigs.k8s.io/gateway-api-inference-extension/pkg/bbr/server"
"sigs.k8s.io/gateway-api-inference-extension/pkg/bbr/tracing"
logutil "sigs.k8s.io/gateway-api-inference-extension/pkg/common/observability/logging"
"sigs.k8s.io/gateway-api-inference-extension/pkg/common/observability/profiling"

@nirrozenbaum nirrozenbaum Mar 8, 2026

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.

nit: remove extra space.

Suggested change

"sigs.k8s.io/gateway-api-inference-extension/version"
)

Expand Down Expand Up @@ -102,6 +104,15 @@ func (r *Runner) Run(ctx context.Context) error {
pflag.VisitAll(func(f *pflag.Flag) {
flags[f.Name] = f.Value
})

if opts.Tracing {
err := tracing.InitTracing(ctx, setupLog)
if err != nil {
setupLog.Error(err, "failed to initialize tracing")
return err
}
}

Comment on lines +107 to +114

@nirrozenbaum nirrozenbaum Mar 8, 2026

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 understand that the intention is to keep separate traces between bbr/epp.
two points about this:

  1. we should generalize tracing helper functions under common/observability/tracing in a consumable way, to allow both epp/bbr consume it and make their own traces independently.
  2. at the same time, we should also allow (in epp, not here) to continue the same trace from bbr. technically speaking, if we want to see a full trace of the request (including bbr + epp) that is very useful.
    having said that, this point is not required in current PR (can be done in follow ups), but just something to keep in mind.

@AtharvaPakade AtharvaPakade Mar 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

  1. I do plan to do that, but just avoided it for now in this PR as epp changes will also required. In futhure PR i will make this uniform, just to give a hint i was planning some pacakge level tracing config that will be initialized by common helper.
  2. So i do have this in mind, i have not drafted a proposal yet but there are few things i have indetified which can add a lot of values to tracing in general.

setupLog.Info("Flags processed", "flags", flags)

logutil.InitLogging(&opts.ZapOptions)
Expand Down
31 changes: 30 additions & 1 deletion config/charts/body-based-routing/templates/bbr.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,13 +33,42 @@ spec:
- {{ printf "%s:%s" .type .name | quote }}
{{- end }}
{{- end }}
{{- if not .Values.bbr.multiNamespace }}
{{- if .Values.bbr.tracing.enabled}}
- --tracing=true
{{- else}}
- --tracing=false
{{- end}}
Comment on lines +36 to +40

@nirrozenbaum nirrozenbaum Mar 8, 2026

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.

is it possible to align with other flags using key/value in separate cmd-line flags? does it work?

Suggested change
{{- if .Values.bbr.tracing.enabled}}
- --tracing=true
{{- else}}
- --tracing=false
{{- end}}
{{- if .Values.bbr.tracing.enabled}}
- --tracing
- "true"
{{- else}}
- --tracing
- "false"
{{- end}}

@AtharvaPakade AtharvaPakade Mar 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This will not work because it resolves to --tracing <value>. Since tracing is a boolean flag, the Go flag parser treats the presence of --tracing as true, and the following value is interpreted as a positional argument rather than the flag’s value.

env:
{{- if not .Values.bbr.multiNamespace }}
- name: NAMESPACE
valueFrom:
fieldRef:
fieldPath: metadata.namespace
{{- end }}
{{- if .Values.bbr.tracing.enabled}}
Comment on lines +42 to +48

@nirrozenbaum nirrozenbaum Mar 8, 2026

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.

out of curiosity, is leaving env with no values under it valid?
e.g., if multiNamespace is false and tracing.enabled is false.

@AtharvaPakade AtharvaPakade Mar 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It is valid and works, but it would be better to add another if to check if one of the two condition are true.

@nirrozenbaum nirrozenbaum Mar 8, 2026

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.

you can leave it as is for now since empty env is working.
there is a planned change for the multiNamespace anyway so we'll need to do some changes here.

- name: OTEL_SERVICE_NAME
value: {{ .Values.bbr.tracing.otelServiceName | default "gateway-api-inference-extension/bbr" }}

@nirrozenbaum nirrozenbaum Mar 8, 2026

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.

can we move default to values.yaml where we hold all other defaults?

- name: OTEL_EXPORTER_OTLP_ENDPOINT
value: {{ .Values.bbr.tracing.otelExporterEndpoint }}
- name: OTEL_RESOURCE_ATTRIBUTES_NODE_NAME
valueFrom:
fieldRef:
apiVersion: v1
fieldPath: spec.nodeName
- name: OTEL_RESOURCE_ATTRIBUTES_POD_NAME
valueFrom:
fieldRef:
apiVersion: v1
fieldPath: metadata.name
- name: OTEL_RESOURCE_ATTRIBUTES
value: 'k8s.namespace.name=$(NAMESPACE),k8s.node.name=$(OTEL_RESOURCE_ATTRIBUTES_NODE_NAME),k8s.pod.name=$(OTEL_RESOURCE_ATTRIBUTES_POD_NAME)'
- name: OTEL_TRACES_SAMPLER
value: {{ .Values.bbr.tracing.sampling.sampler | quote }}
- name: OTEL_TRACES_SAMPLER_ARG
value: {{ .Values.bbr.tracing.sampling.samplerArg | quote }}
- name: OTEL_TRACES_EXPORTER
value: otlp
{{- end}}
ports:
- containerPort: {{ .Values.bbr.port }}
# health check
Expand Down
9 changes: 9 additions & 0 deletions config/charts/body-based-routing/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,15 @@ bbr:
# Log verbosity
v: 3

tracing:
enabled: true

@nirrozenbaum nirrozenbaum Mar 8, 2026

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.

can you align with epp and put default enabled=false?

Suggested change
enabled: true
enabled: false

# Sets Custom service name for traces if left empty it will default to `gateway-api-inference-extension/bbr`
otelServiceName: ""
otelExporterEndpoint: "http://localhost:4317"
sampling:
sampler: "parentbased_traceidratio"
samplerArg: "0.1"

provider:
name: none

Expand Down
8 changes: 8 additions & 0 deletions pkg/bbr/handlers/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,13 @@ import (
"time"

extProcPb "github.com/envoyproxy/go-control-plane/envoy/service/ext_proc/v3"
"go.opentelemetry.io/otel/trace"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/status"
"sigs.k8s.io/controller-runtime/pkg/log"

"sigs.k8s.io/gateway-api-inference-extension/pkg/bbr/framework"
"sigs.k8s.io/gateway-api-inference-extension/pkg/bbr/tracing"
reqenvoy "sigs.k8s.io/gateway-api-inference-extension/pkg/common/envoy/request"
logutil "sigs.k8s.io/gateway-api-inference-extension/pkg/common/observability/logging"
reqcommon "sigs.k8s.io/gateway-api-inference-extension/pkg/common/request"
Expand Down Expand Up @@ -77,6 +79,12 @@ type Response struct {

func (s *Server) Process(srv extProcPb.ExternalProcessor_ProcessServer) error {
ctx := srv.Context()

// Start tracing span for the request
tracer := tracing.GetExtProcTracer()
ctx, span := tracer.Start(ctx, "gateway.request", trace.WithSpanKind(trace.SpanKindServer))
defer span.End()

logger := log.FromContext(ctx)
loggerVerbose := logger.V(logutil.VERBOSE)
loggerVerbose.Info("Processing")
Expand Down
3 changes: 3 additions & 0 deletions pkg/bbr/server/options.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ type Options struct {
// Diagnostics.
//
logging.LoggingOptions // Logging configuration.
Tracing bool // Enable emitting traces
MetricsPort int // The metrics port exposed by BBR.
GRPCHealthPort int // The port for gRPC liveness and readiness probes.
EnablePprof bool // Enables pprof handlers.
Expand All @@ -61,6 +62,7 @@ func NewOptions() *Options {
GRPCPort: DefaultGrpcPort,
GRPCHealthPort: DefaultGrpcHealthPort,
LoggingOptions: *logging.NewOptions(),
Tracing: true,
MetricsPort: 9090,
EnablePprof: true,
SecureServing: true,
Expand All @@ -82,6 +84,7 @@ func (opts *Options) AddFlags(fs *pflag.FlagSet) {
"The port used for gRPC liveness and readiness probes.")
fs.IntVar(&opts.MetricsPort, "metrics-port", opts.MetricsPort,
"The metrics port exposed by BBR.")
fs.BoolVar(&opts.Tracing, "tracing", opts.Tracing, "Enables emitting traces.")
fs.BoolVar(&opts.MetricsEndpointAuth, "metrics-endpoint-auth", opts.MetricsEndpointAuth,
"Enables authentication and authorization of the metrics endpoint.")
fs.BoolVar(&opts.Streaming, "streaming", opts.Streaming,
Expand Down
53 changes: 53 additions & 0 deletions pkg/bbr/tracing/tracing.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
/*
Copyright 2026 The Kubernetes Authors.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package tracing

import (
"context"
"os"

"github.com/go-logr/logr"
"go.opentelemetry.io/otel"
"go.opentelemetry.io/otel/attribute"
"go.opentelemetry.io/otel/trace"
tracingutils "sigs.k8s.io/gateway-api-inference-extension/pkg/common/observability/tracing"
"sigs.k8s.io/gateway-api-inference-extension/version"
)

const (
defaultServiceName = "gateway-api-inference-extension/bbr"
extprocInstrumentationName = defaultServiceName + "/extproc"
)

func InitTracing(ctx context.Context, logger logr.Logger) error {
_, ok := os.LookupEnv("OTEL_SERVICE_NAME")
if !ok {
os.Setenv("OTEL_SERVICE_NAME", defaultServiceName)
}

return tracingutils.InitTracing(ctx, logger)
}

@nirrozenbaum nirrozenbaum Mar 8, 2026

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.

can we instead pass in default service name and completely reuse in both places?


func GetExtProcTracer() trace.Tracer {
return otel.Tracer(
extprocInstrumentationName,
trace.WithInstrumentationVersion(version.BuildRef),
trace.WithInstrumentationAttributes(
attribute.String("commit-sha", version.CommitSHA),
),
)
}

@nirrozenbaum nirrozenbaum Mar 8, 2026

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.

this looks different than what we have in epp.
not a tracing expert, but we should probably align on best practice and reuse the same in both.
BuildRef and CommitSHA seems very useful IMO.

@AtharvaPakade AtharvaPakade Mar 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Like i mentioned earlier i do plan to make it uniform with EPP in future PR. The intention for adding these 2 funcs GetExtProcTracer, InitTracing was to avoid same service names and instrumentation scope for both server as this will mixup traces.

If you suggest, for now i can use default name in case of epp and pass bbr specific names to the common function. By removing the tracing package for now from bbr.

@nirrozenbaum nirrozenbaum Mar 8, 2026 •
edited
Loading

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 recommend to keep it aligned in both places.
so in both epp/bbr runner, you can do something like:

tracingServiceName := env.GetEnvString(envVar, "<THE-DEFAULT-PER-COMPONENT>", setupLog)
tracing.InitTracing(... current args ..., additional-arg: tracingServingName)

@nirrozenbaum nirrozenbaum Mar 8, 2026 •
edited
Loading

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.

so instead of diverging, and then try to align things - it would probably be better to align on current tracing from epp, then if things need to be added (e.g., like CommitSHA and buildRef which seems useful), we add it in the common packages, and then both epp/bbr get those changes.