Add X-Firebase-AppId header to VertexAI requests
#8809
Conversation
dlarocque
requested review from
paulb777,
DellaBitta,
hsubox76 and
rainshen49
🦋 Changeset detectedLatest commit: 767cd77 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Size Report 1Affected Products
Test Logs |
Size Analysis Report 1Affected Products
Test Logs |
It's fine, according to HTTP standard, headers are case insensitive, I guess we just capitalize for better readability in code. |
| @@ -68,10 +68,16 @@ export abstract class VertexAIModel { | |||
| VertexAIErrorCode.NO_PROJECT_ID, | |||
| `The "projectId" field is empty in the local Firebase config. Firebase VertexAI requires this field to contain a valid project ID.` | |||
| ); | |||
| } else if (!vertexAI.app?.options?.appId) { | |||
| throw new VertexAIError( | |||
There was a problem hiding this comment.
In light of Paul's comment about automaticDataCollectionEnabled, not sure if we throw this error if that's false, since we don't actually need appId in that case? Kind of weird edge case though.
There was a problem hiding this comment.
The firebaseAppConfig obtained from Firebase Console and Terraform always contains an appId, so unless the developer deliberately removes the appId (not sure why since it's not sensitive), it should always be present. AppId is not just for telemetry purposes. Thus, stronger validation is better here.
| @@ -68,10 +68,16 @@ export abstract class VertexAIModel { | |||
| VertexAIErrorCode.NO_PROJECT_ID, | |||
| `The "projectId" field is empty in the local Firebase config. Firebase VertexAI requires this field to contain a valid project ID.` | |||
| ); | |||
| } else if (!vertexAI.app?.options?.appId) { | |||
| throw new VertexAIError( | |||
There was a problem hiding this comment.
The firebaseAppConfig obtained from Firebase Console and Terraform always contains an appId, so unless the developer deliberately removes the appId (not sure why since it's not sensitive), it should always be present. AppId is not just for telemetry purposes. Thus, stronger validation is better here.
| @@ -78,6 +78,8 @@ export abstract class VertexAIModel { | |||
| apiKey: vertexAI.app.options.apiKey, | |||
| project: vertexAI.app.options.projectId, | |||
| appId: vertexAI.app.options.appId, | |||
| automaticDataCollectionEnabled: | |||
There was a problem hiding this comment.
No action needed, just wondering out loud if at some number of properties, we just pass vertexAI.app or at least vertexAI.app.options through instead of passing them through one by one.
| @@ -84,7 +84,9 @@ export async function getHeaders(url: RequestUrl): Promise<Headers> { | |||
| headers.append('Content-Type', 'application/json'); | |||
| headers.append('x-goog-api-client', getClientHeaders()); | |||
| headers.append('x-goog-api-key', url.apiSettings.apiKey); | |||
| headers.append('X-Firebase-AppId', url.apiSettings.appId); // Will be converted to 'X-Firebase-Appid' before it's sent in the browser. | |||
| if (url.apiSettings.automaticDataCollectionEnabled !== false) { | |||
There was a problem hiding this comment.
Maybe we should be safe and make this fail on any falsy value in case, say, the user sets it to undefined for some reason. Sometimes that's how people unset properties. So just if (url.apiSettings.automaticDataCollectionEnabled)? I do think this is a very edge case, so consider it a nit.
Do not merge.
X-Firebase-AppIdheader containing the App ID to VertexAI requests.appIdtoApiSettings, and throw an error if it's not defined when initializing the VertexAI instance.In my testing, the browser convertedX-Firebase-AppIdtoX-Firebase-Appid(lowercasesi) in the request. Would this cause any issues?