-
Notifications
You must be signed in to change notification settings - Fork 8.6k
[Metrics][Discover] internal/search/esql_async returns 200 but METRICS_INFO responds with error #260237
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
[Metrics][Discover] internal/search/esql_async returns 200 but METRICS_INFO responds with error #260237
Changes from 7 commits
80e046f
52148fc
e341ef8
384c1cf
7fb6e2d
2ab2a04
4efea20
4e87b71
8c32495
b4bb2ed
3aa02e1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,51 @@ | ||
| /* | ||
| * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
| * or more contributor license agreements. Licensed under the "Elastic License | ||
| * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side | ||
| * Public License v 1"; you may not use this file except in compliance with, at | ||
| * your election, the "Elastic License 2.0", the "GNU Affero General Public | ||
| * License v3.0 only", or the "Server Side Public License, v 1". | ||
| */ | ||
|
|
||
| import type { estypes } from '@elastic/elasticsearch'; | ||
|
|
||
| export type EsqlResponseErrorCause = Partial<estypes.ErrorCause>; | ||
|
|
||
| export const formatErrorCause = (errorCause: EsqlResponseErrorCause): string => { | ||
| const head = [errorCause.type, errorCause.reason] | ||
| .filter((value): value is string => Boolean(value?.trim())) | ||
| .join(': '); | ||
| if (head) { | ||
| return head; | ||
| } | ||
|
|
||
| const rootCause = errorCause.root_cause?.[0]; | ||
|
jorgeoliveira117 marked this conversation as resolved.
|
||
| const fromRootCause = [rootCause?.type, rootCause?.reason] | ||
| .filter((value): value is string => Boolean(value?.trim())) | ||
| .join(': '); | ||
| return fromRootCause || 'Elasticsearch returned an error'; | ||
| }; | ||
|
|
||
| export const extractEsqlResponseErrorCause = ( | ||
| response: object | ||
| ): EsqlResponseErrorCause | undefined => { | ||
| if (!('error' in response) || response.error == null || typeof response.error !== 'object') { | ||
| return undefined; | ||
| } | ||
|
|
||
| return response.error as EsqlResponseErrorCause; | ||
| }; | ||
|
|
||
| export class EsqlResponseError extends Error { | ||
|
jorgeoliveira117 marked this conversation as resolved.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is probably fine for now, but we should not implement custom error messages. If such handlers are needed, they should be handled at the ESQL level rather than on our side.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Totally agree, we created this issue to handle things like this and more |
||
| public readonly type?: string; | ||
| public readonly reason?: string; | ||
| public readonly rootCause?: EsqlResponseErrorCause[]; | ||
|
|
||
| constructor(errorCause: EsqlResponseErrorCause) { | ||
| super(formatErrorCause(errorCause)); | ||
| this.name = 'EsqlResponseError'; | ||
| this.type = errorCause.type; | ||
| this.reason = errorCause.reason ?? undefined; | ||
|
jorgeoliveira117 marked this conversation as resolved.
|
||
| this.rootCause = errorCause.root_cause; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,7 @@ import { | |
| MetricsExecutionContextAction, | ||
| MetricsExecutionContextName, | ||
| } from './execution_context_enums'; | ||
| import { EsqlResponseError, extractEsqlResponseErrorCause } from './esql_response_error'; | ||
| import { esqlResultToPlainObjects } from './esql_result_to_plain_objects'; | ||
| import { getMetricsExecutionContext } from './execution_context'; | ||
|
|
||
|
|
@@ -33,8 +34,21 @@ export interface ExecuteEsqlParams { | |
| uiSettings: IUiSettingsClient; | ||
| } | ||
|
|
||
| export const fetchEsqlResponseOrThrow = async ( | ||
| params: Parameters<typeof getESQLResults>[0] | ||
| ): Promise<Awaited<ReturnType<typeof getESQLResults>>['response']> => { | ||
| const { response } = await getESQLResults(params); | ||
| const errorCause = extractEsqlResponseErrorCause(response); | ||
| if (errorCause) { | ||
| throw new EsqlResponseError(errorCause); | ||
| } | ||
|
|
||
| return response; | ||
| }; | ||
|
|
||
| /** | ||
| * Executes an ES|QL query using the data plugin's search service. | ||
| * Rejects when Elasticsearch returns a response body that contains an `error` object. | ||
| */ | ||
| export async function executeEsqlQuery<TDocument extends object = Record<string, unknown>>({ | ||
| esqlQuery, | ||
|
|
@@ -57,7 +71,7 @@ export async function executeEsqlQuery<TDocument extends object = Record<string, | |
| ? buildEsQuery(undefined, [], filtersWithTime, esQueryConfig) | ||
| : undefined; | ||
|
|
||
| const { response } = await getESQLResults({ | ||
| const response = await fetchEsqlResponseOrThrow({ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A try/catch handler would likely handle the same |
||
| esqlQuery, | ||
| search, | ||
| signal, | ||
|
|
@@ -70,7 +84,5 @@ export async function executeEsqlQuery<TDocument extends object = Record<string, | |
| ), | ||
| }); | ||
|
|
||
| const plainObjects = esqlResultToPlainObjects<TDocument>(response); | ||
|
|
||
| return plainObjects; | ||
| return esqlResultToPlainObjects<TDocument>(response); | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.