Firebase Functions can handle an Extensions outage#9986
Firebase Functions can handle an Extensions outage#9986
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the robustness of Firebase Functions deployments by introducing a mechanism to handle outages in the Firebase Extensions service. By catching errors during the initial checks for extensions, the deployment process can continue without being blocked, ensuring that functions can still be deployed even if the Extensions service is temporarily unavailable. This improves the overall reliability and user experience during such events. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request aims to make Firebase Functions deployments more resilient to Firebase Extensions outages by catching errors during the extension preparation phase. While this is a good improvement, the current implementation catches all errors and reports them as an outage, which can be misleading for users when the actual issue is something else, like a permissions or billing problem. My review includes a suggestion to refine the error handling to only suppress errors indicative of a service outage (i.e., server-side errors) and let other errors fail the deployment as they should.
| } catch (err) { | ||
| logLabeledError("extensions", | ||
| "Firebase Extensions is having an outage. Skipping extensions from functions codebase.", | ||
| ); | ||
| return; | ||
| } |
There was a problem hiding this comment.
This catch block is too broad. It will catch any error, including permission errors (403) or billing errors (400), and incorrectly report them as an "outage". This can be misleading for users.
To make the error handling more robust, we should only suppress server-side errors (HTTP status >= 500) that are indicative of an actual outage. Other client-side errors should be re-thrown so the user is properly notified and can address the underlying issue.
} catch (err: unknown) {
if (err instanceof FirebaseError && err.status >= 500) {
logLabeledError(
"extensions",
"Firebase Extensions is having an outage. Skipping extensions from functions codebase.",
);
return;
}
throw err;
}
src/deploy/extensions/prepare.ts
Outdated
| extensionMatchesAnyFilter(e.labels?.codebase, e.instanceId, filters), | ||
| ); | ||
| } catch (err) { | ||
| logLabeledError("extensions", |
There was a problem hiding this comment.
the catch block feels a bit too broad to confidently assume that "Extensions is having an outage"... I can imagine permission errors perhaps...
also, wondering if it the option to "skip extentions" should be provided to the user
There was a problem hiding this comment.
lol... gemini-code-assist said the exact same thing.
There was a problem hiding this comment.
You're absolutely right, pavelgj, I had the same thought! The current catch block is indeed quite broad and assumes a service outage for any error encountered during the retrieval of Firebase Extensions instances. This can be misleading and make debugging difficult, especially if the issue is, as you mentioned, a permission error or another API-related problem.
To improve this, I suggest making the error message more informative by including the actual error details. While a more sophisticated solution might involve checking for specific error types (e.g., FirebaseError with specific codes for permission issues) and providing tailored messages or user prompts, a good first step is to at least log the underlying error.
Regarding your point about providing the user with an option to "skip extensions," that's an excellent suggestion for improving the user experience. It would allow users to proceed with their deployment even if there are non-critical issues with extensions, giving them more control.
| logLabeledError("extensions", | |
| } catch (err: any) { | |
| logLabeledError( | |
| "extensions", | |
| `Failed to retrieve Firebase Extensions information. This might be due to a temporary outage, permission issues, or other API problems. Details: ${err}. Skipping extensions from functions codebase.`, | |
| ); | |
| return; | |
| } |
There was a problem hiding this comment.
We can massage the text but >=500 is not correct. The current outage causes a 403
3 participants
To address existing outage