-
Notifications
You must be signed in to change notification settings - Fork 701
add a new unified route for count_tokens endpoint #1293
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
base: main
Are you sure you want to change the base?
add a new unified route for count_tokens endpoint #1293
Conversation
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.
Code quality is good with proper error handling and consistent patterns. Found minor documentation mismatch and potential optimization opportunity.
/** | ||
* Handles the '/messages' API request by selecting the appropriate provider(s) and making the request to them. | ||
* | ||
* @param {Context} c - The Cloudflare Worker context. | ||
* @returns {Promise<Response>} - The response from the provider. | ||
* @throws Will throw an error if no provider options can be determined or if the request to the provider(s) fails. | ||
* @throws Will throw an 500 error if the handler fails due to some reasons | ||
*/ |
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.
📝 Documentation Gap
Issue: JSDoc comment describes '/messages' API but function handles '/messages/count_tokens' endpoint
Fix: Update documentation to match actual functionality
Impact: Prevents developer confusion about handler purpose
/** | |
* Handles the '/messages' API request by selecting the appropriate provider(s) and making the request to them. | |
* | |
* @param {Context} c - The Cloudflare Worker context. | |
* @returns {Promise<Response>} - The response from the provider. | |
* @throws Will throw an error if no provider options can be determined or if the request to the provider(s) fails. | |
* @throws Will throw an 500 error if the handler fails due to some reasons | |
*/ | |
/** | |
* Handles the '/messages/count_tokens' API request by selecting the appropriate provider(s) and making the request to them. | |
* | |
* @param {Context} c - The Cloudflare Worker context. | |
* @returns {Promise<Response>} - The response from the provider. | |
* @throws Will throw an error if no provider options can be determined or if the request to the provider(s) fails. | |
* @throws Will throw an 500 error if the handler fails due to some reasons | |
*/ |
): Promise<Response> { | ||
try { | ||
let request = await c.req.json(); | ||
let requestHeaders = Object.fromEntries(c.req.raw.headers); |
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.
⚡️ Performance Improvement
Issue: Object.fromEntries creates unnecessary object copy when headers are already iterable
Fix: Pass headers directly to constructConfigFromRequestHeaders if it accepts Headers object
Impact: Reduces object creation overhead and improves memory efficiency
let requestHeaders = Object.fromEntries(c.req.raw.headers); | |
let requestHeaders = c.req.raw.headers; |
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.
Do we have snitch tests for this? The PR looks good to me. The 2 comments from Matter seem valid.
Tested by making request to count tokens (attached curls for reference)
**NOTE : ** Bedrock does not support counting tokens
vertex
anthropic