fix: preserve zero sample rate - #1784
Conversation
|
@claude review |
There was a problem hiding this comment.
LGTM — correct, minimal fix.
Extended reasoning...
Overview
The PR changes a single line in langfuse/_client/client.py: the sample_rate resolution logic changes from sample_rate = sample_rate or float(os.environ.get(...)) to an explicit if sample_rate is None: check. This matches the parameter's Optional[float] = None default and fixes a genuine bug where sample_rate=0.0 (a falsy value) was silently overridden by the env var or the 1.0 default, causing 100% sampling instead of the user's intended 0%.
Security risks
None. This only affects local sampling-rate resolution during client construction; no auth, crypto, network, or user input parsing is involved.
Level of scrutiny
Low-to-moderate is appropriate: it's a one-line logic correctness fix confined to a single, well-understood code path (parameter precedence resolution), not a design change. The subsequent range validation (0.0 <= sample_rate <= 1.0) still correctly clamps out-of-range values regardless of the resolution path taken.
Other factors
The added test (test_zero_sample_rate_parameter_is_preserved) directly exercises the regression scenario — explicit sample_rate=0.0 with LANGFUSE_SAMPLE_RATE=1.0 set in the environment — and asserts the stored value is 0.0, which is the right test for this fix. The env-var-only path (float(os.environ.get(LANGFUSE_SAMPLE_RATE, 1.0))) was already correct before this change and is untouched. No other callers of sample_rate needed changes. The bug hunting system found no issues, and my own read of the diff and surrounding code confirms the fix is correct and narrowly scoped.
What changed
sample_rate=0.0instead of falling back to the environment or1.0LANGFUSE_SAMPLE_RATEWhy
Langfuse(sample_rate=0.0)currently uses a truthiness fallback, so the documented zero value becomes1.0and traces everything. The environment-variable path already preserves zero; this makes the keyword path follow the same documented contract.This prevents users who intentionally disable sampling from unexpectedly exporting all traces.
Scope
This is limited to
sample_rateresolution. The separate timeout and stale-docstring observations mentioned in the report are intentionally out of scope.Validation
uv run --frozen pytest tests/unit/test_initialization.py -q(19 passed)uv run --frozen ruff check .uv run --frozen ruff format --check langfuse/_client/client.py tests/unit/test_initialization.pyuv run --frozen mypy langfuse --no-error-summary1.0before the fix to0.0after itLinear: LFE-14503
Closes #1782
Greptile Summary
Preserves an explicitly configured zero sample rate instead of falling back to the environment or default.
None.sample_rate=0.0overridesLANGFUSE_SAMPLE_RATE.Confidence Score: 5/5
The PR appears safe to merge, with the focused configuration fix covered by an appropriate regression test.
The explicit
Nonecheck preserves0.0while retaining environment and default resolution for omitted values, and no blocking failure remains.Reviews (1): Last reviewed commit: "fix: preserve zero sample rate" | Re-trigger Greptile
Context used: