Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add support for firealerts events in Eventarc emulator. #7355

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Add support for firealerts events in Eventarc emulator. #7355

wants to merge 13 commits into from

Conversation

GarrettBurroughs
Copy link

Description

Added support for native events within the event arc emulator and

Scenarios Tested

In a firebase functions project, created the following function:

export const handlePerformanceAlert = onThresholdAlertPublished((thresholdAlert) => {
  console.log(thresholdAlert);
});

Ran the emulator suite with:

$ firebase emulators:start

Then used the following curl command to "publish" an event:

curl -X POST -d '
{"events": [
{
  "alerttype": "performance.threshold",
  "id": "",
  "source": "//firebasealerts.googleapis.com/projects/12345",
  "specVersion": "1.0",
  "appid": "",
  "time": "2024-05-31T18:07:36.470521Z",
  "type": "google.firebase.firebasealerts.alerts.v1.published",
  "project": "",
  "data": {
    "@type": "type.googleapis.com/google.events.firebase.firebasealerts.v1.AlertData",
    "createTime": "2024-05-31T18:07:36.470521Z",
    "endTime": "2024-05-31T18:07:36.470521Z",
    "payload": {
      "metricType": "duration",
      "appVersion": "1 (1.0.0)",
      "violationValue": 0.205629,
      "thresholdUnit": "seconds",
      "violationUnit": "seconds",
      "@type": "type.googleapis.com/google.events.firebase.firebasealerts.v1.FireperfThresholdAlertPayload",
      "numSamples": "200",
      "eventName": "custom-trace-3",
      "thresholdValue": 0.15,
      "eventType": "duration_trace",
      "conditionPercentile": 90,
      "investigateUri": ""
    }
  }
}
]}
' http://localhost:9299/google/publishEvents

To which the emulator suite properly responded by running the handlePerformanceAlert function and logging the event to the emulator logs page.

@GarrettBurroughs GarrettBurroughs marked this pull request as draft June 21, 2024 19:30
blidd-google
blidd-google reviewed Jun 24, 2024
View reviewed changes
Copy link
Contributor

@blidd-google blidd-google left a comment

Choose a reason for hiding this comment

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

After reviewing the code, I'm not sure I'm convinced we need to handle custom and native events separately. The event handling logic for both cases is virtually the same; if we'd like to preserve the metadata identifying which triggers handle custom events vs. native events, we can just keep track of that in the event trigger interface.

Let's take a pass at this together and revisit that part of the design today.

src/emulator/eventarcEmulator.ts Outdated
);
}

async triggerNativeEventFunction(event: CloudEvent<any>) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm noticing that the triggerNativeEventFunction and triggerCustomEventFunction have a lot of duplicate logic and this feels like a good opportunity to apply the DRY principle. I think this is what Daniel was referring to in your design doc. Can we refactor this so that they share the same code path?

src/emulator/eventarcEmulator.ts Outdated
const customEventTriggers = this.customEvents[key] || [];
customEventTriggers.push({ projectId, triggerName, eventTrigger });
this.customEvents[key] = customEventTriggers;
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

Again here - I don't a compelling reason why we need to track the custom and native events in separate dictionaries.

src/emulator/eventarcEmulator.ts Outdated
@@ -95,17 +112,33 @@ export class EventarcEmulator implements EmulatorInstance {
});
};

const publishNativeEventsRoute = `/google/publishEvents`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here - the native events handler is essentially doing the same job as the custom events handler.

src/emulator/eventarcEmulator.ts
@@ -10,7 +10,7 @@ import { EmulatorRegistry } from "./registry";
import { FirebaseError } from "../error";
import { cloudEventFromProtoToJson } from "./eventarcEmulatorUtils";

interface CustomEventTrigger {
interface EmulatedEventTrigger {
Copy link
Contributor

Choose a reason for hiding this comment

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

*nit - how about EventarcTrigger? I don't feel strongly about this but it may make sense to apply a more specific naming convention, in case we add other event trigger interfaces for other emulated services.

blidd-google
blidd-google approved these changes Jun 25, 2024
View reviewed changes
Copy link
Contributor

@blidd-google blidd-google left a comment

Choose a reason for hiding this comment

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

Looks great - thanks for making these revisions! I'm going to approve, but let's wait for Daniel to take a look before merging

src/emulator/eventarcEmulator.ts
const publishEventsHandler: express.RequestHandler = (req, res) => {
const channel = `projects/${req.params.project_id}/locations/${req.params.location}/channels/${req.params.channel}`;
const isCustom = req.params.project_id && req.params.channel;
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

taeold
taeold reviewed Jun 28, 2024
View reviewed changes
src/emulator/eventarcEmulator.ts
method: "POST",
path: `/functions/projects/${trigger.projectId}/triggers/${trigger.triggerName}`,
body: JSON.stringify(event),
responseType: "stream",
Copy link
Contributor

Choose a reason for hiding this comment

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

q: why is the response type here stream?

Copy link
Author

Choose a reason for hiding this comment

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

I am not 100% of this either. I know it is showing up as a diff here, but that's just because I extracted out some of the previous code into a function. I am inclined to leave it this way as it is working fine with supporting the new events and I don't want to break anything that was working previously.

@GarrettBurroughs GarrettBurroughs marked this pull request as ready for review July 2, 2024 14:57
joehan
joehan reviewed Jul 10, 2024
View reviewed changes
src/emulator/functionsEmulator.ts
const service = getFunctionService(definition);
const key = this.getTriggerKey(definition);

switch (service) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Note to self/Brian/Daniel - we might need to add PubSub triggers here in to fix #6585

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

@taeold taeold taeold left review comments

@joehan joehan joehan approved these changes

@blidd-google blidd-google blidd-google approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@GarrettBurroughs @joehan @taeold @blidd-google