feat: health check - can connect to firestore #97
Conversation
github-actions
bot
added
type: feature request
There was a problem hiding this comment.
Hi Tony, was just browsing through and I understand this was already submitted, but wanted to revisit. Just a suggestion, thoughts on swapping out using Promise.race and setTimeout (in src/lib/database.ts) and leverage setInterval? So you can poll this.db.listCollections a certain amount of times rather than giving an arbitrary number for the db to race against? That path could tighten up your isConnected function.
| // eslint-disable-next-line no-undef | ||
| let timerId: NodeJS.Timeout; | ||
|
|
||
| const timer = new Promise<boolean>((resolve) => { |
There was a problem hiding this comment.
If you are keeping setTimeout, could you add a unit test or 2 to ensure this is working as expected would be nice. Make sure edgecases are handled.
There was a problem hiding this comment.
Not sure I understand. No need to poll; if listCollections resolves first, it "wins" the race and the function returns. Are you saying you'd like to have a certain number of retries, or perhaps exponential backoff or something? Originally @rogerthatdev said he was happy with the API's default timeout (60s); that just seemed a bit too long, so I parameterized it. If you think there's a need for retries, do you mind opening an issue?
There was a problem hiding this comment.
Or for sure, will file one 👍🏼 Also I know I'm coming in from left-field a bit - ty for the explanation 🎊
| }); | ||
|
|
||
| } catch (err) { | ||
| // GoogleError: Total timeout of API google.firestore.v1.Firestore |
There was a problem hiding this comment.
Not a fan of silent errors. Anyway to capture the errors rather than just seeing isConnected returned false.
There was a problem hiding this comment.
Yeah, I thought about that. Bottom line is it's not healthy yet. If not the expected error, it could be logged if this turns out to be problematic.
There was a problem hiding this comment.
Mind if I file an issue for this as well to do a revisit?
| ) { | ||
| const fs = new Database(); | ||
| let isConnected = await fs.isConnected(); | ||
| let statusCode = isConnected ? 200 : 503; |
There was a problem hiding this comment.
nit: could this possibly return some information about the firestore and if errors were emitted that could get bundled in like:
{
status: 503,
error: {...} // Emitted error after polling is completed without firestore connection
}
There was a problem hiding this comment.
Hmm, not sure I really see a need for this. This is specifically what the fscheck health check checks; either it can connect (200) or it can't (503). I even considered just doing this as a HEAD request, but figured a GET might be easier for some callers to check. If you think more is needed, please open an issue.
Adds a health check GET endpoint that confirms app can connect to Firestore:
/api/fscheckThe endpoint returns either an HTTP
200or503.