fix: log non-retryable 4xx batch drops and warn on silently ignored sample_rate - #1874
Open
simpleqt wants to merge 2 commits into
Open
fix: log non-retryable 4xx batch drops and warn on silently ignored sample_rate#1874simpleqt wants to merge 2 commits into
simpleqt wants to merge 2 commits into
Conversation
Previously, a non-retryable 4xx (e.g. 401 after key rotation, 400 validation) caused execute_task_with_backoff to silently return, permanently losing every score and trace event in the batch with no log or exception. Now raises so handle_exception logs the loss.
…gistered OTel provider Users setting Langfuse(sample_rate=0.1) in an OTel-instrumented app get 100% sampling because the tracer provider is external. The init log claims sample_rate=0.1 is active — misleading. Added an explicit warning matching the id_generator ignored-warning pattern.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| # Non-retryable 4xx: the batch is permanently lost. | ||
| # Raise so handle_exception logs the loss instead of | ||
| # silently pretending the batch was delivered. | ||
| raise e |
Contributor
There was a problem hiding this comment.
When the API returns a non-rate-limited 4xx response such as 400 or 401, raising here causes the surrounding backoff.on_exception(..., Exception, max_tries=self._max_retries) decorator to retry the request. The rejected batch is therefore submitted up to three times with exponential delays before handle_exception logs the loss, which delays flush() and treats a permanent failure as transient. Use a backoff giveup condition, or propagate the failure only after leaving the decorated operation.
Prompt To Fix With AI
This is a comment left during a code review.
Path: langfuse/_task_manager/score_ingestion_consumer.py
Line: 193
Comment:
**Permanent failures are retried**
When the API returns a non-rate-limited 4xx response such as 400 or 401, raising here causes the surrounding `backoff.on_exception(..., Exception, max_tries=self._max_retries)` decorator to retry the request. The rejected batch is therefore submitted up to three times with exponential delays before `handle_exception` logs the loss, which delays `flush()` and treats a permanent failure as transient. Use a backoff `giveup` condition, or propagate the failure only after leaving the decorated operation.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
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
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
Two fixes:
Silent batch loss:
execute_task_with_backoffreturned without logging when a non-retryable 4xx (401 after key rotation, 400 validation) hit the score/trace ingestion API. Every event in the batch was permanently lost with no signal —flush()even reported success. Now raises sohandle_exceptionlogs the loss.Silently ignored
sample_rate: when an OTel global TracerProvider is already registered,sample_ratewas dropped without warning while the init log claimed it was active. Added an explicit warning matching the existingid_generatorignored-warning pattern.Test plan
Both fixes are logging/warning additions that don't change data flow. Existing behavior for correctly-configured setups is unchanged.
The PR should not merge until permanent 4xx responses are logged without being retried as transient failures.
Summary
sample_ratecannot be applied because an OpenTelemetry provider is already registered.Reviews (1) · Last reviewed commit: "docs(client): warn when sample_rate is s..."