Clarify OTel config#8
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThe PR renames OpenTelemetry-related flags ( Changes
Sequence Diagram(s)(omitted — changes are renames and environment-variable handling tweaks; not a new multi-component flow that requires sequence visualization) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
leap0/_sync/client.py (1)
83-93:⚠️ Potential issue | 🟠 MajorPublic constructor rename introduces a breaking API change without a shim.
Changing
otel_enabledtosdk_otel_enabledat Line 92 breaks existing callers immediately. Consider keeping a deprecated alias for one release window.♻️ Backward-compatible transition pattern
def __init__( self, *, api_key: str | None = None, base_url: str | None = None, sandbox_domain: str | None = None, timeout: float = DEFAULT_CLIENT_TIMEOUT, auth_header: str = "authorization", bearer: bool = True, sdk_otel_enabled: bool | None = None, + otel_enabled: bool | None = None, ): + if sdk_otel_enabled is not None and otel_enabled is not None: + raise ValueError("Pass only one of sdk_otel_enabled or otel_enabled") + if sdk_otel_enabled is None: + sdk_otel_enabled = otel_enabled + config = Leap0Config( api_key=api_key, base_url=base_url, sandbox_domain=sandbox_domain, timeout=timeout, auth_header=auth_header, bearer=bearer, sdk_otel_enabled=sdk_otel_enabled, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@leap0/_sync/client.py` around lines 83 - 93, The constructor parameter rename from otel_enabled to sdk_otel_enabled in __init__ breaks callers; restore a backward-compatible shim by adding an optional otel_enabled: bool | None = None parameter to __init__, and inside the method map otel_enabled to sdk_otel_enabled when sdk_otel_enabled is None (prefer sdk_otel_enabled if both provided), emit a DeprecationWarning (via the warnings module) when otel_enabled is used, and keep any docstring/typing updated to mention the deprecation so callers have a one-release transition window.leap0/_sync/sandbox.py (1)
179-188:⚠️ Potential issue | 🟠 Major
create(..., telemetry=...)callers will break after this rename.Renaming to
otel_exportis clear, but removingtelemetrywithout a compatibility alias is a breaking API change for existing SDK users.♻️ Backward-compatible transition pattern
def create( self, *, template_name: str = DEFAULT_TEMPLATE_NAME, vcpu: int = DEFAULT_VCPU, memory_mib: int = DEFAULT_MEMORY_MIB, timeout_min: int = DEFAULT_TIMEOUT_MIN, auto_pause: bool = False, otel_export: bool = False, + telemetry: bool | None = None, env_vars: dict[str, str] | None = None, network_policy: NetworkPolicyDict | None = None, http_timeout: float | None = None, ) -> SandboxT | SandboxData | SandboxStatus: + if telemetry is not None: + if otel_export is not False: + raise ValueError("Pass only one of otel_export or telemetry") + otel_export = telemetry🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@leap0/_sync/sandbox.py` around lines 179 - 188, The public method create has been renamed parameter telemetry -> otel_export which breaks callers; restore backward compatibility by accepting a telemetry parameter (e.g., telemetry: bool | None = None) alongside otel_export in create and resolve the effective flag by preferring the explicit otel_export if provided, otherwise using telemetry when telemetry is not None; update any internal uses in create to use the resolved boolean and mark telemetry as deprecated in the docstring so callers using telemetry continue to work while encouraging otel_export.
🧹 Nitpick comments (2)
tests/_sync/test_client_config.py (1)
24-31: Add a precedence test for explicit SDK flag disable.Please add coverage for
LEAP0_SDK_OTEL_ENABLED="false"+OTEL_EXPORTER_OTLP_ENDPOINTset, to ensure explicit disable is not overridden by endpoint fallback.🧪 Suggested test
+def test_sdk_otel_enabled_env_false_overrides_endpoint(monkeypatch): + monkeypatch.setenv("LEAP0_API_KEY", "test-key") + monkeypatch.setenv("LEAP0_SDK_OTEL_ENABLED", "false") + monkeypatch.setenv("OTEL_EXPORTER_OTLP_ENDPOINT", "http://localhost:4318") + + config = Leap0Config() + + assert config.sdk_otel_enabled is False🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/_sync/test_client_config.py` around lines 24 - 31, Add a new test (e.g., test_explicit_sdk_flag_precedence) in tests/_sync/test_client_config.py that sets LEAP0_API_KEY, sets LEAP0_SDK_OTEL_ENABLED="false", sets OTEL_EXPORTER_OTLP_ENDPOINT to a non-empty value via monkeypatch, constructs Leap0Config(), and asserts that config.sdk_otel_enabled is False to verify the explicit SDK disable flag takes precedence over endpoint fallback.leap0/_async/sandbox.py (1)
142-152: Minor redundancy:endpointis fetched twice.The
endpointvalue is fetched on line 143 for validation, then re-fetched on line 148 as part of the_OTEL_ENV_KEYSiteration. You could reuse the already-fetched value.♻️ Optional refactor to avoid redundant environment lookup
def _inject_otel_env(env_vars: dict[str, str] | None) -> dict[str, str] | None: endpoint = os.environ.get("OTEL_EXPORTER_OTLP_ENDPOINT") if not endpoint: raise ValueError( "otel_export=True requires OTEL_EXPORTER_OTLP_ENDPOINT in the local environment" ) - otel = {k: v for k in _OTEL_ENV_KEYS if (v := os.environ.get(k))} + otel = {"OTEL_EXPORTER_OTLP_ENDPOINT": endpoint} + if headers := os.environ.get("OTEL_EXPORTER_OTLP_HEADERS"): + otel["OTEL_EXPORTER_OTLP_HEADERS"] = headers merged = dict(otel) if env_vars: merged.update(env_vars) return merged🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@leap0/_async/sandbox.py` around lines 142 - 152, The function _inject_otel_env fetches OTEL_EXPORTER_OTLP_ENDPOINT into local variable endpoint for validation but then redundantly re-reads it when building otel from _OTEL_ENV_KEYS; modify _inject_otel_env to reuse the already-read endpoint value by ensuring OTEL_EXPORTER_OTLP_ENDPOINT is included in the otel mapping (e.g., add {"OTEL_EXPORTER_OTLP_ENDPOINT": endpoint} or prefer reading from the existing endpoint variable when constructing otel) so you avoid a second os.environ.get call while preserving current behavior of merging env_vars into merged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@leap0/models/config.py`:
- Around line 68-70: The current logic in config.py for self.sdk_otel_enabled
treats LEAP0_SDK_OTEL_ENABLED == "true" OR presence of
OTEL_EXPORTER_OTLP_ENDPOINT, which means an explicit "false" is ignored when
OTLP endpoint exists; change it so the LEAP0_SDK_OTEL_ENABLED env var wins when
set: check os.environ.get("LEAP0_SDK_OTEL_ENABLED") first (detecting presence
and interpreting "true"/"false" via .lower() == "true"), and only if that env
var is None/empty fall back to using
bool(os.environ.get("OTEL_EXPORTER_OTLP_ENDPOINT")); update the assignment that
references self.sdk_otel_enabled to use this precedence.
---
Outside diff comments:
In `@leap0/_sync/client.py`:
- Around line 83-93: The constructor parameter rename from otel_enabled to
sdk_otel_enabled in __init__ breaks callers; restore a backward-compatible shim
by adding an optional otel_enabled: bool | None = None parameter to __init__,
and inside the method map otel_enabled to sdk_otel_enabled when sdk_otel_enabled
is None (prefer sdk_otel_enabled if both provided), emit a DeprecationWarning
(via the warnings module) when otel_enabled is used, and keep any
docstring/typing updated to mention the deprecation so callers have a
one-release transition window.
In `@leap0/_sync/sandbox.py`:
- Around line 179-188: The public method create has been renamed parameter
telemetry -> otel_export which breaks callers; restore backward compatibility by
accepting a telemetry parameter (e.g., telemetry: bool | None = None) alongside
otel_export in create and resolve the effective flag by preferring the explicit
otel_export if provided, otherwise using telemetry when telemetry is not None;
update any internal uses in create to use the resolved boolean and mark
telemetry as deprecated in the docstring so callers using telemetry continue to
work while encouraging otel_export.
---
Nitpick comments:
In `@leap0/_async/sandbox.py`:
- Around line 142-152: The function _inject_otel_env fetches
OTEL_EXPORTER_OTLP_ENDPOINT into local variable endpoint for validation but then
redundantly re-reads it when building otel from _OTEL_ENV_KEYS; modify
_inject_otel_env to reuse the already-read endpoint value by ensuring
OTEL_EXPORTER_OTLP_ENDPOINT is included in the otel mapping (e.g., add
{"OTEL_EXPORTER_OTLP_ENDPOINT": endpoint} or prefer reading from the existing
endpoint variable when constructing otel) so you avoid a second os.environ.get
call while preserving current behavior of merging env_vars into merged.
In `@tests/_sync/test_client_config.py`:
- Around line 24-31: Add a new test (e.g., test_explicit_sdk_flag_precedence) in
tests/_sync/test_client_config.py that sets LEAP0_API_KEY, sets
LEAP0_SDK_OTEL_ENABLED="false", sets OTEL_EXPORTER_OTLP_ENDPOINT to a non-empty
value via monkeypatch, constructs Leap0Config(), and asserts that
config.sdk_otel_enabled is False to verify the explicit SDK disable flag takes
precedence over endpoint fallback.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 065f1070-b36a-421b-941f-17efc373393f
📒 Files selected for processing (9)
leap0/_async/client.pyleap0/_async/sandbox.pyleap0/_sync/client.pyleap0/_sync/sandbox.pyleap0/models/config.pyleap0/models/sandbox.pytests/_async/test_sandboxes.pytests/_sync/test_client_config.pytests/_sync/test_sandboxes.py
Contributor
Author
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/_sync/test_sandboxes.py`:
- Around line 130-131: The test asserts a Leap0Error but the OTEL env-check in
_inject_otel_env raises ValueError for missing OTEL_EXPORTER_OTLP_ENDPOINT;
update the test to expect ValueError (replace Leap0Error with ValueError in the
pytest.raises call for SandboxesClient(...).create(otel_export=True)) or
alternatively change the implementation in _inject_otel_env to raise Leap0Error
instead — reference SandboxesClient.create and the _inject_otel_env check for
OTEL_EXPORTER_OTLP_ENDPOINT when applying the fix.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8e935f2c-36e2-44b3-a923-3ea5e1439fda
📒 Files selected for processing (7)
leap0/_async/sandbox.pyleap0/_sync/client.pyleap0/_sync/sandbox.pyleap0/models/config.pytests/_sync/test_client_config.pytests/_sync/test_sandboxes.pytests/models/test_config.py
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/_sync/test_client_config.py
- leap0/models/config.py
- leap0/_sync/client.py
- leap0/_async/sandbox.py
- leap0/_sync/sandbox.py
Comment on lines
+130
to
+131
| with pytest.raises(Leap0Error, match="OTEL_EXPORTER_OTLP_ENDPOINT"): | ||
| SandboxesClient(mock_transport, sandbox_domain="s.dev").create(otel_export=True) |
There was a problem hiding this comment.
Exception type assertion appears inconsistent with implementation.
At Line 130, the test expects Leap0Error, but leap0/_sync/sandbox.py (_inject_otel_env) raises ValueError when OTEL_EXPORTER_OTLP_ENDPOINT is missing. This assertion will likely fail unless that path is explicitly wrapped upstream.
Suggested test fix
- with pytest.raises(Leap0Error, match="OTEL_EXPORTER_OTLP_ENDPOINT"):
+ with pytest.raises(ValueError, match="OTEL_EXPORTER_OTLP_ENDPOINT"):
SandboxesClient(mock_transport, sandbox_domain="s.dev").create(otel_export=True)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| with pytest.raises(Leap0Error, match="OTEL_EXPORTER_OTLP_ENDPOINT"): | |
| SandboxesClient(mock_transport, sandbox_domain="s.dev").create(otel_export=True) | |
| with pytest.raises(ValueError, match="OTEL_EXPORTER_OTLP_ENDPOINT"): | |
| SandboxesClient(mock_transport, sandbox_domain="s.dev").create(otel_export=True) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/_sync/test_sandboxes.py` around lines 130 - 131, The test asserts a
Leap0Error but the OTEL env-check in _inject_otel_env raises ValueError for
missing OTEL_EXPORTER_OTLP_ENDPOINT; update the test to expect ValueError
(replace Leap0Error with ValueError in the pytest.raises call for
SandboxesClient(...).create(otel_export=True)) or alternatively change the
implementation in _inject_otel_env to raise Leap0Error instead — reference
SandboxesClient.create and the _inject_otel_env check for
OTEL_EXPORTER_OTLP_ENDPOINT when applying the fix.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
leap0/_async/sandbox.py (1)
198-226:⚠️ Potential issue | 🟠 MajorPreserve
telemetry=compatibility in the async client.This removes the old keyword outright, so existing async callers using
create(telemetry=...)will start failing while the sync client still has a migration path. Keep the deprecated alias here as well and resolve it the same way asSandboxesClient.create.Suggested fix
- otel_export: bool = False, + otel_export: bool | None = None, + telemetry: bool | None = None, env_vars: dict[str, str] | None = None, network_policy: NetworkPolicyDict | None = None, http_timeout: float | None = None, @@ + if otel_export is not None and telemetry is not None and otel_export != telemetry: + raise ValueError("Pass only one of otel_export or telemetry") + effective_otel_export = otel_export if otel_export is not None else bool(telemetry) + params = CreateSandboxParams( template_name=template_name, vcpu=vcpu, memory_mib=memory_mib, timeout_min=timeout_min, auto_pause=auto_pause, - otel_export=otel_export, - env_vars=_inject_otel_env(env_vars) if otel_export else env_vars, + otel_export=effective_otel_export, + env_vars=_inject_otel_env(env_vars) if effective_otel_export else env_vars, network_policy=network_policy, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@leap0/_async/sandbox.py` around lines 198 - 226, The async create method removed the deprecated telemetry keyword—restore backward compatibility by adding a telemetry: bool | None parameter to the method signature and resolve it the same way as SandboxesClient.create (i.e., if telemetry is not None use it, otherwise use otel_export), then pass the resolved boolean into CreateSandboxParams (keep using _inject_otel_env(env_vars) when the resolved value is true) so existing async callers using telemetry continue to work.leap0/_sync/sandbox.py (1)
199-225:⚠️ Potential issue | 🟡 MinorTighten the deprecated alias handling.
Line 224 silently lets
otel_exportwin when both keywords are passed, which can hide migration mistakes. Also, usingtelemetryalone never surfaces that it's deprecated, so callers won't notice until the alias is eventually removed.Suggested fix
+ if otel_export is not None and telemetry is not None and otel_export != telemetry: + raise ValueError("Pass only one of otel_export or telemetry") + if telemetry is not None: + warnings.warn( + "`telemetry` is deprecated; use `otel_export` instead", + DeprecationWarning, + stacklevel=2, + ) effective_otel_export = otel_export if otel_export is not None else bool(telemetry)Add
import warningsat the top of the file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@leap0/_sync/sandbox.py` around lines 199 - 225, Update handling of the otel_export/telemetry parameters: add "import warnings" and change the logic around effective_otel_export so that if both otel_export and telemetry are provided you raise a TypeError (to avoid silently preferring otel_export), and if telemetry is provided alone you emit a warnings.warn(..., DeprecationWarning) telling callers telemetry is deprecated and to use otel_export; finally set effective_otel_export = otel_export if otel_export is not None else bool(telemetry) after those checks.
🧹 Nitpick comments (1)
leap0/_async/sandbox.py (1)
203-214: Document the local OTEL env requirement on the async API too.The sync client now explains that
otel_exportrequiresOTEL_EXPORTER_OTLP_ENDPOINTlocally and forwardsOTEL_EXPORTER_OTLP_HEADERSwhen present. The async path has the same runtime behavior, but the docstring omits it.Suggested fix
- otel_export: Whether OpenTelemetry exporter variables should be injected. + otel_export: Inject OpenTelemetry exporter environment into the sandbox. + Requires ``OTEL_EXPORTER_OTLP_ENDPOINT`` in the local environment and + also forwards ``OTEL_EXPORTER_OTLP_HEADERS`` when present.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@leap0/_async/sandbox.py` around lines 203 - 214, Update the async sandbox creation docstring (the docblock for the function that starts "Create a new sandbox from a template" and accepts the otel_export parameter) to mirror the sync client's note: document that setting otel_export=True requires OTEL_EXPORTER_OTLP_ENDPOINT to be present in the local environment and that if OTEL_EXPORTER_OTLP_HEADERS is set it will be forwarded; add a concise sentence under the otel_export parameter describing these runtime requirements and header forwarding so the async API matches the sync API behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@leap0/_async/sandbox.py`:
- Around line 198-226: The async create method removed the deprecated telemetry
keyword—restore backward compatibility by adding a telemetry: bool | None
parameter to the method signature and resolve it the same way as
SandboxesClient.create (i.e., if telemetry is not None use it, otherwise use
otel_export), then pass the resolved boolean into CreateSandboxParams (keep
using _inject_otel_env(env_vars) when the resolved value is true) so existing
async callers using telemetry continue to work.
In `@leap0/_sync/sandbox.py`:
- Around line 199-225: Update handling of the otel_export/telemetry parameters:
add "import warnings" and change the logic around effective_otel_export so that
if both otel_export and telemetry are provided you raise a TypeError (to avoid
silently preferring otel_export), and if telemetry is provided alone you emit a
warnings.warn(..., DeprecationWarning) telling callers telemetry is deprecated
and to use otel_export; finally set effective_otel_export = otel_export if
otel_export is not None else bool(telemetry) after those checks.
---
Nitpick comments:
In `@leap0/_async/sandbox.py`:
- Around line 203-214: Update the async sandbox creation docstring (the docblock
for the function that starts "Create a new sandbox from a template" and accepts
the otel_export parameter) to mirror the sync client's note: document that
setting otel_export=True requires OTEL_EXPORTER_OTLP_ENDPOINT to be present in
the local environment and that if OTEL_EXPORTER_OTLP_HEADERS is set it will be
forwarded; add a concise sentence under the otel_export parameter describing
these runtime requirements and header forwarding so the async API matches the
sync API behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7bff5547-5f6f-4ae4-bcd5-e2d93808fc25
📒 Files selected for processing (4)
leap0/_async/sandbox.pyleap0/_sync/sandbox.pyleap0/constants.pyleap0/models/config.py
✅ Files skipped from review due to trivial changes (1)
- leap0/constants.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Refactor
Behavior