Skip to content

feat: add warning nudges for cross-region event triggers#10408

Draft
shettyvarun268 wants to merge 2 commits intonextfrom
clean-event-warning
Draft

feat: add warning nudges for cross-region event triggers#10408
shettyvarun268 wants to merge 2 commits intonextfrom
clean-event-warning

Conversation

@shettyvarun268
Copy link
Copy Markdown
Contributor

This PR implements robust cross-region latency warnings within the Firebase Functions deployment pipeline to nudge developers away from unnecessary cross-region network hops. When an event-triggered function (such as Firestore or Cloud Storage) is explicitly assigned or defaults to us-central1 while its backing event resource operates in a distinct non-US location, the CLI now issues a user-friendly warning nudge encouraging explicit collocation with the event trigger's region. This validation integrates globally following Gen 2 EventArc trigger resolution, correctly handles US multi-region databases (such as nam5) to avoid false positives, and leaves deployment executions unblocked to prevent service disruptions.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a warning to alert users when a function in us-central1 is triggered by a resource in a different region, helping to avoid unnecessary cross-region network hops. The review feedback suggests refining the logic to exclude the us multi-region to prevent false positives and improving test isolation by moving cache clearing to the beforeEach block in the test suite.

Comment on lines +21 to +37
// Warn if an event function defaults to or is assigned to us-central1 but its trigger is elsewhere,
// to avoid unnecessary cross-region network hops. We ignore nam5 since it covers us-central1.
for (const ep of backend.allEndpoints(want)) {
if (
ep.region === "us-central1" &&
backend.isEventTriggered(ep) &&
ep.eventTrigger?.region &&
ep.eventTrigger.region !== "us-central1" &&
ep.eventTrigger.region !== "nam5"
) {
utils.logLabeledWarning(
"functions",
`Function ${ep.id} located in us-central1 uses a trigger located in ${ep.eventTrigger.region}. ` +
`To avoid unnecessary cross-region network hops, you should explicitly assign this function to ${ep.eventTrigger.region}.`,
);
}
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The current logic triggers a warning for the us multi-region (commonly used for Cloud Storage), which includes us-central1. This results in a false positive warning for a valid configuration. Additionally, the suggestion to 'explicitly assign this function to ${ep.eventTrigger.region}' is not applicable for multi-regions like us or nam5 as they are not valid deployment regions for functions.

Consider adding us to the exclusion list and simplifying the loop logic.

  // Warn if an event function defaults to or is assigned to us-central1 but its trigger is elsewhere,
  // to avoid unnecessary cross-region network hops. We ignore nam5 and us since they cover us-central1.
  for (const ep of backend.allEndpoints(want)) {
    const triggerRegion = ep.eventTrigger?.region;
    if (
      ep.region === "us-central1" &&
      backend.isEventTriggered(ep) &&
      triggerRegion &&
      triggerRegion !== "us-central1" &&
      triggerRegion !== "nam5" &&
      triggerRegion !== "us"
    ) {
      utils.logLabeledWarning(
        "functions",
        `Function ${ep.id} located in us-central1 uses a trigger located in ${triggerRegion}. ` +
          `To avoid unnecessary cross-region network hops, you should explicitly assign this function to ${triggerRegion}.`,
      );
    }
  }

Comment on lines 22 to 27
beforeEach(() => {
storageStub = sinon.stub(storage, "getBucket").throws("unexpected call to storage.getBucket");
firestoreStub = sinon
.stub(firestore, "getDatabase")
.throws("unexpected call to firestore.getDatabase");
});
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

To ensure test isolation and prevent side effects from the Firestore service cache between test cases, firestoreService.clearCache() should be called in the beforeEach block.

Suggested change
beforeEach(() => {
storageStub = sinon.stub(storage, "getBucket").throws("unexpected call to storage.getBucket");
firestoreStub = sinon
.stub(firestore, "getDatabase")
.throws("unexpected call to firestore.getDatabase");
});
beforeEach(() => {
storageStub = sinon.stub(storage, "getBucket").throws("unexpected call to storage.getBucket");
firestoreStub = sinon
.stub(firestore, "getDatabase")
.throws("unexpected call to firestore.getDatabase");
firestoreService.clearCache();
});

Comment on lines +173 to +174
const logLabeledWarningStub = sinon.stub(utils, "logLabeledWarning");
firestoreService.clearCache();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This manual cache clearing is redundant if moved to the beforeEach block.

Suggested change
const logLabeledWarningStub = sinon.stub(utils, "logLabeledWarning");
firestoreService.clearCache();
const logLabeledWarningStub = sinon.stub(utils, "logLabeledWarning");

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants