[traces] init the trace sdk #1638

Frapschen · 2025-09-23T09:08:30Z

What type of PR is this?
/kind feature

What this PR does / why we need it:
Set up the trace instrument, and update the manifest of inferencepool chart.

This is the initial PR for tracing, which simply sets up a global TracerProvider via an init function. Subsequent tracing-related PRs can then focus solely on adding spans:

ctx, span := tracer.Start(r.Context(), "hello-span")
defer span.End()

// do some work to track with hello-span

Which issue(s) this PR fixes:
issue: #1520

Does this PR introduce a user-facing change?:

Set up the trace instrument, and update the manifest of inferencepool chart

netlify · 2025-09-23T09:08:35Z

✅ Deploy Preview for gateway-api-inference-extension ready!

Name	Link
🔨 Latest commit	`81e1af7`
🔍 Latest deploy log	https://app.netlify.com/projects/gateway-api-inference-extension/deploys/68db5e29934b000008bf6f5b
😎 Deploy Preview	https://deploy-preview-1638--gateway-api-inference-extension.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

k8s-ci-robot · 2025-09-23T09:08:40Z

Hi @Frapschen. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions 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.

config/charts/inferencepool/README.md

cmd/epp/runner/runner.go

config/charts/inferencepool/README.md

config/charts/inferencepool/templates/epp-deployment.yaml

config/charts/inferencepool/values.yaml

pkg/common/traces.go

pkg/common/telemetry.go

pkg/common/traces.go

JeffLuoo

Just a nit for typo in logs to replace MeterProvider with TracerProvider and other LGTM. Will let @liu-cong @nirrozenbaum @ahg-g to take another review.

pkg/common/telemetry.go

ahg-g · 2025-09-25T18:15:47Z

/ok-to-test

Frapschen · 2025-09-26T00:46:51Z

The error is:

go: downloading github.com/x448/float16 v0.8.4
go: downloading golang.org/x/text v0.17.0
# golang.org/x/tools/internal/tokeninternal
../../../pkg/mod/golang.org/x/[email protected]/internal/tokeninternal/tokeninternal.go:64:9: invalid array length -delta * delta (constant -256 of type int64)
make: *** [Makefile:400: /home/prow/go/src/sigs.k8s.io/gateway-api-inference-extension/bin/controller-gen] Error 1
+ EXIT_VALUE=2
+ set +o xtrace
Cleaning up after docker in docker.
================================================================================
Waiting 30 seconds for pods stopped with terminationGracePeriod:30

The make test works fine on my Mac

JeffLuoo · 2025-09-26T14:07:30Z

/retest

config/charts/inferencepool/templates/_helpers.tpl

config/charts/inferencepool/values.yaml

cmd/epp/runner/runner.go

liu-cong

@JeffLuoo If the telemetry.go looks good to you, can you lgtm on that? I can lgtm on the flag and helm changes.

config/charts/inferencepool/values.yaml

liu-cong · 2025-09-29T20:20:05Z

config/charts/inferencepool/values.yaml

      enabled: false
+  trace:
+    enabled: false
+    otelExporterEndpoint: "http://localhost:4317"


Do we think it's "rare" for users to configre the endpoint and sampling params? If so, I suggest not including them here, and just let those rare, advanced users to configure via the env var section directly.

@liu-cong Most of the time, users should set a real OTel collector address, so I think it's a very useful setting.

And for sampling, it is also a common setting for tracing configuration.

+1 that the endpoint and the sampling rate are commonly configured fields.

config/charts/inferencepool/README.md

liu-cong

/lgtm

config/charts/inferencepool/README.md

liu-cong · 2025-09-30T03:57:48Z

pkg/common/telemetry.go

