-
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
[Alerting]: harden APIs of built-in alert index-threshold #60702
[Alerting]: harden APIs of built-in alert index-threshold #60702
Conversation
Pinging @elastic/kibana-alerting-services (Team:Alerting Services) |
389055e
to
5130327
Compare
resolves elastic#59889 The index threshold APIs - used by both the index threshold UI and the alert executor - were returning errors (500's from http endpoints) when getting errors from ES. These have been changed so that the error is logged as a warning, and the relevant API returns an "empty" result. Another 500 response was found while experimenting with this. Apparently the date_range agg requires a date format to be passed in if the date format in ES is not an ISO date. The repro on this was to select the `.security` alias (or it's index) within the index threshold alert UI, and then select one of it's date fields.
5130327
to
8e78f89
Compare
@@ -94,6 +94,7 @@ export async function timeSeriesQuery( | |||
dateAgg: { | |||
date_range: { | |||
field: timeField, | |||
format: 'strict_date_time', |
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.
This was added to indicate the format of the dates that we pass in as the date range values. Without it, an error will occur if the dates in the index are not in this format, eg epoch_millis
.
@@ -134,8 +135,8 @@ export async function timeSeriesQuery( | |||
esResult = await callCluster('search', esQuery); | |||
} catch (err) { | |||
// console.log('time_series_query.ts error\n', JSON.stringify(err, null, 4)); | |||
logger.warn(`${logPrefix} error: ${JSON.stringify(err.message)}`); | |||
throw new Error('error running search'); |
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.
Instead of throwing an error, just return an empty result. Also improve the log message (doesn't need to be stringified).
|
||
let result: TimeSeriesResult; | ||
try { |
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.
try/catch was removed, as we now catch the only directly throwable API (above), so anything else would be a JS runtime error.
@@ -53,6 +53,7 @@ async function createEsDocument(es: any, epochMillis: number, testedValue: numbe | |||
source: DOCUMENT_SOURCE, | |||
reference: DOCUMENT_REFERENCE, | |||
date: new Date(epochMillis).toISOString(), | |||
date_epoch_millis: epochMillis, |
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.
added a new date field, but in epoch millis format, to test using it as a time field
@@ -139,6 +139,12 @@ export default function fieldsEndpointTests({ getService }: FtrProviderContext) | |||
expect(field.name).to.eql('updated_at'); | |||
expect(field.type).to.eql('date'); | |||
}); | |||
|
|||
// TODO: the pattern '*a:b,c:d*' throws an exception in dev, but not ci! |
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.
This is a bit weird. Seems like FT is running ES in some mode where bad cluster names won't result in exceptions, but running yarn es
will. If you enter the pattern above when running in dev (yarn es, yarn start), you'll see the exception logged in the console, but you don't see it in in FT.
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.
LGTM!
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.
LGTM
@elasticmachine merge upstream |
@elasticmachine merge upstream |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
) resolves elastic#59889 The index threshold APIs - used by both the index threshold UI and the alert executor - were returning errors (500's from http endpoints) when getting errors from ES. These have been changed so that the error is logged as a warning, and the relevant API returns an "empty" result. Another 500 response was found while experimenting with this. Apparently the date_range agg requires a date format to be passed in if the date format in ES is not an ISO date. The repro on this was to select the `.security` alias (or it's index) within the index threshold alert UI, and then select one of it's date fields.
…60813) resolves #59889 The index threshold APIs - used by both the index threshold UI and the alert executor - were returning errors (500's from http endpoints) when getting errors from ES. These have been changed so that the error is logged as a warning, and the relevant API returns an "empty" result. Another 500 response was found while experimenting with this. Apparently the date_range agg requires a date format to be passed in if the date format in ES is not an ISO date. The repro on this was to select the `.security` alias (or it's index) within the index threshold alert UI, and then select one of it's date fields.
resolves #59889
The index threshold APIs - used by both the index threshold UI and the
alert executor - were returning errors (500's from http endpoints) when
getting errors from ES.
These have been changed so that the error is logged as a warning, and the
relevant API returns an "empty" result.
Another 500 response was found while experimenting with this. Apparently
the date_range agg requires a date format to be passed in if the date format
in ES is not the format sent in the date range values - in this case an ISO date.
The repro on this was to select the
.security
alias (or it's index) within theindex threshold alert UI, and then select one of it's date fields, which are
formatted as epoch_millis, not ISO.
Checklist
Delete any items that are not applicable to this PR.