Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Respect batch size limits when flushing queue #417

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

adam-on-tailwind
Copy link

Problem

I noticed that observations are being dropped by the client. When I looked into it further I realized that this happens when we exceed the batch size limit. The LangfuseCore#processQueueItems method is separating queued items into those that should be processed and those that should be skipped. The problem is that the flush method doesn't respect that segregation, despite the fact that it invokes processQueueItems. Therefore it's possible to send an oversized batch to the server. I'm pretty sure that the server is enforcing batch size limits, which is why we're seeing dropped observations.

Changes

I updated the flush method to consume the results of processQueueItems. Now it will only send the processedItems and will re-queue the remainingItems for the next time we flush. I suppose that there is a possibility that the queue could grow without bound. If that's an issue, my suggestion would be to implement a fixed size queue instead.

Release info Sub-libraries affected

langfuse-core

Bump level

  • Major
  • Minor
  • Patch

Libraries affected

  • All of them
  • langfuse
  • langfuse-node

Changelog notes

  • Do not flush batches that exceed maximum batch size

Copy link

vercel bot commented Oct 4, 2024

@adam-on-tailwind is attempting to deploy a commit to the langfuse Team on Vercel.

A member of the Team first needs to authorize it.

@CLAassistant
Copy link

CLAassistant commented Oct 4, 2024

CLA assistant check
All committers have signed the CLA.

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Disclaimer: Experimental PR review

PR Summary

This PR updates the flush method in LangfuseCoreStateless to respect batch size limits when flushing the queue, addressing the issue of dropped observations.

  • Implemented processQueueItems method to separate items into processed and remaining queues
  • Updated flush method to only send processed items and re-queue remaining items
  • Added test case in langfuse.flush.spec.ts to verify correct handling of oversized batches
  • Ensures client respects batch size limits, preventing server-side observation drops
  • Potential for unbounded queue growth, suggesting consideration of a fixed-size queue implementation

2 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings

@adam-on-tailwind adam-on-tailwind force-pushed the adambom-flush-remaining-items branch 2 times, most recently from 0d140e1 to bcaa644 Compare October 4, 2024 18:00
@adam-on-tailwind adam-on-tailwind force-pushed the adambom-flush-remaining-items branch from bcaa644 to 2fd0e77 Compare October 4, 2024 18:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants