feat(tracing): add tracing support and configuration options for BBR#2518
Conversation
- add tracing package for BBR-specific instrumentation and helpers - update `body-based-routing` helm chart to support configuring tracing via OTEL env vars (service name, exporter endpoint, sampler, sampler arg, and exporter type)
✅ Deploy Preview for gateway-api-inference-extension ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Hi @AtharvaPakade. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/ok-to-test |
nirrozenbaum
left a comment
There was a problem hiding this comment.
@AtharvaPakade thanks!
this looks like a great start.
I left few comments.
@sallyom would be great if you can have a look as well :)
| {{- if not .Values.bbr.multiNamespace }} | ||
| - name: NAMESPACE | ||
| valueFrom: | ||
| fieldRef: | ||
| fieldPath: metadata.namespace | ||
| {{- end }} | ||
| {{- if .Values.bbr.tracing.enabled}} |
There was a problem hiding this comment.
out of curiosity, is leaving env with no values under it valid?
e.g., if multiNamespace is false and tracing.enabled is false.
There was a problem hiding this comment.
It is valid and works, but it would be better to add another if to check if one of the two condition are true.
There was a problem hiding this comment.
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.
| if opts.Tracing { | ||
| err := tracing.InitTracing(ctx, setupLog) | ||
| if err != nil { | ||
| setupLog.Error(err, "failed to initialize tracing") | ||
| return err | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
I understand that the intention is to keep separate traces between bbr/epp.
two points about this:
- we should generalize tracing helper functions under
common/observability/tracingin a consumable way, to allow both epp/bbr consume it and make their own traces independently. - 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.
There was a problem hiding this comment.
- 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.
- 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.
| "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" | ||
|
|
There was a problem hiding this comment.
nit: remove extra space.
| {{- if .Values.bbr.tracing.enabled}} | ||
| - --tracing=true | ||
| {{- else}} | ||
| - --tracing=false | ||
| {{- end}} |
There was a problem hiding this comment.
is it possible to align with other flags using key/value in separate cmd-line flags? does it work?
| {{- if .Values.bbr.tracing.enabled}} | |
| - --tracing=true | |
| {{- else}} | |
| - --tracing=false | |
| {{- end}} | |
| {{- if .Values.bbr.tracing.enabled}} | |
| - --tracing | |
| - "true" | |
| {{- else}} | |
| - --tracing | |
| - "false" | |
| {{- end}} |
There was a problem hiding this comment.
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.
| {{- end }} | ||
| {{- if .Values.bbr.tracing.enabled}} | ||
| - name: OTEL_SERVICE_NAME | ||
| value: {{ .Values.bbr.tracing.otelServiceName | default "gateway-api-inference-extension/bbr" }} |
There was a problem hiding this comment.
can we move default to values.yaml where we hold all other defaults?
| v: 3 | ||
|
|
||
| tracing: | ||
| enabled: true |
There was a problem hiding this comment.
can you align with epp and put default enabled=false?
| enabled: true | |
| enabled: false |
| 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) | ||
| } |
There was a problem hiding this comment.
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), | ||
| ), | ||
| ) | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)There was a problem hiding this comment.
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.
… scope names - accept `defaultServiceName` as a parameter in `InitTracing` instead of hardcoding it, allowing BBR and EPP to set their own default service names - remove the BBR-specific tracing package; tracer initialization is now handled by the common tracing package - use distinct instrumentation scope names per component (`/bbr/extproc`, `/epp/extproc`) with version and commit-sha attributes - update Helm chart defaults: disable tracing by default and set explicit default service name for BBR
AtharvaPakade
left a comment
There was a problem hiding this comment.
@nirrozenbaum , I have made the discussed changes
|
@sallyom @Gregory-Pereira @JeffLuoo can you have a look to make sure this is aligned with your view? |
|
/lgtm Thanks for the work! |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: AtharvaPakade, nirrozenbaum The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
…ubernetes-sigs#2518) * feat(tracing): add tracing support and configuration options for BBR - add tracing package for BBR-specific instrumentation and helpers - update `body-based-routing` helm chart to support configuring tracing via OTEL env vars (service name, exporter endpoint, sampler, sampler arg, and exporter type) * refactor(tracing): make InitTracing generic and unify instrumentation scope names - accept `defaultServiceName` as a parameter in `InitTracing` instead of hardcoding it, allowing BBR and EPP to set their own default service names - remove the BBR-specific tracing package; tracer initialization is now handled by the common tracing package - use distinct instrumentation scope names per component (`/bbr/extproc`, `/epp/extproc`) with version and commit-sha attributes - update Helm chart defaults: disable tracing by default and set explicit default service name for BBR
…ubernetes-sigs/gateway-api-inference-extension#2518) * feat(tracing): add tracing support and configuration options for BBR - add tracing package for BBR-specific instrumentation and helpers - update `body-based-routing` helm chart to support configuring tracing via OTEL env vars (service name, exporter endpoint, sampler, sampler arg, and exporter type) * refactor(tracing): make InitTracing generic and unify instrumentation scope names - accept `defaultServiceName` as a parameter in `InitTracing` instead of hardcoding it, allowing BBR and EPP to set their own default service names - remove the BBR-specific tracing package; tracer initialization is now handled by the common tracing package - use distinct instrumentation scope names per component (`/bbr/extproc`, `/epp/extproc`) with version and commit-sha attributes - update Helm chart defaults: disable tracing by default and set explicit default service name for BBR
…ubernetes-sigs/gateway-api-inference-extension#2518) * feat(tracing): add tracing support and configuration options for BBR - add tracing package for BBR-specific instrumentation and helpers - update `body-based-routing` helm chart to support configuring tracing via OTEL env vars (service name, exporter endpoint, sampler, sampler arg, and exporter type) * refactor(tracing): make InitTracing generic and unify instrumentation scope names - accept `defaultServiceName` as a parameter in `InitTracing` instead of hardcoding it, allowing BBR and EPP to set their own default service names - remove the BBR-specific tracing package; tracer initialization is now handled by the common tracing package - use distinct instrumentation scope names per component (`/bbr/extproc`, `/epp/extproc`) with version and commit-sha attributes - update Helm chart defaults: disable tracing by default and set explicit default service name for BBR
What type of PR is this?
/kind feature
What this PR does / why we need it:
This PR introduces BBR-specific tracing support by adding a dedicated
pkg/bbr/tracingpackage.The motivation behind a component-specific tracing package (rather than directly using the shared common tracing utilities) is to avoid conflicts in InstrumentationScope names between BBR and EPP. Each component should own its tracer with a distinct instrumentation scope name (e.g., gateway-api-inference-extension/bbr/extproc), ensuring traces from BBR and EPP don't overlap and remain distinguishable in the backend. Additionally, having a dedicated package per component allows us to add component-specific helpers that can enrich spans with attributes relevant to that server, keeping the tracing logic modular and tailored to each component's context.
On the configuration side, the
body-based-routingHelm chart is updated to expose tracing options — enabling/disabling tracing, exporter endpoint, sampler, sampler arg, and service name — which get injected as OTEL environment variables into the BBR pod.Currently, only BBR has been updated. EPP will follow the same pattern in a future PR.
Which issue(s) this PR fixes:
Fixes #2327
Does this PR introduce a user-facing change?: