Skip to content

fix(sglang): remove otel for emebedding models#5100

Merged
ishandhanani merged 1 commit intomainfrom
ishan/fix-5764561
Dec 28, 2025
Merged

fix(sglang): remove otel for emebedding models#5100
ishandhanani merged 1 commit intomainfrom
ishan/fix-5764561

Conversation

@ishandhanani
Copy link
Contributor

@ishandhanani ishandhanani commented Dec 27, 2025

Based on sgl-project/sglang#15814 (comment) I'm realizing that embedding doesn't have proper tracing support. The tracing PIC in SGL will implement it in a different PR. Solution here is to not enable tracing support for embedding

Summary by CodeRabbit

  • Chores
    • Removed the --enable-otel flag and related OpenTelemetry configuration from the embedding script. OpenTelemetry tracing is not yet supported for embedding models.

✏️ Tip: You can customize this high-level summary in your review settings.

@ishandhanani ishandhanani requested review from a team as code owners December 27, 2025 19:22
@github-actions github-actions bot added the fix label Dec 27, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 27, 2025

Walkthrough

The script removes all OpenTelemetry tracing support. It eliminates the --enable-otel flag parsing, OTEL environment variable exports, trace configuration initialization, and related help documentation. The embedding worker startup line is simplified to exclude all trace-related arguments.

Changes

Cohort / File(s) Summary
OpenTelemetry removal
examples/backends/sglang/launch/agg_embed.sh
Removed --enable-otel flag support, ENABLE_OTEL parsing, TRACE_ARGS initialization, and all OTEL-related environment variables. Updated help text to note that OpenTelemetry tracing is not yet supported for embedding models. Simplified worker startup to exclude trace configuration.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 A hop through the traces, we skip them away,
No telemetry follows the embedding's pathway,
Simpler the startup, cleaner the run,
When tracing is removed, the script weighs less—what fun! 🎉

Pre-merge checks

❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Description check ❓ Inconclusive The description explains the rationale and references the upstream discussion, but lacks the structured format specified in the template with missing sections like 'Details', 'Where should the reviewer start', and formal issue closure syntax. Expand the description to follow the template structure: add a 'Details' section explaining the changes, specify 'Where should the reviewer start', and use proper issue closure syntax (e.g., 'Closes #5764561' if applicable).
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the main change: removing OTEL support from embedding models in sglang, which aligns with the changeset.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7053bb2 and aa6f5cc.

📒 Files selected for processing (1)
  • examples/backends/sglang/launch/agg_embed.sh
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Build and Test - dynamo
🔇 Additional comments (2)
examples/backends/sglang/launch/agg_embed.sh (2)

23-23: LGTM! Clear documentation of tracing limitation.

The note clearly communicates that OpenTelemetry tracing is not yet supported for embedding models, which aligns with the PR objectives and references the upstream discussion.


40-49: Worker startup correctly simplified with metrics-only configuration.

The embedding worker startup has been properly configured with only --enable-metrics enabled. OTEL tracing is intentionally disabled for embedding models, as documented in the help text (line 23). No --enable-otel flag or OTEL environment variables are present in agg_embed.sh, which is the correct approach distinct from other sglang launcher scripts. The configuration is referenced in tests and documentation without OTEL expectations.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@ishandhanani ishandhanani merged commit 036eb4f into main Dec 28, 2025
29 checks passed
@ishandhanani ishandhanani deleted the ishan/fix-5764561 branch December 28, 2025 04:56
jh-nv pushed a commit that referenced this pull request Jan 2, 2026
Signed-off-by: Jie Hao <jihao@nvidia.com>
@dagil-nvidia
Copy link
Collaborator

Auto-linked to DIS-1228

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants