-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Onboard Synthetics TLS rule type with FAAD #186551
Conversation
/ci |
/ci |
/ci |
/ci |
Pinging @elastic/response-ops (Team:ResponseOps) |
Pinging @elastic/obs-ux-infra_services-team (Team:obs-ux-infra_services) |
spaceId, | ||
indexedStartedAt, | ||
alertsLocator, | ||
basePath.publicBaseUrl | ||
), | ||
...updateState(ruleState, foundCerts), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ymao1 is the correct place for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume Ying has more knowledge here, but this doesn't seem quite right, especially since we're calling it again in the return of the executor. And it doesn't look these fields were in the context before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, I think you want to set the state during the initial reporting of the alert:
const { uuid, start } = alertsClient.report({
id: alertId,
actionGroup: TLS_CERTIFICATE.id,
state: { ...updateState(ruleState, foundCerts), ...summary },
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay thanks! I thought it was weird but wanted to make sure 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved in this commit 1068617
💚 Build Succeeded
Metrics [docs]History
To update your PR or re-run it, just comment with: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code LGTM, but left a question about the ...updateState(ruleState, foundCerts),
in setting the alert context variables.
spaceId, | ||
indexedStartedAt, | ||
alertsLocator, | ||
basePath.publicBaseUrl | ||
), | ||
...updateState(ruleState, foundCerts), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume Ying has more knowledge here, but this doesn't seem quite right, especially since we're calling it again in the return of the executor. And it doesn't look these fields were in the context before.
x-pack/plugins/observability_solution/synthetics/server/alert_rules/tls_rule/message_utils.ts
Show resolved
Hide resolved
@MiriamAparicio Were you able to generate active alerts and then recover & verify that the alert documents contain the expected information? |
hi @ymao1, |
Pinging @elastic/obs-ux-management-team (Team:obs-ux-management) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💚 Build Succeeded
Metrics [docs]History
|
Resolves: #169867
This PR onboards the Synthetics TLS rule type with FAAD.
I can't get this rule to alert easily to help verify, pls let me know if there is a good way to test.