-
Notifications
You must be signed in to change notification settings - Fork 344
fix: log non-retryable 4xx batch drops and warn on silently ignored sample_rate - #1874
fix: log non-retryable 4xx batch drops and warn on silently ignored sample_rate #1874simpleqt 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Claude Code Review
This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P1 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.
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.
Uh oh!
There was an error while loading. Please reload this page.
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..."