Conversation
…SIZE_BYTES Co-authored-by: Hassieb Pakzad <hassiebp@users.noreply.github.com>
|
@claude review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
No review was started: this request came from a bot account. Manual reviews can only be requested by someone with write access to this repository. Ask a maintainer to comment @claude review, or have your automation post the comment from a user account with write access.
Tip: disable this comment in your organization's Code Review settings.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f8635204c3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if raw_value.isascii() and raw_value.isdigit() and int(raw_value) > 0: | ||
| return int(raw_value) |
There was a problem hiding this comment.
On supported Python 3.11+ versions, converting a decimal string longer than the interpreter's default 4,300-digit limit raises ValueError. Because this conversion occurs directly in the condition, setting LANGFUSE_OTEL_MAX_BATCH_SIZE_BYTES to such a numeric string aborts Langfuse client initialization instead of warning and retaining the exporter default as promised for invalid values. Parse once inside a guarded conversion and route conversion failures through the existing warning fallback.
Useful? React with 👍 / 👎.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if not raw_value: | ||
| return None | ||
|
|
||
| if raw_value.isascii() and raw_value.isdigit() and int(raw_value) > 0: |
There was a problem hiding this comment.
On Python 3.11+, if LANGFUSE_OTEL_MAX_BATCH_SIZE_BYTES contains more digits than the runtime permits for integer conversion, int(raw_value) raises ValueError before the invalid-value warning runs. The exception is not caught while the default exporter is created, so a tracing-enabled client fails to initialize instead of retaining the default limit.
Knowledge Base Used: Client initialization and resource management
Prompt To Fix With AI
This is a comment left during a code review.
Path: langfuse/_client/span_processor.py
Line: 58
Comment:
**Long numbers can block initialization**
On Python 3.11+, if `LANGFUSE_OTEL_MAX_BATCH_SIZE_BYTES` contains more digits than the runtime permits for integer conversion, `int(raw_value)` raises `ValueError` before the invalid-value warning runs. The exception is not caught while the default exporter is created, so a tracing-enabled client fails to initialize instead of retaining the default limit.
**Knowledge Base Used:** [Client initialization and resource management](https://app.greptile.com/personal-org-4986/-/custom-context/knowledge-base/langfuse/langfuse-python/-/docs/client-initialization-and-resources.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| raw_value = os.environ.get(LANGFUSE_OTEL_MAX_BATCH_SIZE_BYTES, "").strip() | ||
| if not raw_value: | ||
| return None |
There was a problem hiding this comment.
If LANGFUSE_OTEL_MAX_BATCH_SIZE_BYTES is explicitly set to an empty or whitespace-only value, stripping it makes the function return as though the variable were absent. The exporter keeps its default limit without an invalid-value warning, making the configuration mistake harder to detect.
| raw_value = os.environ.get(LANGFUSE_OTEL_MAX_BATCH_SIZE_BYTES, "").strip() | |
| if not raw_value: | |
| return None | |
| raw_value = os.environ.get(LANGFUSE_OTEL_MAX_BATCH_SIZE_BYTES) | |
| if raw_value is None: | |
| return None | |
| raw_value = raw_value.strip() |
Prompt To Fix With AI
This is a comment left during a code review.
Path: langfuse/_client/span_processor.py
Line: 54-56
Comment:
**Blank values hide misconfiguration**
If `LANGFUSE_OTEL_MAX_BATCH_SIZE_BYTES` is explicitly set to an empty or whitespace-only value, stripping it makes the function return as though the variable were absent. The exporter keeps its default limit without an invalid-value warning, making the configuration mistake harder to detect.
```suggestion
raw_value = os.environ.get(LANGFUSE_OTEL_MAX_BATCH_SIZE_BYTES)
if raw_value is None:
return None
raw_value = raw_value.strip()
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
| def _resolve_max_batch_size_bytes() -> Optional[int]: | ||
| """Return the configured batch byte limit, or None to keep the exporter default.""" | ||
| raw_value = os.environ.get(LANGFUSE_OTEL_MAX_BATCH_SIZE_BYTES, "").strip() | ||
| if not raw_value: | ||
| return None | ||
|
|
||
| if raw_value.isascii() and raw_value.isdigit() and int(raw_value) > 0: |
There was a problem hiding this comment.
🟡 (optional) Setting LANGFUSE_OTEL_MAX_BATCH_SIZE_BYTES to an all-digit value longer than 4300 characters crashes Langfuse() client construction instead of falling back with a warning like other invalid values. _resolve_max_batch_size_bytes() (span_processor.py:52-66) checks raw_value.isascii() and raw_value.isdigit() and int(raw_value) > 0; for an overlong digit string, int(raw_value) raises ValueError (CPython's default int<->str conversion limit, 4300 digits since Python 3.11) instead of the value simply failing isdigit(). Nothing catches it here or at the call site (span_processor.py:155) or in resource_manager.py:251 where LangfuseSpanProcessor is built with no try/except, so Langfuse() itself raises. …
Why this was flagged
…Fix: wrap the int() conversion in try/except (or bound the string length first) so any malformed value, including an overlong digit string, falls back to the warning+default path like '0', '-5', 'abc', and '1.5' already do.
Trigger: LANGFUSE_OTEL_MAX_BATCH_SIZE_BYTES is set to a string of only ASCII digits but longer than CPython's default int<->str conversion limit (sys.get_int_max_str_digits(), default 4300 since Python 3.11). Entry point: LangfuseSpanProcessor.init (span_processor.py:155) calls _resolve_max_batch_size_bytes() whenever span_exporter is None. There, raw_value.isascii() and raw_value.isdigit() and int(raw_value) > 0 (span_processor.py:58) evaluates int(raw_value), which raises ValueError('Exceeds the limit ... for integer string conversion') for such a string; unlike the isdigit()==False branch at lines 61-66, this raise is not caught. resource_manager.py:251 constructs LangfuseSpanProcessor with no surrounding try/except, so the exception propagates and Langfuse() itself raises, unlike every other invalid value the tests cover (0, -5, abc, 1.5), which log a…
Verification: nit. Real but trivial edge case. At span_processor.py:58 the validation is raw_value.isascii() and raw_value.isdigit() and int(raw_value) > 0. For an all-ASCII-digit string longer than CPython's default int<->str conversion limit (4300 digits, present since Python 3.7.14 and 3.11+), isascii() and isdigit() are both True, so int(raw_value) is evaluated and raises an uncaught ValueError…
This branch has not been deployed
2 participants
What does this PR do?
Brings the JS SDK's export batch byte cap (langfuse-js#928) to Python, using the upstream OTLP/HTTP exporter's new
max_request_size(open-telemetry/opentelemetry-python#5369, released inopentelemetry-exporter-otlp-proto-http1.45.0).LANGFUSE_OTEL_MAX_BATCH_SIZE_BYTES(positive integer, same name and rules as JS) and passes it to the SDK-created defaultOTLPSpanExporterasmax_request_size.0, negative, non-integer) log a warning and keep the default limit.span_exporters are not touched. Thespan_exporterdocstring now says so.The minimum OTel version and the lockfile are intentionally unchanged. 1.45.0 is 4 days old, so
exclude-newer = "7 days"blocks it. Requiring it would also force anopentelemetry-sdkupgrade on everyone.Type of change
Verification
End-to-end against a local HTTP server with
LANGFUSE_OTEL_MAX_BATCH_SIZE_BYTES=5000. OTel 1.45 was installed in a throwaway venv; the lockfile is unchanged:ruff format --check .also flagstests/unit/test_media.py. That file already fails onmainand is not touched here.Checklist
code_review.md..env.templateif needed.The PR should address the client-initialization failure for oversized numeric configuration before merging.
Summary
The PR adds an environment-configurable serialized request-size limit for the SDK-created OTLP exporter, using signature detection to retain compatibility with older OpenTelemetry versions.
Reviews (1) · Last reviewed commit: "feat(otel): cap oversized export batches..."