Skip to content

Conversation

johncalesp
Copy link
Contributor

I noticed that even if the flag OTEL_ENABLED is disabled, when you call tracer.start_as_current_span directly, it looks like otel gets enable with default params and tries to send the content to host='localhost', port=4318.
To fix this, I used nullcontext in case the flag is not enable and so we can avoid this error.
I tested on dlcluster and it works.

@johncalesp johncalesp requested a review from Copilot July 18, 2025 17:26
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes an issue where OpenTelemetry (OTEL) would initialize with default settings and attempt to send telemetry data to localhost:4318 even when the OTEL_ENABLED flag was disabled. The fix uses nullcontext() as a no-op context manager when telemetry is disabled, preventing unwanted OTEL initialization and network calls.

Key changes:

  • Added conditional telemetry context managers that use nullcontext() when OTEL_ENABLED is false
  • Added null checks before calling span methods to prevent errors when spans are None
  • Ensured telemetry operations are properly gated behind the OTEL_ENABLED environment variable

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
src/flexible_inference_benchmark/main.py Updated main experiment span creation to use conditional context manager and added null check for span attribute setting
src/flexible_inference_benchmark/engine/backend_functions.py Modified all OpenTelemetry span creation to be conditional and added null checks for span method calls throughout the request processing

Copy link
Member

@hjjq hjjq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!!

@johncalesp johncalesp merged commit 5993940 into main Jul 18, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants