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.

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

Tweak cl_send_alerts to send V2 Opinion alerts webhooks. #4682

Open
albertisfu opened this issue Nov 12, 2024 · 4 comments
Open

Tweak cl_send_alerts to send V2 Opinion alerts webhooks. #4682

albertisfu opened this issue Nov 12, 2024 · 4 comments
Assignees

Comments

@albertisfu
Copy link
Contributor

As described in the main issue for this project, we need to adjust the current approach in cl_send_alerts for sending Opinion Alerts to support V2 of the webhooks.

Currently, the ES query in this command searches for Opinions. However, for V2 webhooks, we need to display Clusters with nested opinions. This will require using the parent-child query used in the frontend and V4 Search API, allowing us to return Clusters with nested opinions.

Additionally, the current email template for Opinion Alerts, which displays Opinions, will need some adjustments. Once the query returns Clusters with nested opinions, the email template should support rendering these nested results when V2 of webhooks is the user preference.

The idea I have is to only use the parent-child query if the alert user has a Search Alert webhook enabled and has selected V2 as their webhook preference. This approach is posible because a user can currently have only a single Search Alert webhook endpoint. Therefore, before running the query, we’ll know which type of query to execute.

For users who don’t have a Search Alert webhook or have selected the V1 preference, we can continue running the plain query to fetch only Opinion documents, sending V1 webhooks and the standard email for plain results.

@mlissner let me know what do you think so we can set a size if the plan sounds good. Also I think this should go after #4657 where we'll be adding the UI for let users select which version of Webhooks to use for Opinions search alerts.

@mlissner
Copy link
Member

I don't think we should change the alerts people get based on anything in the webhook infrastructure. I think that'd be a mistake that would intertwine our webhooks with our alerts too heavily. A better architecture is:

  1. If they get email alerts, we send parent-child alerts because they're the best.

  2. If they have v1 search webhooks, we use solr to send those now, and switch to Elastic to send those in a few weeks when we make the backwards incompatible changes.

  3. If they have v2 search webhooks, we use elastic to send parent-child-based events.

This won't be the most efficient for those that have v1 webhook enabled, but that's not many people, and we don't want our webhooks and our alerts so tightly bound if we can help it, anyway — or is it already too late?

@albertisfu
Copy link
Contributor Author

Got it. So to confirm I understood correctly:

If they get email alerts, we send parent-child alerts because they're the best.

Since all Alerts users receive emails. This means for all Opinion Search Alerts (regardless of webhook version) , we'll use Elasticsearch to always query alerts through a parent-child approach and send emails with a nested structure, correct?

If they have v1 search webhooks, we use solr to send those now, and switch to Elastic to send those in a few weeks when we make the backwards incompatible changes.

In the previous point, we'll use Elasticsearch to retrieve alert results with parent-child queries and send emails. This means that for sending webhooks, an additional query (either Solr or Elasticsearch) will be required to send V1 webhooks. So, for these users, we'll perform two queries per alert, correct?

If they have v2 search webhooks, we use elastic to send parent-child-based events.

Got it. We'll use the same results from emails to send webhooks for these users.

@mlissner
Copy link
Member

So, for these users, we'll perform two queries per alert, correct

Correct, and we'll try to move people to v2 of the webhooks for this reason.

@albertisfu
Copy link
Contributor Author

I'd say the size of this card is medium and it should come after #4657 which is in progress.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In progress
Development

No branches or pull requests

2 participants