diff --git a/server/routes/agent_routes.test.ts b/server/routes/agent_routes.test.ts index ad570c4c..4872ba78 100644 --- a/server/routes/agent_routes.test.ts +++ b/server/routes/agent_routes.test.ts @@ -125,7 +125,7 @@ describe('test execute agent route', () => { "headers": Object {}, "payload": Object { "error": "Internal Server Error", - "message": "Execute agent failed!", + "message": "An internal server error occurred.", "statusCode": 500, }, "statusCode": 500, diff --git a/server/routes/agent_routes.ts b/server/routes/agent_routes.ts index 826d740d..db3a5afb 100644 --- a/server/routes/agent_routes.ts +++ b/server/routes/agent_routes.ts @@ -7,6 +7,7 @@ import { schema } from '@osd/config-schema'; import { IRouter } from '../../../../src/core/server'; import { AGENT_API } from '../../common/constants/llm'; import { AssistantServiceSetup } from '../services/assistant_service'; +import { handleError } from './error_handler'; export function registerAgentRoutes(router: IRouter, assistantService: AssistantServiceSetup) { router.post( @@ -39,18 +40,7 @@ export function registerAgentRoutes(router: IRouter, assistantService: Assistant ); return res.ok({ body: response }); } catch (e) { - context.assistant_plugin.logger.error('Execute agent failed!', e); - if (e.statusCode >= 400 && e.statusCode <= 499) { - return res.customError({ - body: { message: typeof e.body === 'string' ? e.body : JSON.stringify(e.body) }, - statusCode: e.statusCode, - }); - } else { - return res.customError({ - body: 'Execute agent failed!', - statusCode: 500, - }); - } + return handleError(e, res, context.assistant_plugin.logger); } }) ); diff --git a/server/routes/error_handler.test.ts b/server/routes/error_handler.test.ts new file mode 100644 index 00000000..8ce7278d --- /dev/null +++ b/server/routes/error_handler.test.ts @@ -0,0 +1,83 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +import { handleError } from './error_handler'; +import { loggerMock } from '../../../../src/core/server/logging/logger.mock'; +import { AgentNotFoundError } from './errors'; +import { opensearchDashboardsResponseFactory } from '../../../../src/core/server'; + +describe('Error handler', () => { + it('should return 404 not found response if error is AgentNotFoundError', () => { + const mockedLogger = loggerMock.create(); + const error = handleError( + new AgentNotFoundError('test error'), + opensearchDashboardsResponseFactory, + mockedLogger + ); + expect(error.status).toBe(404); + expect(error.options.body).toMatchInlineSnapshot('"Agent not found"'); + }); + + it('should return 4xx with original error body', () => { + const mockedLogger = loggerMock.create(); + const error = handleError( + { + statusCode: 429, + body: { + status: 429, + error: { + type: 'OpenSearchStatusException', + reason: 'System Error', + details: 'Request is throttled at model level.', + }, + }, + }, + opensearchDashboardsResponseFactory, + mockedLogger + ); + expect(error.status).toBe(429); + expect(error.options.body).toMatchInlineSnapshot(` + Object { + "message": "{\\"status\\":429,\\"error\\":{\\"type\\":\\"OpenSearchStatusException\\",\\"reason\\":\\"System Error\\",\\"details\\":\\"Request is throttled at model level.\\"}}", + } + `); + }); + + it('shuld return generic 5xx error', () => { + const mockedLogger = loggerMock.create(); + const error = handleError( + { + statusCode: 502, + body: { + status: 502, + error: { + type: 'OpenSearchStatusException', + reason: 'System Error', + details: 'Some bad thing happened', + }, + }, + }, + opensearchDashboardsResponseFactory, + mockedLogger + ); + expect(error.status).toBe(502); + + // No extra info should returned + expect(error.payload).toBe(undefined); + expect(error.options.body).toBe(undefined); + }); + + it('should return generic internalError for unhandled server-side issues', () => { + const mockedLogger = loggerMock.create(); + const error = handleError( + new Error('Arbitrary Error'), + opensearchDashboardsResponseFactory, + mockedLogger + ); + expect(error.status).toBe(500); + expect(error.payload).toEqual('Internal Error'); + expect(error.options).toMatchInlineSnapshot('Object {}'); + }); +}); diff --git a/server/routes/error_handler.ts b/server/routes/error_handler.ts new file mode 100644 index 00000000..f8c4f017 --- /dev/null +++ b/server/routes/error_handler.ts @@ -0,0 +1,33 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +import { Logger, OpenSearchDashboardsResponseFactory } from '../../../../src/core/server'; +import { AgentNotFoundError } from './errors'; + +// eslint-disable-next-line @typescript-eslint/no-explicit-any +export const handleError = (e: any, res: OpenSearchDashboardsResponseFactory, logger: Logger) => { + logger.error('Error occurred', e); + // Handle specific type of Errors + if (e instanceof AgentNotFoundError) { + return res.notFound({ body: 'Agent not found' }); + } + + // handle http response error of calling backend API + if (e.statusCode) { + if (e.statusCode >= 400 && e.statusCode <= 499) { + return res.customError({ + body: { message: typeof e.body === 'string' ? e.body : JSON.stringify(e.body) }, + statusCode: e.statusCode, + }); + } else { + return res.customError({ + statusCode: e.statusCode, + }); + } + } + + // Return an general internalError for unhandled server-side issues + return res.internalError(); +}; diff --git a/server/routes/errors.ts b/server/routes/errors.ts new file mode 100644 index 00000000..5483b595 --- /dev/null +++ b/server/routes/errors.ts @@ -0,0 +1,11 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +export class AgentNotFoundError extends Error { + constructor(message: string) { + super(message); + this.message = message; + } +} diff --git a/server/routes/get_agent.ts b/server/routes/get_agent.ts index 51e57305..e94ae6b0 100644 --- a/server/routes/get_agent.ts +++ b/server/routes/get_agent.ts @@ -5,64 +5,55 @@ import { OpenSearchClient } from '../../../../src/core/server'; import { ML_COMMONS_BASE_API } from '../utils/constants'; +import { AgentNotFoundError } from './errors'; -/** - * - */ export const getAgentIdByConfigName = async ( configName: string, client: OpenSearchClient['transport'] ): Promise => { - try { - const path = `${ML_COMMONS_BASE_API}/config/${configName}`; - const response = await client.request({ - method: 'GET', - path, - }); + const path = `${ML_COMMONS_BASE_API}/config/${configName}`; + const response = await client.request({ + method: 'GET', + path, + }); - if ( - !response || - !(response.body.ml_configuration?.agent_id || response.body.configuration?.agent_id) - ) { - throw new Error(`cannot get agent ${configName} by calling the api: ${path}`); - } - return response.body.ml_configuration?.agent_id || response.body.configuration.agent_id; - } catch (error) { - const errorMessage = JSON.stringify(error.meta?.body) || error; - throw new Error(`get agent ${configName} failed, reason: ${errorMessage}`); + if ( + !response || + !(response.body.ml_configuration?.agent_id || response.body.configuration?.agent_id) + ) { + throw new AgentNotFoundError( + `cannot get agent by config name ${configName} by calling the api: ${path}` + ); } + return response.body.ml_configuration?.agent_id || response.body.configuration.agent_id; }; export const searchAgent = async ( { name }: { name: string }, client: OpenSearchClient['transport'] ) => { - try { - const requestParams = { - query: { - term: { - 'name.keyword': name, - }, - }, - _source: ['_id'], - sort: { - created_time: 'desc', + const requestParams = { + query: { + term: { + 'name.keyword': name, }, - size: 1, - }; + }, + _source: ['_id'], + sort: { + created_time: 'desc', + }, + size: 1, + }; - const response = await client.request({ - method: 'GET', - path: `${ML_COMMONS_BASE_API}/agents/_search`, - body: requestParams, - }); + const path = `${ML_COMMONS_BASE_API}/agents/_search`; + const response = await client.request({ + method: 'GET', + path, + body: requestParams, + }); - if (!response || response.body.hits.total.value === 0) { - return undefined; - } - return response.body.hits.hits[0]._id; - } catch (error) { - const errorMessage = JSON.stringify(error.meta?.body) || error; - throw new Error(`search ${name} agent failed, reason: ` + errorMessage); + if (!response || response.body.hits.total.value === 0) { + throw new AgentNotFoundError(`cannot find agent by name ${name} by calling the api: ${path}`); } + return response.body.hits.hits[0]._id as string; }; diff --git a/server/routes/summary_routes.test.ts b/server/routes/summary_routes.test.ts index 8033bc8b..0b39a9f0 100644 --- a/server/routes/summary_routes.test.ts +++ b/server/routes/summary_routes.test.ts @@ -13,6 +13,7 @@ import { SUMMARY_ASSISTANT_API } from '../../common/constants/llm'; import { registerData2SummaryRoutes, registerSummaryAssistantRoutes } from './summary_routes'; import { AssistantClient } from '../services/assistant_client'; import { RequestHandlerContext } from '../../../../src/core/server'; +import * as AgentHelpers from './get_agent'; const mockedLogger = loggerMock.create(); export const createMockedAssistantClient = ( @@ -147,7 +148,7 @@ describe('test summary route', () => { "headers": Object {}, "payload": Object { "error": "Internal Server Error", - "message": "Execute agent failed!", + "message": "An internal server error occurred.", "statusCode": 500, }, "statusCode": 500, @@ -167,9 +168,10 @@ describe('test summary route', () => { }, }, }); + const spy = jest.spyOn(AgentHelpers, 'getAgentIdByConfigName').mockResolvedValue('test_agent'); const result = (await insightRequest({ summaryType: 'alerts', - insightType: 'test', + insightType: 'os_insight', summary: 'summary', question: 'Please summarize this alert, do not use any tool.', context: 'context', @@ -185,6 +187,7 @@ describe('test summary route', () => { "statusCode": 429, } `); + spy.mockRestore(); }); it('return 4xx when executeAgent throws 4xx error in string format for insight API', async () => { @@ -192,9 +195,10 @@ describe('test summary route', () => { statusCode: 429, body: 'Request is throttled at model level', }); + const spy = jest.spyOn(AgentHelpers, 'getAgentIdByConfigName').mockResolvedValue('test_agent'); const result = (await insightRequest({ summaryType: 'alerts', - insightType: 'test', + insightType: 'os_insight', summary: 'summary', question: 'Please summarize this alert, do not use any tool.', context: 'context', @@ -210,6 +214,7 @@ describe('test summary route', () => { "statusCode": 429, } `); + spy.mockRestore(); }); it('return 5xx when executeAgent throws 5xx for insight API', async () => { @@ -217,9 +222,10 @@ describe('test summary route', () => { statusCode: 500, body: 'Server error', }); + const spy = jest.spyOn(AgentHelpers, 'getAgentIdByConfigName').mockResolvedValue('test_agent'); const result = (await insightRequest({ summaryType: 'alerts', - insightType: 'test', + insightType: 'os_insight', summary: 'summary', question: 'Please summarize this alert, do not use any tool.', context: 'context', @@ -229,12 +235,13 @@ describe('test summary route', () => { "headers": Object {}, "payload": Object { "error": "Internal Server Error", - "message": "Execute agent failed!", + "message": "An internal server error occurred.", "statusCode": 500, }, "statusCode": 500, } `); + spy.mockRestore(); }); it('return 4xx when execute agent throws 4xx error for data2Summary API', async () => { @@ -312,7 +319,7 @@ describe('test summary route', () => { "headers": Object {}, "payload": Object { "error": "Internal Server Error", - "message": "Execute agent failed!", + "message": "An internal server error occurred.", "statusCode": 500, }, "statusCode": 500, diff --git a/server/routes/summary_routes.ts b/server/routes/summary_routes.ts index 9d4aa9c6..dd23ff3b 100644 --- a/server/routes/summary_routes.ts +++ b/server/routes/summary_routes.ts @@ -4,11 +4,13 @@ */ import { schema } from '@osd/config-schema'; -import { IRouter, OpenSearchClient, RequestHandlerContext } from '../../../../src/core/server'; +import { IRouter, OpenSearchClient } from '../../../../src/core/server'; import { SUMMARY_ASSISTANT_API } from '../../common/constants/llm'; import { getOpenSearchClientTransport } from '../utils/get_opensearch_client_transport'; import { getAgentIdByConfigName, searchAgent } from './get_agent'; import { AssistantServiceSetup } from '../services/assistant_service'; +import { handleError } from './error_handler'; +import { AgentNotFoundError } from './errors'; const SUMMARY_AGENT_CONFIG_ID = 'os_summary'; const LOG_PATTERN_SUMMARY_AGENT_CONFIG_ID = 'os_summary_with_log_pattern'; @@ -47,32 +49,16 @@ export function registerSummaryAssistantRoutes( req.body.index && req.body.dsl && req.body.topNLogPatternData ? LOG_PATTERN_SUMMARY_AGENT_CONFIG_ID : SUMMARY_AGENT_CONFIG_ID; - let response; try { - response = await assistantClient.executeAgentByConfigName(agentConfigId, { + const response = await assistantClient.executeAgentByConfigName(agentConfigId, { context: req.body.context, question: req.body.question, index: req.body.index, input: req.body.dsl, topNLogPatternData: req.body.topNLogPatternData, }); - } catch (e) { - context.assistant_plugin.logger.error('Execute agent failed!', e); - if (e.statusCode >= 400 && e.statusCode <= 499) { - return res.customError({ - body: { message: typeof e.body === 'string' ? e.body : JSON.stringify(e.body) }, - statusCode: e.statusCode, - }); - } else { - return res.customError({ - body: 'Execute agent failed!', - statusCode: 500, - }); - } - } - let insightAgentIdExists = false; - try { + let insightAgentIdExists = false; if (req.body.insightType) { insightAgentIdExists = !!(await detectInsightAgentId( req.body.insightType, @@ -80,21 +66,17 @@ export function registerSummaryAssistantRoutes( client )); } - } catch (e) { - context.assistant_plugin.logger.error( - `Cannot find insight agent for ${req.body.insightType}`, - e - ); - } - const summary = response.body.inference_results[0]?.output[0]?.result; - if (summary) { + const summary = response.body.inference_results[0]?.output[0]?.result; + if (!summary) { + return res.customError({ + body: 'Execute agent failed with empty response!', + statusCode: 500, + }); + } return res.ok({ body: { summary, insightAgentIdExists } }); - } else { - return res.customError({ - body: 'Execute agent failed with empty response!', - statusCode: 500, - }); + } catch (e) { + return handleError(e, res, context.assistant_plugin.logger); } }) ); @@ -121,13 +103,17 @@ export function registerSummaryAssistantRoutes( dataSourceId: req.query.dataSourceId, }); const assistantClient = assistantService.getScopedClient(req, context); - const insightAgentId = await detectInsightAgentId( - req.body.insightType, - req.body.summaryType, - client - ); try { + const insightAgentId = await detectInsightAgentId( + req.body.insightType, + req.body.summaryType, + client + ); + if (!insightAgentId) { + return res.notFound({ body: 'Agent not found' }); + } + const response = await assistantClient.executeAgent(insightAgentId, { context: req.body.context, summary: req.body.summary, @@ -135,27 +121,15 @@ export function registerSummaryAssistantRoutes( }); const insight = response.body.inference_results[0]?.output[0]?.result; - if (insight) { - return res.ok({ body: { insight } }); - } else { + if (!insight) { return res.customError({ body: 'Execute agent failed with empty response!', statusCode: 500, }); } + return res.ok({ body: { insight } }); } catch (e) { - context.assistant_plugin.logger.error('Execute agent failed!', e); - if (e.statusCode >= 400 && e.statusCode <= 499) { - return res.customError({ - body: { message: typeof e.body === 'string' ? e.body : JSON.stringify(e.body) }, - statusCode: e.statusCode, - }); - } else { - return res.customError({ - body: 'Execute agent failed!', - statusCode: 500, - }); - } + return handleError(e, res, context.assistant_plugin.logger); } }) ); @@ -168,12 +142,20 @@ function detectInsightAgentId( ) { // We have separate agent for os_insight and user_insight. And for user_insight, we can // only get it by searching on name since it is not stored in agent config. - if (insightType === 'os_insight') { - return getAgentIdByConfigName(OS_INSIGHT_AGENT_CONFIG_ID, client); - } else if (insightType === 'user_insight' && summaryType === 'alerts') { - return searchAgent({ name: 'KB_For_Alert_Insight' }, client); + try { + if (insightType === 'os_insight') { + return getAgentIdByConfigName(OS_INSIGHT_AGENT_CONFIG_ID, client); + } else if (insightType === 'user_insight' && summaryType === 'alerts') { + return searchAgent({ name: 'KB_For_Alert_Insight' }, client); + } + } catch (e) { + // It only detects if the agent exists, we don't want to throw the error when not found the agent + // So we return `undefined` to indicate the insight agent id not found + if (e instanceof AgentNotFoundError) { + return undefined; + } + throw e; } - return undefined; } export function registerData2SummaryRoutes( @@ -220,18 +202,7 @@ export function registerData2SummaryRoutes( }); } } catch (e) { - context.assistant_plugin.logger.error('Execute agent failed!', e); - if (e.statusCode >= 400 && e.statusCode <= 499) { - return res.customError({ - body: { message: typeof e.body === 'string' ? e.body : JSON.stringify(e.body) }, - statusCode: e.statusCode, - }); - } else { - return res.customError({ - body: 'Execute agent failed!', - statusCode: 500, - }); - } + return handleError(e, res, context.assistant_plugin.logger); } }) ); diff --git a/server/routes/text2viz_routes.test.ts b/server/routes/text2viz_routes.test.ts index 946dd578..8363074e 100644 --- a/server/routes/text2viz_routes.test.ts +++ b/server/routes/text2viz_routes.test.ts @@ -146,7 +146,7 @@ describe('test text2viz route', () => { "headers": Object {}, "payload": Object { "error": "Internal Server Error", - "message": "Execute agent failed!", + "message": "An internal server error occurred.", "statusCode": 500, }, "statusCode": 500, @@ -228,7 +228,7 @@ describe('test text2viz route', () => { "headers": Object {}, "payload": Object { "error": "Internal Server Error", - "message": "Execute agent failed!", + "message": "An internal server error occurred.", "statusCode": 500, }, "statusCode": 500, diff --git a/server/routes/text2viz_routes.ts b/server/routes/text2viz_routes.ts index fb32e9b8..cab97c87 100644 --- a/server/routes/text2viz_routes.ts +++ b/server/routes/text2viz_routes.ts @@ -13,6 +13,7 @@ import { TEXT2VIZ_API, } from '../../common/constants/llm'; import { AssistantServiceSetup } from '../services/assistant_service'; +import { handleError } from './error_handler'; const inputSchema = schema.string({ maxLength: TEXT2VEGA_INPUT_SIZE_LIMIT, @@ -86,18 +87,7 @@ export function registerText2VizRoutes(router: IRouter, assistantService: Assist } return res.badRequest(); } catch (e) { - context.assistant_plugin.logger.error('Execute agent failed!', e); - if (e.statusCode >= 400 && e.statusCode <= 499) { - return res.customError({ - body: { message: typeof e.body === 'string' ? e.body : JSON.stringify(e.body) }, - statusCode: e.statusCode, - }); - } else { - return res.customError({ - body: 'Execute agent failed!', - statusCode: 500, - }); - } + return handleError(e, res, context.assistant_plugin.logger); } }) ); @@ -126,18 +116,7 @@ export function registerText2VizRoutes(router: IRouter, assistantService: Assist const result = JSON.parse(response.body.inference_results[0].output[0].result); return res.ok({ body: result }); } catch (e) { - context.assistant_plugin.logger.error('Execute agent failed!', e); - if (e.statusCode >= 400 && e.statusCode <= 499) { - return res.customError({ - body: { message: typeof e.body === 'string' ? e.body : JSON.stringify(e.body) }, - statusCode: e.statusCode, - }); - } else { - return res.customError({ - body: 'Execute agent failed!', - statusCode: 500, - }); - } + return handleError(e, res, context.assistant_plugin.logger); } }) ); diff --git a/server/services/chat/olly_chat_service.test.ts b/server/services/chat/olly_chat_service.test.ts index c42fca7f..7b1c327e 100644 --- a/server/services/chat/olly_chat_service.test.ts +++ b/server/services/chat/olly_chat_service.test.ts @@ -233,7 +233,7 @@ describe('OllyChatService', () => { interactionId: 'interactionId', }) ).rejects.toMatchInlineSnapshot( - `[Error: get agent os_chat failed, reason: Error: cannot get agent os_chat by calling the api: /_plugins/_ml/config/os_chat]` + `[Error: cannot get agent by config name os_chat by calling the api: /_plugins/_ml/config/os_chat]` ); }); });