feat(fs): preferRest app option for Firestore #1901

alexander-fenster · 2022-09-15T16:56:19Z

Fixes #1879 by providing a way to pass preferRest to the Firestore constructor. This is coming from the solution we provided in https://issuetracker.google.com/issues/158014637. The preferRest option might become default in @google-cloud/firestore later (possibly after the next major), but for now, we'd better keep this behind a flag, so the customers asked for a way to pass this flag from firebase-admin.

MarkDuckworth

LGTM

lahirumaramba

Thank you @alexander-fenster !
This is great! We need an internal API proposal for changes to the public API before we can merge this PR. I also left a comment below on making the config an App option.

lahirumaramba · 2022-09-15T19:25:44Z

src/app/core.ts

+   * HTTP/1.1 REST transport if possible. Note that gRPC will still be
+   * used for calls that need bi-directional streaming.
+   */
+  preferRest?: boolean;


I am not entirely sure about making this an app option... is the idea to reuse the same config for other gRPC libraries in the future?

This was just my first thought to pass it the same way as other options - alternatively, we can make it a second parameter to the Firestore client, or enable this via an environment variable. What would you suggest? I don't know this codebase :) I'll follow up internally to figure out the API proposal.

I have been using the admin SDK for a couple of years and have recently changed over to using import which uses getFirestore(appInstance) which would only work if either an ENV variable was set, or http was set in the app instance (or add more arguments to getFirestore)

We are actually thinking of adding the second parameter to getFirestore, looks like the cleanest solution. I'll get back here after we discuss internally.

