Add X-Firebase-AppId header to VertexAI requests
#8809
Conversation
dlarocque
requested review from
paulb777,
DellaBitta,
hsubox76 and
rainshen49
🦋 Changeset detectedLatest commit: e8c97ad 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 1This report is too large (97,096 characters) to be displayed here in a GitHub comment. Please use the below link to see the full report on Google Cloud Storage.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.
dlarocque
force-pushed
the
dl/firebase-app-id
branch
from
d09a548 to
76f635a
Compare
These headers are only sent when
automaticDataCollectionis true. This is false by default, so users must explicitly set it to true for these headers to be sent.X-Firebase-Appidheader containing the App ID to VertexAI requests.X-Firebase-Appversionheader containing the Firebase JS SDK version (e.g. 11.4.0) 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?Sample headers from a request: