-
Notifications
You must be signed in to change notification settings - Fork 24
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
Integrate feedback(Thumb up/down) with memory API #22
Changes from all commits
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 |
---|---|---|
|
@@ -105,6 +105,18 @@ const updateSessionRoute = { | |
}, | ||
}; | ||
|
||
const feedbackRoute = { | ||
path: `${ASSISTANT_API.FEEDBACK}/{interactionId}`, | ||
validate: { | ||
params: schema.object({ | ||
interactionId: schema.string(), | ||
}), | ||
body: schema.object({ | ||
satisfaction: schema.boolean(), | ||
}), | ||
}, | ||
}; | ||
|
||
export function registerChatRoutes(router: IRouter, routeOptions: RoutesOptions) { | ||
const createStorageService = (context: RequestHandlerContext) => | ||
new AgentFrameworkStorageService( | ||
|
@@ -293,4 +305,26 @@ export function registerChatRoutes(router: IRouter, routeOptions: RoutesOptions) | |
} | ||
} | ||
); | ||
|
||
router.put( | ||
feedbackRoute, | ||
async ( | ||
context, | ||
request, | ||
response | ||
): Promise<IOpenSearchDashboardsResponse<HttpResponsePayload | ResponseError>> => { | ||
const storageService = createStorageService(context); | ||
const { interactionId } = request.params; | ||
|
||
try { | ||
const getResponse = await storageService.updateInteraction(interactionId, { | ||
feedback: request.body, | ||
}); | ||
return response.ok({ body: getResponse }); | ||
} catch (error) { | ||
context.assistant_plugin.logger.error(error); | ||
return response.custom({ statusCode: error.statusCode || 500, body: error.message }); | ||
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. Do we want to return back an error code? For interaction I'd imagine it could be a fire and forget thing. 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. i think users should be notified if their feedback didn't go through? |
||
} | ||
} | ||
); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -181,4 +181,32 @@ export class AgentFrameworkStorageService implements StorageService { | |
throw new Error('update converstaion failed, reason:' + JSON.stringify(error.meta?.body)); | ||
} | ||
} | ||
|
||
async updateInteraction( | ||
interactionId: string, | ||
additionalInfo: Record<string, Record<string, boolean | string>> | ||
): Promise<SessionOptResponse> { | ||
try { | ||
const response = await this.client.transport.request({ | ||
method: 'PUT', | ||
path: `/_plugins/_ml/memory/interaction/${interactionId}/_update`, | ||
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. nit: can path be part of the constants file too? like |
||
body: { | ||
additional_info: additionalInfo, | ||
}, | ||
}); | ||
if (response.statusCode === 200) { | ||
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. i might be wrong but i thought javascript client throws errors on non success. if it's not |
||
return { | ||
success: true, | ||
}; | ||
} else { | ||
return { | ||
success: false, | ||
statusCode: response.statusCode, | ||
message: JSON.stringify(response.body), | ||
}; | ||
} | ||
} catch (error) { | ||
throw new Error('update interaction failed, reason:' + JSON.stringify(error.meta?.body)); | ||
} | ||
} | ||
} |
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.
Though traceId == interactionId, I think it's better we use interactionId explicitly, rather than rely on traceId. @SuZhou-Joe Do we already get interaction id of each message at FE?
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.
For each
markdown
message, yes.