I would have expected that you'd set it the same way you'd configure the ignoreUndefinedProperties Firestore setting (given how it's implemented in the @google-cloud/firestore library)

So:

getFirestore().settings({ preferRest: true })

But if more libraries will eventually support REST over only GRPC, then I'd expect to be in the AppOptions, like:

initializeApp({ preferRest: true, })

Maybe both should be possible if you want REST only for Firestore but not other libraries.
But if you want REST for ALL libraries, then it would be nice if you could define it via the AppOptions.

GriffinJohnston · 2022-10-02T15:51:14Z

Any progress on this? We're so close!

lahirumaramba · 2022-10-03T17:37:20Z

Any progress on this? We're so close!

Thank you @GriffinJohnston ! We have started the internal API review process for this.

si1k · 2022-11-08T16:30:36Z

We have started the internal API review process for this.

Typically how long does the API review process take? 1 month? 3 months? 12 months?

Excited for this functionality. Thank you!

alexander-fenster · 2022-11-08T17:25:19Z

We have started the internal API review process for this.

Typically how long does the API review process take? 1 month? 3 months? 12 months?

Excited for this functionality. Thank you!

The review is completed, we decided we'll take a slightly different approach (adding initializeFirestore method that would accept options, similar to how Web SDK works). I will fix the PR and resend it for review soon.

ishowta · 2022-11-11T09:30:42Z

By the way firebase-auth's new blocking function feature has a timeout of 7 seconds, making it tough to use without this feature.
https://firebase.google.com/docs/auth/extend-with-blocking-functions#understanding_blocking_functions

ganey · 2022-11-11T10:05:00Z

By the way firebase-auth's new blocking function feature has a timeout of 7 seconds, making it tough to use without this feature. https://firebase.google.com/docs/auth/extend-with-blocking-functions#understanding_blocking_functions

Glad I didn't start using that then, you can't always init the admin sdk and load from firestore within 7 seconds 🤦

nikcheerla · 2022-11-18T08:46:50Z

Any new progress on this? this feature would be really useful for us as we have seen significant speedup when using preferRest on the backend, but need to use it with the admin SDK.

If this option won't land soon, any suggestions for using the admin SDK on a serverless backend (Firebase cloud functions) but still getting the improved preferRest behavior?

GriffinJohnston · 2022-11-21T18:45:40Z

+1 to keeping a fire lit under this project. I think Googlers maybe don't understand how big a deal this is. I talk to quite a few devs using Firestore and the only complaint every single one of them mentions is this exact cold start issue.

alexander-fenster · 2022-11-21T23:04:58Z

@lahirumaramba PTAL. I replaced the implementation according to the design as we discussed.

…min-node into preferRest

lahirumaramba

Thank you @alexander-fenster ! LGTM!

etc/firebase-admin.firestore.api.md

lahirumaramba · 2022-11-22T20:47:24Z

src/firestore/index.ts

+ * 
+ * @param settings - Settings object to be passed to the constructor.
+ *
+ * @returns The `Firestore` service associated with the provided app.


Should this be @returns The Firestore service associated with the provided app and settings?

src/firestore/index.ts

src/firestore/firestore-internal.ts

lahirumaramba

@alexander-fenster Let's get TW review on the reference docs. Thanks!

MarkDuckworth · 2022-11-28T19:27:06Z

src/firestore/firestore-internal.ts

+ */
+export interface FirestoreSettings {
+  /**
+   * Use HTTP/1.1 REST transport where possible.


It may be worth clarifying the behavior of this setting.

preferRest will force the use of HTTP/1.1 REST transport until an operation requires GRPC. When an operation requires GRPC, this Firestore client will load dependent GRPC libraries and then use GRPC transport for all communication from that point forward. Currently the only operation that requires GRPC is creating a snapshot listener using onSnapshot(...).

IchordeDionysos · 2022-12-16T09:49:37Z

@alexander-fenster With the new function initializeFirestore, there has been tech debt added as there is unexpected behavior:

e.g., code like this will fail with an initializeFirestore() has already been called with different options. error:

  initializeFirestore(getApp(), {
    preferRest: true,
  });

  getFirestore().settings({
    timestampsInSnapshots: true,
    ignoreUndefinedProperties: true,
  });

However, it's not possible to do the following:

  initializeFirestore(getApp(), {
    preferRest: true,
    timestampsInSnapshots: true,
    ignoreUndefinedProperties: true,
  });

as only preferRest is configured for the initializeFirestore function.

However, this seems to work:

  getFirestore().settings({
    timestampsInSnapshots: true,
    ignoreUndefinedProperties: true,
    preferRest: true,
  });

GriffinJohnston · 2022-12-16T18:01:37Z

I'm currently using a simple setup like this:

const { initializeApp } = require('firebase-admin/app')
const { initializeFirestore } = require('firebase-admin/firestore')

const app = initializeApp()
const db = initializeFirestore(app, { preferRest: true })

module.exports = {
  db
}

I then require db from individual functions. This setup is producing the following error:

Error: initializeFirestore() has already been called with different options. To avoid this error, call initializeFirestore() with the same options as when it was originally called, or call getFirestore() to return the already initialized instance.

Am I doing something obviously wrong? How do you suggest using this new initializeFirestore function?

lahirumaramba · 2022-12-16T19:07:03Z

I'm currently using a simple setup like this:
const { initializeApp } = require('firebase-admin/app')
const { initializeFirestore } = require('firebase-admin/firestore')

const app = initializeApp()
const db = initializeFirestore(app, { preferRest: true })

module.exports = {
  db
}
I then require db from individual functions. This setup is producing the following error:

Error: initializeFirestore() has already been called with different options. To avoid this error, call initializeFirestore() with the same options as when it was originally called, or call getFirestore() to return the already initialized instance.

Am I doing something obviously wrong? How do you suggest using this new initializeFirestore function?

Hi @GriffinJohnston, This error only occurs if you are trying to call initializeFirestore() with different options with the same app instance... I ran some tests with the code you shared and I am unable to reproduce the issue. Could you provide us with a minimal repro on this?

Folks, for any issues you are encountering with this feature please create a new issue. That way the team can easily track and address them separately. Thanks again for using this feature and giving us your feedback! We appreciate it!!

GriffinJohnston · 2022-12-16T19:13:09Z

@lahirumaramba Thanks for getting back to me - sorry for turning this into a support thread :)

I haven't been able to get initializeApp to work without that error, but I've got it working now with the following:

const { initializeApp } = require('firebase-admin/app')
const { getFirestore } = require('firebase-admin/firestore')

initializeApp()
getFirestore().settings({
  preferRest: true,
})

module.exports = {
  db: getFirestore(),
}

I'll open a new issue if I have futher questions, but could you confirm that using getFirestore().settings({preferRest: true}) should work correctly? If it does, I'm a little confused about the purpose of the new initializeFirestore method.

Thanks again for getting this out the door.

lahirumaramba · 2022-12-16T19:34:57Z

According to the @google-cloud/firestore docs, .settings({..}) should work if you set it before calling any other Firestore method.

Specifies custom settings to be used to configure the Firestore instance.
Can only be invoked once and before any other Firestore method.

If settings are provided via both settings() and the Firestore constructor, 
both settings objects are merged and any settings provided via settings() take precedence.

@alexander-fenster is currently looking into this.

@GriffinJohnston if you can reproduce the issue with initializeFirestore() please file a new issue with a minimal repro and we can investigate. Thanks!

omar-dev-amrk · 2023-01-10T07:32:29Z

Hey all
I have just a little question about using this new flag. Am I doing it correctly?

const admin = require("firebase-admin");
const { getFirestore } = require("firebase-admin/firestore");

admin.initializeApp({
  credential: admin.credential.cert(serviceAccount),
  storageBucket: SECRETS.storage.bucket,
});

getFirestore().settings({
  preferRest: true,
});

module.exports = admin;

// then outside this file

const users = await admin.firestore().collection("Users").get();

lahirumaramba · 2023-01-17T15:41:28Z

Hi @omar-dev-amrk, below is the correct way to use the API:

const { initializeApp } = require('firebase-admin/app')
const { initializeFirestore } = require('firebase-admin/firestore')

const app = initializeApp()
const db = initializeFirestore(app, { preferRest: true })

There was a known caching issue that we have discussed above that was fixed in #2040. We will include the fix in this week's release. Thanks!

omar-dev-amrk · 2023-01-18T10:58:55Z

Hey @lahirumaramba
Thank you for responding. I just tried it and I can confirm that it's actually working as expected.

Can I use it in production now, or would it be safer to wait for the caching issue fix?

lahirumaramba · 2023-01-19T17:51:31Z

Hey @omar-dev-amrk the fix is now included in the v11.5.0 release. Feel free to use it in production. If you encounter any problems, please open a new issue.

Folks, if you get a chance to try out this feature let us know what you think. Thanks!

omar-dev-amrk · 2023-02-02T15:24:15Z

Hello @lahirumaramba
I have deployed the preferRest option to production, and some weird downtimes are happening.

After the deployment finished, I kept getting errors like:

request to https://firestore.googleapis.com/v1/projects/PROJECT/databases/(default)/documents:commit? failed, reason: Client network socket disconnected before secure TLS connection was established at /workspace/app/orders/makeOrderDone.function.js:126:25

and

TypeError: callback is not a function
at /workspace/node_modules/google-gax/build/src/fallbackServiceStub.js:119:33
Function execution took 564 ms, finished with status: 'connection error'

Process exited with code 16 at process.<anonymous> (/layers/google.nodejs.functions-framework/functions-framework/node_modules/@google-cloud/functions-framework/build/src/invoker.js:92:22) at process.emit (events.js:400:28) at process.emit (domain.js:475:12) at process.exit (internal/process/per_thread.js:179:15) at sendCrashResponse (/layers/google.nodejs.functions-framework/functions-framework/node_modules/@google-cloud/functions-framework/build/src/logger.js:44:9) at process.<anonymous> (/layers/google.nodejs.functions-framework/functions-framework/node_modules/@google-cloud/functions-framework/build/src/invoker.js:88:44) at process.emit (events.js:400:28) at process.emit (domain.js:475:12) at processPromiseRejections (internal/process/promises.js:245:33) at processTicksAndRejections (internal/process/task_queues.js:96:32)

 Function execution took 228 ms, finished with status: 'crash'

Error: A backoff operation is already in progress. at QueryWatch.<anonymous> (/workspace/app/orders/makeOrderDone.function.js:179:27) at QueryWatch.closeStream (/workspace/node_modules/@google-cloud/firestore/build/src/watch.js:226:18) at /workspace/node_modules/@google-cloud/firestore/build/src/watch.js:335:18 at runMicrotasks (<anonymous>) at processTicksAndRejections (internal/process/task_queues.js:95:5) at runNextTicks (internal/process/task_queues.js:64:3) at listOnTimeout (internal/timers.js:526:9) at processTimers (internal/timers.js:500:7)

What's weird was that it works fine after a certain amount of errors, then goes back to erring again.

What can we do in the meantime?
This is causing major issues at the moment

lahirumaramba · 2023-02-02T23:54:16Z

@omar-dev-amrk Since Cloud Firestore support is provided by the @google-cloud/firestore library, I would suggest you to report this directly on the nodejs-firestore GitHub repo. Thanks!

alexander-fenster · 2023-02-04T00:05:17Z

Just FYI, I filed googleapis/gax-nodejs#1423 about "callback is not a function" error. It looks like it tries to fail the request for whatever reason but fails to call callback(err) or something like that. I'll take a look, must be an issue in google-gax.

Psycarlo · 2023-02-12T18:59:42Z

What is the recommended way to use preferRest -- for my code structure, that looks like the following:

index.ts

import { initializeApp } from 'firebase-admin/app'
[...]

initializeApp()

export {
<all my functions here>
}

example.ts

import * as functions from 'firebase-functions'
import { getFirestore } from 'firebase-admin/firestore'

export const exampleFunction = functions [...] {
  ...

  await getFirestore()
      .collection('collection') [...]
      
   ...
}

Doing the following on index.ts is enough?

const app = initializeApp()
initializeFirestore(app, { preferRest: true })

MarkDuckworth · 2023-02-13T16:22:12Z

Your approach looks good, it's aligned with this comment: #1901 (comment). Are you having trouble?

const app = initializeApp()
This gives you the default app instance.

initializeFirestore(app, { preferRest: true })
This is initializing the Firestore service for the default app instance and sets preferRest: true.

await getFirestore()
This gets the Firestore service for the default app instance, which was initialized with preferRest: true.

Psycarlo · 2023-02-13T19:32:51Z

When I turn on preferRest, I am always getting this warning:

Google API requested!
   - URL: "https://oauth2.googleapis.com/token"
   - Be careful, this may be a production service.

Is this something I need to worry about @MarkDuckworth ?

Maybe something to do with: googleapis/nodejs-firestore#1811

Psycarlo · 2023-02-19T21:18:22Z

@MarkDuckworth My E2E tests using playwright and firebase functions also fail in CI Github Actions

ishowta · 2023-05-01T18:57:02Z

It may be inappropriate to write here, but I took a simple measurement of performance

Memory 8GB

- first firestore get latency (cold start)

default          -> 600ms ~ 800ms
preferRest: true -> 400ms ~ 500ms

- firestore get latency

default          -> 15ms ~ 30ms
preferRest: true -> 60ms ~ 80ms


Memory 1GB

- first firestore get latency (cold start)

default          -> 800ms ~ 1000ms
preferRest: true -> 600ms ~ 800ms

- firestore get latency

default          -> 15ms ~ 30ms
preferRest: true -> 60ms ~ 80ms


Memory 512MB

- first firestore get latency (cold start)

default          -> 1.5s ~ 2.0s
preferRest: true -> 1.0s ~ 1.5s

- firestore get latency

default          -> 15ms ~ 30ms
preferRest: true -> 60ms ~ 80ms


Memory 128MB

- first firestore get latency (cold start)

default          -> 3.5s ~ 4.0s
preferRest: true -> 2.0s ~ 2.5s

- firestore get latency

default          -> 20ms ~ 80ms
preferRest: true -> 80ms ~ 120ms

HarshitPersona · 2023-05-25T08:31:50Z

const middleware = async (req, res) => {
try {
const apiKey = req.headers["x-api-key"];
if (!apiKey)
return res.status(401).send({ message: "A valid API Key is required!" });

const { initializeApp } = require("firebase-admin/app");
const {
  initializeFirestore,
  getFirestore,
} = require("firebase-admin/firestore");

if (!isAlreadyInitialized) {
  const app = initializeApp();
  initializeFirestore(app, { preferRest: true });
  isAlreadyInitialized = true;
}

const db = getFirestore();

const query = db.collection("apps").where("apiKey", "==", apiKey);

const querySnapshot = await query.get();
if (req.headers["testing"] === "yes") {
  return res.send({ tests: "fgfhfh" });
} - I am using this piece of code along with the preferRest option as suggested here but still, I am getting a latency of 5-6 seconds on cold start.

HarshitPersona · 2023-05-25T08:36:35Z

My package version is 11.8.0

{
"dependencies": {
"firebase-admin": "^11.8.0"
}
}

This seems to be the recommended technique: firebase/firebase-admin-node#1901 (comment)

Psycarlo · 2023-07-20T22:33:15Z

Any progress on this #1901 (comment) ?

MarkDuckworth · 2023-07-21T14:51:21Z

@Psycarlo, please open an issue and fill in the required fields so that we have enough context to help you resolve this. Thanks!

alexander-fenster force-pushed the preferRest branch from f0ab63e to b07ac82 Compare September 15, 2022 17:02

lahirumaramba self-requested a review September 15, 2022 18:17

lahirumaramba self-assigned this Sep 15, 2022

MarkDuckworth approved these changes Sep 15, 2022

View reviewed changes

lahirumaramba reviewed Sep 15, 2022

View reviewed changes

lahirumaramba assigned alexander-fenster and unassigned lahirumaramba Sep 15, 2022

feat: preferRest option for Firestore

44d2779

alexander-fenster force-pushed the preferRest branch from b07ac82 to 44d2779 Compare November 21, 2022 23:02

Merge branch 'master' into preferRest

b8f6fa1

alexander-fenster added 2 commits November 21, 2022 15:07

test: fix unit test

34453df

Merge branch 'preferRest' of github.com:alexander-fenster/firebase-ad…

624c951

…min-node into preferRest

ganey approved these changes Nov 22, 2022

View reviewed changes

lahirumaramba approved these changes Nov 22, 2022

View reviewed changes

lahirumaramba changed the title ~~feat: preferRest app option for Firestore~~ feat(fs): preferRest app option for Firestore Nov 22, 2022

lahirumaramba added release-note release:stage Stage a release candidate labels Nov 22, 2022

lahirumaramba requested changes Nov 22, 2022

View reviewed changes

docs: pr feedback: update comment

25ee0c3

lahirumaramba removed the release:stage Stage a release candidate label Nov 22, 2022

MarkDuckworth reviewed Nov 28, 2022

View reviewed changes

lahirumaramba mentioned this pull request Jan 13, 2023

fix(firestore): Fix PreferRest caching issue #2040

Merged

dconeybe mentioned this pull request Jan 31, 2023

Error: 14 UNAVAILABLE: 403:Forbidden googleapis/nodejs-firestore#1814

Open

omar-dev-amrk mentioned this pull request Feb 3, 2023

intermittent downtimes after preferRest enabled googleapis/nodejs-firestore#1823

Closed

alexander-fenster mentioned this pull request Feb 4, 2023

REST transport: callback is not a function googleapis/gax-nodejs#1423

Closed

gewfy mentioned this pull request Feb 7, 2023

Improve functions cold starts 29ki/29k#1751

Merged

This was referenced May 26, 2023

Getting Cold start latency of 5+ seconds even after using the preferRest option for firebase-admin #2195

Open

Getting Cold start latency of 5+ seconds even after using the preferRest option for firebase-admin googleapis/nodejs-firestore#1851

Closed

johnnyoshika added a commit to examind-ai/ltijs-firestore that referenced this pull request Jul 7, 2023

Pass in preferRest to initializeFirestore

b6f1b10

This seems to be the recommended technique: firebase/firebase-admin-node#1901 (comment)

thecodingwizard mentioned this pull request Feb 29, 2024

Improve Firebase Functions cold start times cpinitiative/usaco-guide#4356

Open

feat(fs): preferRest app option for Firestore #1901

feat(fs): preferRest app option for Firestore #1901

Conversation

alexander-fenster commented Sep 15, 2022 • edited Loading

MarkDuckworth left a comment

Choose a reason for hiding this comment

lahirumaramba left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IchordeDionysos Sep 19, 2022 • edited Loading

Choose a reason for hiding this comment

GriffinJohnston commented Oct 2, 2022

lahirumaramba commented Oct 3, 2022

si1k commented Nov 8, 2022

alexander-fenster commented Nov 8, 2022 • edited Loading

ishowta commented Nov 11, 2022

ganey commented Nov 11, 2022

nikcheerla commented Nov 18, 2022 • edited Loading

GriffinJohnston commented Nov 21, 2022

alexander-fenster commented Nov 21, 2022

lahirumaramba left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lahirumaramba left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IchordeDionysos commented Dec 16, 2022

GriffinJohnston commented Dec 16, 2022 • edited Loading

lahirumaramba commented Dec 16, 2022

GriffinJohnston commented Dec 16, 2022 • edited Loading

lahirumaramba commented Dec 16, 2022

omar-dev-amrk commented Jan 10, 2023

lahirumaramba commented Jan 17, 2023

omar-dev-amrk commented Jan 18, 2023

lahirumaramba commented Jan 19, 2023

omar-dev-amrk commented Feb 2, 2023 • edited Loading

lahirumaramba commented Feb 2, 2023 • edited Loading

alexander-fenster commented Feb 4, 2023

Psycarlo commented Feb 12, 2023

MarkDuckworth commented Feb 13, 2023

Psycarlo commented Feb 13, 2023 • edited Loading

Psycarlo commented Feb 19, 2023

ishowta commented May 1, 2023 • edited Loading

HarshitPersona commented May 25, 2023

HarshitPersona commented May 25, 2023

Psycarlo commented Jul 20, 2023

MarkDuckworth commented Jul 21, 2023

alexander-fenster commented Sep 15, 2022 •

edited

Loading

IchordeDionysos Sep 19, 2022 •

edited

Loading

alexander-fenster commented Nov 8, 2022 •

edited

Loading

nikcheerla commented Nov 18, 2022 •

edited

Loading

GriffinJohnston commented Dec 16, 2022 •

edited

Loading

GriffinJohnston commented Dec 16, 2022 •

edited

Loading

omar-dev-amrk commented Feb 2, 2023 •

edited

Loading

lahirumaramba commented Feb 2, 2023 •

edited

Loading

Psycarlo commented Feb 13, 2023 •

edited

Loading

ishowta commented May 1, 2023 •

edited

Loading