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

Add tracing, factor out some helper functions, fix a few bugs with Nodejs mode #68

Merged
merged 11 commits into from
Jun 21, 2024

Conversation

ankrgyl
Copy link
Contributor

@ankrgyl ankrgyl commented Jun 17, 2024

This change is a bunch of small tweaks. To summarize:

  • Expose a few functions (like app url) out so we can access them when using it as a library
  • Fix a series of streaming/incompat bugs that have accumulated between cloudflare's workerd runtime and node. For example, Node's fetch decompresses data, while workerd's does not. Also, we weren't properly closing streams based on how we use Vercel's AI SDK parser, so I just rewrote that to use our own streaming logic and pulled out the parser code.
  • Add support for tracing.

Copy link

vercel bot commented Jun 17, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
ai-proxy ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 21, 2024 6:18pm

@ankrgyl ankrgyl changed the title Factor out app url Add tracing, factor out some helper functions, fix a few bugs with Nodejs mode Jun 21, 2024
}

// https://github.com/openai/openai-node/blob/07b3504e1c40fd929f4aae1651b83afc19e3baf8/src/resources/chat/completions.ts#L43-L49
// Updated for https://github.com/openai/openai-node/commit/f10c757d831d90407ba47b4659d9cd34b1a35b1d
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this comment superseded by the next line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure. It's just copy/pasted from Vercel's code. Would rather not change it.

@@ -65,6 +71,12 @@ export interface CacheKeyOptions {
excludeOrgName?: boolean;
}

export interface SpanLogging {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: for uniformity, should we name this type SpanLogger?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep

setName: (name: string) => void;
log: (args: ExperimentLogPartialArgs) => void;
end: () => void;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to explicitly flush the logger here, or is it guaranteed that the beforeExit event will fire in all environments that we run this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's ok to make it the responsibility of the caller (which is currently our own tightly controlled server, where asyncFlush is false by default anyway).

@ankrgyl ankrgyl merged commit 7d4bfd6 into main Jun 21, 2024
1 of 2 checks passed
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