Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

Gateway: forward X-Fireworks-Genie header from client#63460

Merged
olafurpg merged 2 commits into
mainfrom
olafurpg-cody-2555-gateway-forward-x-fireworks-genie-http-header-to-fireworks
Jun 25, 2024
Merged

Gateway: forward X-Fireworks-Genie header from client#63460
olafurpg merged 2 commits into
mainfrom
olafurpg-cody-2555-gateway-forward-x-fireworks-genie-http-header-to-fireworks

Conversation

@olafurpg

Copy link
Copy Markdown
Contributor

Previously, there was no way to enable the "tracing" feature from Fireworks https://readme.fireworks.ai/docs/enabling-tracing This PR solves the problem by forwarding the X-Fireworks-Genie HTTP header to Fireworks if this HTTP header is set by the Gateway client.

Fixes CODY-2555

Test plan

N/A

Changelog

Previously, there was no way to enable the "tracing" feature from
Fireworks https://readme.fireworks.ai/docs/enabling-tracing
This PR solves the problem by forwarding the `X-Fireworks-Genie` HTTP
header to Fireworks if this HTTP header is set by the Gateway client.

Fixes CODY-2555
@cla-bot cla-bot Bot added the cla-signed label Jun 25, 2024

@eseliger eseliger left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

As talked about in person, this is free of charge, and a client side change will need to explicitly enable this for requests. Seems low risk 👍


// Run the request transformer.
methods.transformRequest(req)
methods.transformRequest(r, req)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Might be nice to adopt the new naming scheme here as well (upstream and downstream req)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good idea. Done

@olafurpg

Copy link
Copy Markdown
Contributor Author

FYI @rafax @chrsmith

@olafurpg olafurpg marked this pull request as ready for review June 25, 2024 02:43
@olafurpg olafurpg enabled auto-merge (squash) June 25, 2024 02:43
@olafurpg olafurpg merged commit a426134 into main Jun 25, 2024
@olafurpg olafurpg deleted the olafurpg-cody-2555-gateway-forward-x-fireworks-genie-http-header-to-fireworks branch June 25, 2024 02:49
@chrsmith

Copy link
Copy Markdown
Contributor

Post-hoc LGTM. This changes looks good. Although I wish we had better names than "upstream" and "downstream" for these concepts. I personally find it kinda confusing. (Thinking that the caller would be "upstream", and the thing I was forwarding the call to would be "downstream".)

incommingRequest and outboundRequest? callToCG and callTo3rdParty? #namingishard

@olafurpg

olafurpg commented Jul 2, 2024

Copy link
Copy Markdown
Contributor Author

@chrsmith agreed on naming. I went with upstream/downstream because I saw several mentions of "upstream" referring to the request going to the LLM provider. I didn't want to introduce a third naming convention. No objections to doing a batch rename to something clearer

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants