IS_FIREBASE_CLI missing on deploy functions @9.13.1 ? #3517
Comments
|
Same error log as in #3512 , same firebase-tools version. Not sure it's related. |
|
Looks like there are 2 issues here:
We expected the process for parsing user code to discover user-defined functions to undergo some changes in the coming months and wanted to start tightening up the contract for what environment variables are available during the parsing process (the exact contract is still being worked out). Apologies for breaking your setup - I knew the above change may possible to break someone's workflow in theory, but didn't think it will affect anyone in practice. I was wrong. Do you mind sharing your setup in more detail? I'd love to understand your use case better. In particular, I noticed that you wrote:
How are you using local-only function to reduce cold start time (in prod I assume?). |
No prob. It's an undocumented feature so... I knew this day would come 😅 In older/smaller projects we used to work like this: const functions = require('firebase-functions');
const admin = require('firebase-admin');
admin.initializeApp();
const lib1 = require('./lib1.js'); // <== related functionality together for various functions
const lib2 = require('./lib2.js');
const lib3 = require('./lib3.js');
// ...
const f = functions.region('europe-west1');
exports.onCreateCol1 = f.firestore.document('col1/{id}')
.onCreate(async (snap, context) => await lib1.onCreateCol1(snap, context));
exports.onWriteCol2 = f.firestore.document('col2/{id}')
.onWrite(async (change, context) => await lib2.onWriteCol2(change, context));
// ...Then on larger projects where cold start time was an issue for very specific functions, we followed suggestions from Firebase and Doug and came up with a mix of our own by using const functions = require('firebase-functions');
const admin = require('firebase-admin');
admin.initializeApp();
const f = functions.region('europe-west1');
if ( process.env.IS_FIREBASE_CLI
|| process.env.K_SERVICE == 'onCreateCol1'
) {
const libCol1 = require('./minimalLibForCol1.js'); // <== as specific as possible, short start time is important
exports.onCreateCol1 = f.firestore.document('col1/{id}')
.onCreate(async (snap, context) => await libCol1.onCreateCol1(snap, context));
}
if ( process.env.IS_FIREBASE_CLI
|| process.env.K_SERVICE == 'onWriteCol2'
|| process.env.K_SERVICE == 'onWriteCol3'
|| process.env.K_SERVICE == 'onWriteCol4'
) {
const otherLib = require('./otherLib.js'); // related stuff together, time is not that important
exports.onWriteCol2 = f.firestore.document('col2/{id}')
.onWrite(async (change, context) => await otherLib.onWriteCol2(change, context));
exports.onWriteCol3 = f.firestore.document('col3/{id}')
.onWrite(async (change, context) => await otherLib.onWriteCol3(change, context));
exports.onWriteCol4 = f.firestore.document('col4/{id}')
.onWrite(async (change, context) => await otherLib.onWriteCol4(change, context));
}
// if ( process.env.IS_FIREBASE_CLI ...So:
The use case comes about the need to auto-scale resources very easily without us having to work with GKE or similar. We've grown much with a particular project, and during the daytime there's traffic enough to always have some instances running for a particular function. But there's always that user that uploads 20 files at a time and those 20 triggers seem to create many new instances, leading to cold start time for many of the 20 triggers. The user ended up with list of mixed-up "Ready" and "Processing..." items for several seconds. So... either we migrated everything to GKE, or tried to fix it on firebase functions. We went for the latter. We also had to "cheat" by triggering the empty function (run but without actually processing anything, ends up quickly) when we could anticipate the user would need it. That allowed us to create some instances in advance to avoid cold start time most of the times. For other functions (mostly Hope it helps. |
google-oss-bot
added
Needs: Attention
and removed
Needs: Author Feedback
|
@maganap Thanks for sharing your setup in great detail. I learned a lot. As for immediate workaround to your problem, my suggestion is to replace the condition Give it a try and let us know how it goes (and if it's good enough, feel free to close the issue). Tangent - the way you dealt with your cold start issue is pretty fantastic. One more lead you might want to follow https://issuetracker.google.com/issues/158014637. It's a very long thread, but the summary of it is that gRPC client used in the Firestore SDK (which is in turn used in the Admin SDK) may be a big contributor to the cold-start time. I know some Googlers are working to improve the situation - though you might want to keep it under your radar. |
|
@taeold actually @maganap I use the same workflow as you, where I have some callable functions I only want to test in the shell / emulator. For that I use the However maybe the same changes that broke |
|
@samtstern Thanks for chiming in. I didn't realize To be precise, there are 2 different environments I have in mind:
This issue is about removal of To further complicate the picture, we have to distinguish trigger parsing that's done during I'm now coming to realize all these complications, and in hindsight #3422 feels a bit immature. I will try to come up with a more mature proposal w.r.t. "during trigger parsing, these environment variables are available to you. Similarly function runtime, these environment variables are available to you..." similar in style of what Cloud Functions documentation discusses in https://cloud.google.com/functions/docs/env-var#using_build_environment_variables. We should apply whatever we agree upon to both function deploy process and emulator setup process. |
… trigger parsing (#3422) We move the logic for setting up environment variables for function instances from post-trigger parsing to pre-trigger parsing. This makes environment variables accessible during trigger parsing, e.g. ``` const myFunc = functions.runWith({minInstances: process.env.TIER == "PROD" ? 20 : 0})... ``` (Of course, there isn't a way for users to specify environment variables for now. That may change soon.) This change simply refactors existing logic - nothing new is introduced and function deploys should work exactly as it is done today. *Warning* There is technically a breaking change where we no longer pass caller's `process.env` when forking a process to parse function triggers in user's source code. We think this is removing what was previously an undefined behavior but recognize we may be breaking someone's workflow. We will soon propose a support for setting project-level environment variables more easily.
|
To be able to move on from @9.12, as ugly as it looks, I ended up with something like this to minimize cold start time... at least until I find a more elegant solution. Note:
This covers the cases:
const functions = require('firebase-functions');
const reqPath = `./fn/${process.env.K_SERVICE}`;
const isFn = fn => process.env.K_SERVICE == fn || process.env.NODE_ENV !== 'production';
exports.test2 = isFn('test2') && functions.firestore.document('col2/{id}')
.onWrite( async (change, context) => await require(reqPath)(change, context) );
exports.test3 = isFn('test3') && functions.firestore.document('col3/{id}')
.onWrite( async (change, context) => await require(reqPath)(change, context) );With single function files like this: And async function test2(change, context) {
// ...
}
module.exports = test2;The rest of functions that don't require a short cold start time are just grouped in common files for simplicity. |
|
@maganap thanks for explaining! Your cold start trick looks a lot like the one that this library uses: |
|
@samtstern Nice reference! I'll take a deeper look at it. Thanks! |
|
Hey, bump for the access of For instance, when you have a staging env and you want to listen for changes on different buckets in staging env... |
|
This FR seems related to firebase/firebase-functions#1084. tl;dr - process.env will probably never be available during trigger discovery. Params will. |
[REQUIRED] Environment info
firebase-tools: 9.13.1
node: v12.22.1
Platform: macOS 10.15.7
[REQUIRED] Test case
To reduce cold start time in functions, I use to work with a structure like this:
By using
IS_FIREBASE_CLI, I could run the functions shell and deploy functions without issues, until I updated tofirebase-tools@9.13.1. Now, I can run the shell but I can't deploy.I'm getting different errors (today different from yesterday, I can still see it in the terminal but I don't have the firebase-debug.log from yesterday).
Today:
Yesterday, it was like the deploy command ran successfully, but it didn't deploy anything.
If I remove the
if()statement, it does deploy properly.I know this use of
IS_FIREBASE_CLIis not documented but, I'm still wondering what's happened sinceIS_FIREBASE_CLIstill exists and it seems to be used in other cases.Any other suggestions to be able to deploy using
if ( process.env.K_SERVICE == 'test' ) {? (which of course won't allow me to deploy sinceprocess.env.K_SERVICEis not set while deploying).Thanks in advance.
[REQUIRED] Steps to reproduce
Just try to deploy the piece of code above with:
firebase deploy --only functions:test[REQUIRED] Expected behavior
The function
testdeploys as it used to with earlier versions, including 9.12.1.[REQUIRED] Actual behavior
The function won't deploy. It is still found by the functions shell, though.
The log below is exactly the same as if I ran the deploy command with
if ( false ) {to enclose the function to be deployed.The text was updated successfully, but these errors were encountered: