env var controlled #1532
env var controlled #1532
Conversation
ishandhanani
requested review from
ryanolson,
grahamking,
paulhendricks,
biswapanda,
tmonty12,
GuanLuo,
rmccorm4,
alec-flowers,
kkranen,
oandreeva-nv,
jthomson04,
PeaBrane and
a team
as code owners
WalkthroughThis change updates the filtering logic for LLM metric annotation events in the SSE response stream. Now, whether these events are included or excluded depends on the presence of the DYN_RICH_EVENT_STREAM environment variable, introducing a runtime flag to control their visibility. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SSE_Handler
participant Env
Client->>SSE_Handler: Request SSE stream
SSE_Handler->>Env: Check DYN_RICH_EVENT_STREAM
alt Env var is set
SSE_Handler-->>Client: Include LLM metric annotation events
else Env var is not set
SSE_Handler-->>Client: Exclude LLM metric annotation events
end
Possibly related PRs
Poem
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/llm/src/http/service/openai.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: pre-merge-rust (lib/bindings/python)
- GitHub Check: pre-merge-rust (lib/runtime/examples)
- GitHub Check: pre-merge-rust (.)
- GitHub Check: Build and Test - vllm
| // Only filter out metrics if DYN_RICH_EVENT_STREAM is not set | ||
| if std::env::var("DYN_RICH_EVENT_STREAM").is_err() | ||
| && annotated.event.as_deref() == Some(crate::preprocessor::ANNOTATION_LLM_METRICS) | ||
| { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Repeated env-var look-up & unclear semantics – cache once and parse a boolean
std::env::var("DYN_RICH_EVENT_STREAM") is executed for every single SSE chunk.
Although an env fetch is cheap, the lock and Unicode conversion still add avoidable overhead on high-QPS streams.
More importantly, the logic treats the variable as a mere presence flag: any value (even "0" or "false") enables rich events. That is surprising for operators who may try to disable it with "0".
Proposed refactor:
- // Only filter out metrics if DYN_RICH_EVENT_STREAM is not set
- if std::env::var("DYN_RICH_EVENT_STREAM").is_err()
+ // Only filter out metrics when the feature flag is disabled
+ static ENABLE_RICH_STREAM: once_cell::sync::Lazy<bool> =
+ once_cell::sync::Lazy::new(|| {
+ matches!(
+ std::env::var("DYN_RICH_EVENT_STREAM")
+ .ok()
+ .as_deref()
+ .map(str::to_ascii_lowercase)
+ .as_deref(),
+ Some("1" | "true" | "yes")
+ )
+ });
+
+ if !*ENABLE_RICH_STREAM
&& annotated.event.as_deref() == Some(crate::preprocessor::ANNOTATION_LLM_METRICS)
{Benefits
• Caches the flag once (no per-event cost).
• Allows intuitive "true"/"false" values.
• Keeps default behaviour unchanged (unset ⇒ metrics filtered out).
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In lib/llm/src/http/service/openai.rs around lines 509 to 512, the environment
variable DYN_RICH_EVENT_STREAM is queried repeatedly for every SSE chunk and
treated as a presence flag, which causes unnecessary overhead and unclear
semantics. Fix this by reading and parsing the environment variable once at
initialization into a boolean flag that interprets "true"/"false" strings
correctly, then use this cached boolean in the conditional to decide whether to
filter metrics, preserving the default behavior when the variable is unset.
Overview:
Details:
Where should the reviewer start?
Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Summary by CodeRabbit