+	traceExporter, err := initTraceExporter(ctx, logger)
+	if err != nil {
+		loggerWrap.Handle(fmt.Errorf("%s: %v", "init trace exporter fail", err))
+		return nil


should this return an error?

liu-cong · 2025-09-30T03:59:28Z

/lgtm cancel

I have a question on the error return, left a comment

JeffLuoo

Just a few nits from my side. Other than that it LGTM. @liu-cong please give it another look.

JeffLuoo · 2025-09-30T14:05:42Z

pkg/common/telemetry.go

+	var traceExporter sdktrace.SpanExporter
+	traceExporter, err := stdouttrace.New(stdouttrace.WithPrettyPrint())
+	if err != nil {
+		return nil, fmt.Errorf("fail to create stdouttrace exporter: %w", err)


fail -> failed to align with other error messages in the same file.

JeffLuoo · 2025-09-30T14:07:06Z

pkg/common/telemetry.go

+	if exporterType == "otlp" {
+		traceExporter, err = otlptracegrpc.New(ctx, otlptracegrpc.WithInsecure())
+		if err != nil {
+			return nil, fmt.Errorf("fail to create otlp-grcp exporter: %w", err)


ditto: use failed

JeffLuoo · 2025-09-30T14:08:02Z

pkg/common/telemetry.go

+			loggerWrap.Handle(fmt.Errorf("%s: %v", "failed to shutdown TraceProvider", err))
+		}
+
+		logger.Info("trace provider shutting down")


Can you also use logging.DEFAULT for the info message?

JeffLuoo · 2025-09-30T14:08:26Z

pkg/common/telemetry.go

+
+	traceExporter, err := initTraceExporter(ctx, logger)
+	if err != nil {
+		loggerWrap.Handle(fmt.Errorf("%s: %v", "init trace exporter fail", err))


nit: init trace exporter failed

JeffLuoo · 2025-09-30T14:08:57Z

pkg/common/telemetry.go

+
+		sampler = sdktrace.ParentBased(sdktrace.TraceIDRatioBased(fraction))
+	} else {
+		loggerWrap.Handle(fmt.Errorf("un supported sampler type: %s, fallback to parentbased_traceidratio with 0.1 Ratio", samplerType))


nit: Unsupported without space.

k8s-ci-robot · 2025-09-30T14:09:40Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Frapschen, JeffLuoo
Once this PR has been reviewed and has the lgtm label, please assign danehans for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Sep 23, 2025

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 23, 2025

k8s-ci-robot requested review from ahg-g and robscott September 23, 2025 09:08

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 23, 2025

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 23, 2025

nirrozenbaum reviewed Sep 23, 2025

View reviewed changes

config/charts/inferencepool/README.md Outdated Show resolved Hide resolved

JeffLuoo reviewed Sep 23, 2025

View reviewed changes

JeffLuoo reviewed Sep 24, 2025

View reviewed changes

pkg/common/traces.go Outdated Show resolved Hide resolved

pkg/common/telemetry.go Show resolved Hide resolved

Frapschen force-pushed the add-trace-sdk branch from 10dfec1 to 65892cf Compare September 24, 2025 13:05

Frapschen requested a review from JeffLuoo September 24, 2025 13:21

JeffLuoo reviewed Sep 24, 2025

View reviewed changes

pkg/common/traces.go Outdated Show resolved Hide resolved

JeffLuoo approved these changes Sep 24, 2025

View reviewed changes

pkg/common/telemetry.go Show resolved Hide resolved

JeffLuoo reviewed Sep 24, 2025

View reviewed changes

pkg/common/telemetry.go Show resolved Hide resolved

Frapschen mentioned this pull request Sep 25, 2025

REQUEST: New membership for @Frapschen kubernetes/org#5862

Closed

11 tasks

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 25, 2025

Frapschen force-pushed the add-trace-sdk branch from 8bf120f to bb94492 Compare September 26, 2025 00:36

Frapschen added 3 commits September 26, 2025 22:20

init the trace sdk

56294ac

apply review's suggestion

b95f05a

update

d6b5ed6

Frapschen force-pushed the add-trace-sdk branch from bb94492 to d6b5ed6 Compare September 26, 2025 14:20

liu-cong reviewed Sep 26, 2025

View reviewed changes

config/charts/inferencepool/templates/_helpers.tpl Outdated Show resolved Hide resolved

config/charts/inferencepool/values.yaml Outdated Show resolved Hide resolved

cmd/epp/runner/runner.go Show resolved Hide resolved

Frapschen added 2 commits September 27, 2025 19:52

move otel env to helm template

3469c62

update

8cb7935

Frapschen added 3 commits September 27, 2025 20:19

skip lint

7caa0b1

fix lint

adb63c3

add file header

a8b406a

liu-cong reviewed Sep 29, 2025

View reviewed changes

Frapschen requested a review from liu-cong September 30, 2025 02:25

Frapschen force-pushed the add-trace-sdk branch from b3d90eb to b81d83e Compare September 30, 2025 03:41

liu-cong reviewed Sep 30, 2025

View reviewed changes

k8s-ci-robot assigned liu-cong Sep 30, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 30, 2025

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 30, 2025

Frapschen requested a review from liu-cong September 30, 2025 04:20

update README.md

81e1af7

Frapschen force-pushed the add-trace-sdk branch from 7cb9b3c to 81e1af7 Compare September 30, 2025 04:35

JeffLuoo approved these changes Sep 30, 2025

View reviewed changes

[traces] init the trace sdk #1638

Are you sure you want to change the base?

[traces] init the trace sdk #1638

Conversation

Frapschen commented Sep 23, 2025

Uh oh!

netlify bot commented Sep 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for gateway-api-inference-extension ready!

Uh oh!

k8s-ci-robot commented Sep 23, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JeffLuoo left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ahg-g commented Sep 25, 2025

Uh oh!

Frapschen commented Sep 26, 2025

Uh oh!

JeffLuoo commented Sep 26, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

liu-cong left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Frapschen Sep 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

liu-cong left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liu-cong commented Sep 30, 2025

Uh oh!

JeffLuoo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Sep 30, 2025

Uh oh!

Uh oh!

netlify bot commented Sep 23, 2025 •

edited

Loading

JeffLuoo left a comment •

edited

Loading

Frapschen Sep 30, 2025 •

edited

Loading