feat(observability): add fine-grained OTel spans for the plugin pipeline and model selector by gyliu513 · Pull Request #165 · llm-d/llm-d-inference-payload-processor

gyliu513 · 2026-06-13T17:14:25Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Adds fine-grained OpenTelemetry spans for IPP's request/response plugin pipeline and the model-selector pipeline (filters → scorers → picker), so a single sampled request shows which plugins ran, in what order, and how long each took, instead of one opaque gateway.request span.

Until now the entire request lifecycle produced exactly one span.

Which issue(s) this PR fixes:

Fixes #163

Release note (write NONE if no user-facing change):

Adds fine-grained OpenTelemetry spans for IPP's request/response plugin pipeline and the model-selector pipeline

gyliu513 · 2026-06-13T17:14:35Z

/cc @nirrozenbaum

nirrozenbaum · 2026-06-14T12:13:12Z

+	defer stageSpan.End()
+
 	for _, reqPlugin := range reqPlugins {
+		name := reqPlugin.TypedName()


nit: maybe better to name this typedName?
it reads a bit strange name.Type and name.Name.

Suggested change

name := reqPlugin.TypedName()

typedName := reqPlugin.TypedName()

Done, renamed to typedName here and in the response/filter/scorer/picker loops for consistency. Thanks.

nirrozenbaum · 2026-06-14T12:14:42Z

+		pluginCtx, span := tracer.Start(ctx, "plugin."+name.Type,
+			trace.WithSpanKind(trace.SpanKindInternal),
+			trace.WithAttributes(
+				attribute.String("llm_d.plugin.extension_point", requestPluginExtensionPoint),
+				attribute.String("llm_d.plugin.type", name.Type),
+				attribute.String("llm_d.plugin.name", name.Name),
+			))


I'm not a tracing expert so forgive me if the question is obvious..
why do we specify type twice?
once in plugin.name.Type and another one in attribute?

Good question, it's intentional.

The span name (plugin.type)is the human-readable label shown in trace UIs; it's meant to be low-cardinality and isn't reliably queryable across tracing backends.

The llm_d.plugin.type attribute is the structured, indexable dimension you filter and aggregate on (e.g. "p99 latency grouped by plugin type"). Per OTel conventions, the span name is for display and attributes carry the queryable data, so keeping both is by design.

nirrozenbaum · 2026-06-14T12:17:53Z

+	defer stageSpan.End()
+
 	for _, respPlugin := range respPlugins {
+		name := respPlugin.TypedName()


ditto, typedName

nirrozenbaum · 2026-06-14T12:18:51Z


+	tracer := tracing.Tracer(modelSelectorTracerScope)
 	for _, filter := range p.filters {
+		name := filter.TypedName()


nirrozenbaum · 2026-06-14T12:23:25Z

+	if result != nil && result.TargetModel != nil {
+		span.SetAttributes(attribute.String("llm_d.picker.selected_model", result.TargetModel.GetName()))
+	}


Picker is guaranteed to select a target model. we can remove the conditional.

Suggested change

if result != nil && result.TargetModel != nil {

span.SetAttributes(attribute.String("llm_d.picker.selected_model", result.TargetModel.GetName()))

}

span.SetAttributes(attribute.String("llm_d.picker.selected_model", result.TargetModel.GetName()))

nirrozenbaum · 2026-06-14T12:25:54Z

+			span.RecordError(err)
+			span.SetStatus(codes.Error, err.Error())
+		} else if result != nil && result.TargetModel != nil {
+			span.SetAttributes(attribute.String("llm_d.model_selector.selected_model", result.TargetModel.GetName()))


this would fit better before the log line Model selection completed. without the conditionals.

nirrozenbaum

@gyliu513 thanks, overall looks good.
left few minor comments.

gyliu513

Thanks @nirrozenbaum

gyliu513 · 2026-06-14T12:52:05Z

+	defer stageSpan.End()
+
 	for _, reqPlugin := range reqPlugins {
+		name := reqPlugin.TypedName()


Done, renamed to typedName here and in the response/filter/scorer/picker loops for consistency. Thanks.

gyliu513 · 2026-06-14T12:52:56Z

+		pluginCtx, span := tracer.Start(ctx, "plugin."+name.Type,
+			trace.WithSpanKind(trace.SpanKindInternal),
+			trace.WithAttributes(
+				attribute.String("llm_d.plugin.extension_point", requestPluginExtensionPoint),
+				attribute.String("llm_d.plugin.type", name.Type),
+				attribute.String("llm_d.plugin.name", name.Name),
+			))


Good question, it's intentional.

The span name (plugin.type)is the human-readable label shown in trace UIs; it's meant to be low-cardinality and isn't reliably queryable across tracing backends.

The llm_d.plugin.type attribute is the structured, indexable dimension you filter and aggregate on (e.g. "p99 latency grouped by plugin type"). Per OTel conventions, the span name is for display and attributes carry the queryable data, so keeping both is by design.

gyliu513 · 2026-06-14T12:53:20Z

+	defer stageSpan.End()
+
 	for _, respPlugin := range respPlugins {
+		name := respPlugin.TypedName()


gyliu513 · 2026-06-14T12:53:33Z


+	tracer := tracing.Tracer(modelSelectorTracerScope)
 	for _, filter := range p.filters {
+		name := filter.TypedName()


gyliu513 · 2026-06-14T12:54:16Z

+			span.RecordError(err)
+			span.SetStatus(codes.Error, err.Error())
+		} else if result != nil && result.TargetModel != nil {
+			span.SetAttributes(attribute.String("llm_d.model_selector.selected_model", result.TargetModel.GetName()))


gyliu513 · 2026-06-14T14:05:26Z

+	if result != nil && result.TargetModel != nil {
+		span.SetAttributes(attribute.String("llm_d.picker.selected_model", result.TargetModel.GetName()))
+	}


shaneutt · 2026-06-15T18:06:47Z

+		span.End()
 	}

 	return nil


Seems that we may want to add more test coverage for these changes:

We need a test that does this:

Set up a tracetest.SpanRecorder (as done in telemetry_test.go).

Run runRequestPlugins with one or more fake plugins.

Assert that a request_plugins stage span is created with child plugin.* spans.

Assert that a failing plugin produces a span with codes.Error status.

If I'm not mistaken, I don't believe we have that currently which would leave an opportunity for regressions.

I'll add a runRequestPlugins test using a tracetest.SpanRecorder (mirroring telemetry_test.go) with fake plugins that asserts the request_plugins stage span, nested plugin.* child spans, and a codes.Error status when a plugin fails.

shaneutt · 2026-06-15T18:09:01Z

Should we now use the new tracing.Tracer helper?

Yes, I'll fix it. It was merged after I created this PR.

shaneutt · 2026-06-15T18:09:12Z

A new constant for this was added above but we're not using it here yet.

Yes, I'll fix it. It was merged after I created this PR.

shaneutt · 2026-06-15T18:11:29Z

 	before := time.Now()
-	result := p.picker.Pick(ctx, cycleState, scoredModels)
-	metrics.RecordPluginProcessingLatency(pickerExtensionPoint, p.picker.TypedName().Type, p.picker.TypedName().Name, time.Since(before))
+	result := p.picker.Pick(spanCtx, cycleState, scoredModels)


It appears that this call may be able to return a nil value for TargetModel?

If it does we will panic on GetName() below.

Hi @shaneutt , @nirrozenbaum post a comment here and I agree that the Picker is guaranteed to select a target model, do we still need the guard?

shaneutt · 2026-06-15T18:11:37Z

+		debugLogger.Info("Completed running picker plugin", "plugin", typedName, "result", result)
 	}

 	return result


I think we're in a situation with this one, as before, where test coverage needs expansion.

Yes, will add a test case

shaneutt · 2026-06-15T18:11:53Z

 		return nil, err
 	}

+	span.SetAttributes(attribute.String("llm_d.model_selector.selected_model", result.TargetModel.GetName()))


Similar to above: no nil guards

This one is already safe — lines 91-94 guard immediately

shmuelk · 2026-06-18T13:34:43Z


+// instrumentationName is the default OTel instrumentation scope used when no
+// explicit scope is supplied to Tracer.
+const instrumentationName = "llm-d-inference-payload-processor"


I recommend shortning the name. How about:

Suggested change

const instrumentationName = "llm-d-inference-payload-processor"

const instrumentationName = "llm-d-ipp"

shmuelk · 2026-06-18T13:35:10Z

+const instrumentationName = "llm-d-inference-payload-processor"
+
+// Tracer returns a tracer for the given instrumentation scope, defaulting to
+// "llm-d-inference-payload-processor". The build version and commit SHA are


Suggested change

// "llm-d-inference-payload-processor". The build version and commit SHA are

// "llm-d-ipp". The build version and commit SHA are

gyliu513 · 2026-06-18T16:17:07Z

+
+	// handlersTracerScope is the OTel instrumentation scope for spans emitted by
+	// the request/response handlers, following the package-path naming convention.
+	handlersTracerScope = "llm-d-inference-payload-processor/pkg/handlers"


@shmuelk with your above comment, do we need to update here as well to

Suggested change

handlersTracerScope = "llm-d-inference-payload-processor/pkg/handlers"

handlersTracerScope = "llm-d-ipp/pkg/handlers"

/cc @nirrozenbaum @shaneutt

gyliu513 · 2026-06-18T16:17:48Z

+	// modelSelectorTracerScope is the OTel instrumentation scope for spans
+	// emitted by the model-selector pipeline, following the package-path
+	// naming convention.
+	modelSelectorTracerScope = "llm-d-inference-payload-processor/pkg/modelselector"


@shmuelk ditto here, how about

Suggested change

modelSelectorTracerScope = "llm-d-inference-payload-processor/pkg/modelselector"

modelSelectorTracerScope = "llm-d-ipp/pkg/modelselector"

gyliu513 · 2026-06-25T20:08:07Z

@shmuelk @shaneutt @nirrozenbaum can you help review?

github-actions · 2026-07-17T01:35:54Z

This PR is marked as stale after 21d of inactivity. After an additional 14d of inactivity (7d to become rotten, then 7d more), it will be closed. To prevent this PR from being closed, add a comment or remove the lifecycle/stale label.

gyliu513 · 2026-07-22T13:34:40Z

@shmuelk @shaneutt @nirrozenbaum can you help review? I think all comments are addressed.

shmuelk · 2026-07-28T11:09:27Z

A similar PR is being discussed in the ll-d-router repo.Some, including myself think this very low level tracing is too much.

All of my requested changes have been made.

From my perspective this PR can be merged if the functionality is desired and not deemed to be too much.

gyliu513 · 2026-07-28T13:36:49Z

@shmuelk I was talking with @elevran at llm-d/llm-d-router#1834, but I think we should be good to merge this.

gyliu513 · 2026-07-31T15:34:20Z

@shmuelk @nirrozenbaum can we get this merged? I can do some performance test later

gyliu513 · 2026-08-04T15:02:37Z

@shmuelk llm-d/llm-d-router#1834 has been merged, can you help merge this as well? I want to have this in next release, thanks!

github-actions · 2026-08-26T01:31:08Z

This PR is marked as stale after 21d of inactivity. After an additional 14d of inactivity (7d to become rotten, then 7d more), it will be closed. To prevent this PR from being closed, add a comment or remove the lifecycle/stale label.

…ine and model selector Signed-off-by: Guangya Liu <gyliu513@gmail.com>

Signed-off-by: Guangya Liu <gyliu513@gmail.com>

gyliu513 · 2026-08-28T02:26:27Z

Hey @nirrozenbaum , I think this can be merged, can you help check?

nirrozenbaum

@gyliu513 thank you for the PR.
left some comments/questions

nirrozenbaum · 2026-08-31T07:30:20Z

-			logger.Error(err, "Failed to execute request plugin", "plugin", reqPlugin.TypedName())
+			span.RecordError(err)
+			span.SetStatus(codes.Error, err.Error())
+			span.End()


nit: can we have

defer span.End()

only once after we create it in L177 instead of having writing it twice?

Good catch on the duplication, but a plain defer span.End() right after tracer.Start() here wouldn't do what we want, since the span is created inside the for loop body of this function (not a per-iteration helper), the defer would only fire when runRequestPlugins returns, not per plugin, so all the per-plugin spans would stay open until the loop finishes and get bogus end-times. llm-d-router's runScorer (scheduler_profile.go:288-289) hits the same shape and solves it by extracting the per-plugin span+call into its own function so the defer scopes correctly per call. I can refactor runRequestPlugins/runResponsePlugins the same way if you'd like, happy to do that as a follow-up in this PR.

yeah extracting to a helper function would make it nicer :)

nirrozenbaum · 2026-08-31T07:31:01Z

-			logger.Error(err, "Failed to execute response plugin", "plugin", respPlugin.TypedName())
+			span.RecordError(err)
+			span.SetStatus(codes.Error, err.Error())
+			span.End()


same as above reply

nirrozenbaum · 2026-08-31T07:33:50Z

-			verboseLogger.Info("Running filter plugin", "plugin", filter.TypedName())
+			verboseLogger.Info("Running filter plugin", "plugin", typedName)
 		}
+		spanCtx, span := tracer.Start(ctx, "plugin."+typedName.Type,


what happens if there are multiple plugins from the same type?
isn't it better to have `plugin. +typedName.String()' to include both type and name here?

This mirrors the convention llm-d-router uses for its scorer spans (scheduler_profile.go:288), span name carries just the Type for low cardinality, and both llm_d.plugin.type/llm_d.plugin.name are separate attributes for the actual instance. If two plugin instances share a Type, their spans will look identical by name in the waterfall, but the attributes (visible on hover in Jaeger/Tempo/etc.) disambiguate them. Keeping it consistent with llm-d-router's existing pattern rather than introducing a different naming scheme here, but open to revisiting org-wide if you feel disambiguation-by-name is worth the cardinality cost.

I'm not a tracing expert, but I think it might be worth considering revisiting this point.
looking on a waterfall of spans, it should be clear what each refers to without having to drill down.
e.g., if we see multiple spans like this there's no way to understand without drilling down:
https://github.com/llm-d/llm-d-router/blob/db17418ade6cd5a15c6bbb04c9d0f0089c37034f/pkg/epp/framework/plugins/scheduling/scorer/endpointattribute/endpointattribute.go#L32

nirrozenbaum · 2026-08-31T07:34:36Z

-			verboseLogger.Info("Running scorer plugin", "plugin", scorer.TypedName())
+			verboseLogger.Info("Running scorer plugin", "plugin", typedName)
 		}
+		spanCtx, span := tracer.Start(ctx, "plugin."+typedName.Type,


nirrozenbaum · 2026-08-31T07:36:03Z

+			trace.WithAttributes(
+				attribute.String("llm_d.plugin.extension_point", scorerExtensionPoint),
+				attribute.String("llm_d.plugin.type", typedName.Type),
+				attribute.String("llm_d.plugin.name", typedName.Name),
+				attribute.Int("llm_d.scorer.candidate_count", len(models)),
+				attribute.Float64("llm_d.scorer.weight", scorer.Weight()),


I think this should come after the Score function and include the score as well.
ideally one would like to see in the traces the scorer typed name, the score and and the weight.
score is missing cause it's too early.

Agreed, this is worth fixing. llm-d-router (scheduler_profile.go:297-316) and llm-d-kv-cache (traced_scorer.go:60-82) both do this: span.SetAttributes(...) right after Score() returns, recording aggregate score.max/score.avg (not per-model, to keep cardinality bounded). I'll follow the same pattern here — add llm_d.scorer.score.max/llm_d.scorer.score.avg computed from scores right before span.End().

apologize for picking to this point.
I'm not sure max/avg gives us information that's really helpful in terms of observability.
I was thinking on a more detailed option, something like creating a new span scores and then have a entry with two attributes - target and score (and I would recommend doing the same in llm-d-router).
that could allow us to drill into a specific request and understand what was the score for each of the models/endpoints, the span one level up can specify the selected target.
in case max-picker is used, then obviously it's the target with max score, but in case a different picker is used, it's not obvious which would get selected.

Signed-off-by: Guangya Liu <gyliu513@gmail.com>

github-actions Bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. kind/feature Categorizes issue or PR as related to a new feature. labels Jun 13, 2026

github-actions Bot requested a review from nirrozenbaum June 13, 2026 17:14

nirrozenbaum reviewed Jun 14, 2026

View reviewed changes

gyliu513 commented Jun 14, 2026

View reviewed changes

gyliu513 force-pushed the feat/issue-163-pipeline-spans branch from b367c50 to ea46ec9 Compare June 14, 2026 14:08

shaneutt reviewed Jun 15, 2026

View reviewed changes

shmuelk reviewed Jun 18, 2026

View reviewed changes

gyliu513 commented Jun 18, 2026

View reviewed changes

gyliu513 force-pushed the feat/issue-163-pipeline-spans branch from bacd262 to 5b3594e Compare June 22, 2026 15:02

github-actions Bot added the lifecycle/stale label Jul 17, 2026

gyliu513 force-pushed the feat/issue-163-pipeline-spans branch from 5b3594e to cc6fe05 Compare July 22, 2026 13:34

github-actions Bot removed the lifecycle/stale label Jul 22, 2026

gyliu513 force-pushed the feat/issue-163-pipeline-spans branch 2 times, most recently from 56cbcd0 to 293942a Compare July 27, 2026 02:52

gyliu513 force-pushed the feat/issue-163-pipeline-spans branch from 293942a to 2e6cff8 Compare July 31, 2026 15:30

gyliu513 mentioned this pull request Aug 13, 2026

llm-d SIG-Observability: open work inventory llm-d/llm-d#2269

Open

github-actions Bot added the lifecycle/stale label Aug 26, 2026

gyliu513 added 3 commits August 27, 2026 22:25

feat(observability): add fine-grained OTel spans for the plugin pipel…

292ac32

…ine and model selector Signed-off-by: Guangya Liu <gyliu513@gmail.com>

address comments from Nir

6eb73f4

Signed-off-by: Guangya Liu <gyliu513@gmail.com>

address comments from shaneutt

b603127

Signed-off-by: Guangya Liu <gyliu513@gmail.com>

gyliu513 force-pushed the feat/issue-163-pipeline-spans branch from 2e6cff8 to b603127 Compare August 28, 2026 02:25

github-actions Bot removed the lifecycle/stale label Aug 28, 2026

nirrozenbaum reviewed Aug 31, 2026

View reviewed changes

adding score to span

6485152

Signed-off-by: Guangya Liu <gyliu513@gmail.com>

	name := reqPlugin.TypedName()
	typedName := reqPlugin.TypedName()

	const instrumentationName = "llm-d-inference-payload-processor"
	const instrumentationName = "llm-d-ipp"

	// "llm-d-inference-payload-processor". The build version and commit SHA are
	// "llm-d-ipp". The build version and commit SHA are

	handlersTracerScope = "llm-d-inference-payload-processor/pkg/handlers"
	handlersTracerScope = "llm-d-ipp/pkg/handlers"

	modelSelectorTracerScope = "llm-d-inference-payload-processor/pkg/modelselector"
	modelSelectorTracerScope = "llm-d-ipp/pkg/modelselector"

Uh oh!

Conversation

gyliu513 commented Jun 13, 2026

Uh oh!

gyliu513 commented Jun 13, 2026

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!

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!

Choose a reason for hiding this comment

Uh oh!

nirrozenbaum left a comment

Choose a reason for hiding this comment

Uh oh!

gyliu513 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gyliu513 Jun 15, 2026 • 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!

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!

gyliu513 Jun 15, 2026 •

edited

Loading