-
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 all 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,141 @@ | ||
| /* | ||
| * 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 EsqlResponseErrorCause, | ||
| EsqlResponseError, | ||
| extractEsqlEmbeddedError, | ||
| extractEsqlResponseErrorCause, | ||
| formatErrorCause, | ||
| } from './esql_response_error'; | ||
|
|
||
| describe('formatErrorCause', () => { | ||
| it('returns message from error type and reason', () => { | ||
| expect( | ||
| formatErrorCause({ | ||
| type: 'remote_transport_exception', | ||
| reason: 'ccs query failed', | ||
| }) | ||
| ).toBe('remote_transport_exception: ccs query failed'); | ||
| }); | ||
|
|
||
| it('returns message from root_cause when type and reason are missing', () => { | ||
| expect( | ||
| formatErrorCause({ | ||
| root_cause: [{ type: 'index_not_found_exception', reason: 'no such index [metrics-*]' }], | ||
| }) | ||
| ).toBe('index_not_found_exception: no such index [metrics-*]'); | ||
| }); | ||
|
|
||
| it('returns generic message for empty error object', () => { | ||
| expect(formatErrorCause({})).toBe('Elasticsearch returned an error'); | ||
| }); | ||
| }); | ||
|
|
||
| describe('extractEsqlResponseErrorCause', () => { | ||
| it('extracts error cause from response error object', () => { | ||
| expect( | ||
| extractEsqlResponseErrorCause({ | ||
| error: { type: 'remote_transport_exception', reason: 'ccs query failed' }, | ||
| }) | ||
| ).toEqual({ | ||
| type: 'remote_transport_exception', | ||
| reason: 'ccs query failed', | ||
| }); | ||
| }); | ||
|
|
||
| it('returns undefined when response has no error object', () => { | ||
| expect(extractEsqlResponseErrorCause({ columns: [], values: [] })).toBeUndefined(); | ||
| }); | ||
|
|
||
| it('returns undefined when error is null', () => { | ||
| expect(extractEsqlResponseErrorCause({ error: null })).toBeUndefined(); | ||
| }); | ||
|
|
||
| it('returns undefined when error is not an object', () => { | ||
| expect(extractEsqlResponseErrorCause({ error: 'not-an-object' })).toBeUndefined(); | ||
| }); | ||
| }); | ||
|
|
||
| describe('extractEsqlEmbeddedError', () => { | ||
| it('returns cause and top-level status when present', () => { | ||
| expect( | ||
| extractEsqlEmbeddedError({ | ||
| error: { type: 'remote_transport_exception', reason: 'ccs failed' }, | ||
| status: 400, | ||
| }) | ||
| ).toEqual({ | ||
| cause: { type: 'remote_transport_exception', reason: 'ccs failed' }, | ||
| status: 400, | ||
| }); | ||
| }); | ||
|
|
||
| it('omits status when absent or not a finite number', () => { | ||
| expect( | ||
| extractEsqlEmbeddedError({ | ||
| error: { type: 'x', reason: 'y' }, | ||
| }) | ||
| ).toEqual({ cause: { type: 'x', reason: 'y' } }); | ||
|
|
||
| expect( | ||
| extractEsqlEmbeddedError({ | ||
| error: { type: 'x', reason: 'y' }, | ||
| status: '400', | ||
| } as object) | ||
| ).toEqual({ cause: { type: 'x', reason: 'y' } }); | ||
|
|
||
| expect( | ||
| extractEsqlEmbeddedError({ | ||
| error: { type: 'x', reason: 'y' }, | ||
| status: Number.NaN, | ||
| }) | ||
| ).toEqual({ cause: { type: 'x', reason: 'y' } }); | ||
| }); | ||
| }); | ||
|
|
||
| describe('EsqlResponseError', () => { | ||
| it('extends Error with name, message, and copied fields', () => { | ||
| const err = new EsqlResponseError({ | ||
| type: 'illegal_argument_exception', | ||
| reason: 'bad request', | ||
| }); | ||
|
|
||
| expect(err).toBeInstanceOf(Error); | ||
| expect(err).toBeInstanceOf(EsqlResponseError); | ||
| expect(err.name).toBe('EsqlResponseError'); | ||
| expect(err.message).toBe('illegal_argument_exception: bad request'); | ||
| expect(err.type).toBe('illegal_argument_exception'); | ||
| expect(err.reason).toBe('bad request'); | ||
| expect(err.rootCause).toBeUndefined(); | ||
| }); | ||
|
|
||
| it('copies root_cause to rootCause', () => { | ||
| const rootCause = [{ type: 'shard_failure', reason: 'failed on node-1' }]; | ||
| const err = new EsqlResponseError({ root_cause: rootCause }); | ||
|
|
||
| expect(err.rootCause).toEqual(rootCause); | ||
| }); | ||
|
|
||
| it('normalizes null reason to undefined (Elasticsearch types allow null)', () => { | ||
| const cause = { type: 'x', reason: null } as EsqlResponseErrorCause; | ||
| const err = new EsqlResponseError(cause); | ||
|
|
||
| expect(err.reason).toBeUndefined(); | ||
| expect(err.message).toBe('x'); | ||
| }); | ||
|
|
||
| it('stores optional Elasticsearch payload status', () => { | ||
| const err = new EsqlResponseError( | ||
| { type: 'remote_transport_exception', reason: 'ccs failed' }, | ||
| { status: 400 } | ||
| ); | ||
|
|
||
| expect(err.status).toBe(400); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,72 @@ | ||
| /* | ||
| * 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". | ||
| */ | ||
|
|
||
| // TODO https://github.com/elastic/kibana/issues/260667 | ||
| 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 interface EsqlEmbeddedError { | ||
| readonly cause: EsqlResponseErrorCause; | ||
| readonly status?: number; | ||
| } | ||
|
|
||
| /** | ||
| * When Elasticsearch returns a body like `{ error: { type, reason }, status: 400 }`, | ||
| * returns the error cause and optional status from the payload. | ||
| */ | ||
| export const extractEsqlEmbeddedError = (response: object): EsqlEmbeddedError | undefined => { | ||
| if (!('error' in response) || response.error == null || typeof response.error !== 'object') { | ||
| return undefined; | ||
| } | ||
|
|
||
| const body = response as { status?: unknown }; | ||
| const status = | ||
| typeof body.status === 'number' && Number.isFinite(body.status) ? body.status : undefined; | ||
|
|
||
| return { | ||
|
jorgeoliveira117 marked this conversation as resolved.
|
||
| cause: response.error as EsqlResponseErrorCause, | ||
| ...(status !== undefined ? { status } : {}), | ||
| }; | ||
| }; | ||
|
|
||
| export const extractEsqlResponseErrorCause = ( | ||
|
jorgeoliveira117 marked this conversation as resolved.
|
||
| response: object | ||
| ): EsqlResponseErrorCause | undefined => extractEsqlEmbeddedError(response)?.cause; | ||
|
|
||
| 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[]; | ||
| public readonly status?: number; | ||
|
|
||
| constructor(errorCause: EsqlResponseErrorCause, options?: { status?: number }) { | ||
| 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; | ||
| this.status = options?.status; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,7 @@ import { | |
| MetricsExecutionContextAction, | ||
| MetricsExecutionContextName, | ||
| } from './execution_context_enums'; | ||
| import { EsqlResponseError, extractEsqlEmbeddedError } 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 embedded = extractEsqlEmbeddedError(response as object); | ||
| if (embedded) { | ||
| throw new EsqlResponseError(embedded.cause, { status: embedded.status }); | ||
| } | ||
|
|
||
| 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.