-
Notifications
You must be signed in to change notification settings - Fork 8.5k
[OneChat] Conversation Response Errors #229866
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
[OneChat] Conversation Response Errors #229866
Conversation
- Error details panel - Round retry button
|
@elasticmachine merge upstream |
x-pack/platform/plugins/shared/onechat/public/application/hooks/use_send_message_mutation.ts
Outdated
Show resolved
Hide resolved
x-pack/platform/plugins/shared/onechat/public/application/hooks/use_send_message_mutation.ts
Show resolved
Hide resolved
| export const RoundError: React.FC<RoundErrorProps> = ({ error }) => { | ||
| const getStackTrace = (error: unknown) => { | ||
| if (error instanceof Error && typeof error.stack === 'string') { | ||
| return error.stack; |
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.
Aren't stack traces of bundled js basically useless? I couldn't reproduce but the most common chat error I'm getting is hitting context length window (?) with long tool calls
I see sth like this in toast:
Error: event: error
data: {"error":{"code":"bad_request","message":"Received a bad request status code for request from inference entity id [.rainbow-sprinkles-elastic] status [400].
Hope the message will be included in the error details. Can we verify this somehow? (I couldn't reproduce this easily)
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.
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.
Okay yeah you're right. Error.prototype.stack is not the place we should be looking. Let me look a bit closer at the error types to see what we should be using instead here.
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.
I think maybe the errors that are thrown on the server are not flowing through properly to the client. When I catch an error in the frontend observable it no longer has the OnechatError type. I'm looking into it now, but it seems like maybe the HTTP layer is catching and then repackaging the error as a generic Internal Server Error.
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.
hmm I had to deal with that in #229254 and this formatter logic helped and i see you are using this. all onechat errors are instances of ServerSentEventError and the default err.message property is not populated and yeah something felt very off when working with onechat errors on client side. I'm fine to look into this as a followup
| const [message, setMessage] = useState<string>(''); | ||
| const { sendMessage, isResponseLoading, error, pendingMessage, retry } = useSendMessageMutation(); |
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.
it feels slightly off to me that message lives in component and pendingMessage lives in a hook?
I feel like those things are closely tied together, should this be a single hook perhaps?
That would be a nicer interface if other team wants to use onechat's building blocks. I have useChat in mind as an example
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.
Yes, I agree. I plan on re-organizing this state in a follow up PR. I'll probably create a MessagesProvider which will provide the state for the current and pending messages, and a ConversationProvider which will provide the state for the current conversation.
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.
question (maybe nitpick): does it make sense to separate those out into two providers or a single provider that represents the state of the current conversation including the messages?
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.
Yes, you might be right. My thinking was to have separate contexts so that if there was a component that is only concerned with sending messages / messages state, they would only need to subscribe to the Messages context and not the entire Conversation context. Because when any state changes in a context, all of it's subscribers must re-render even if not subscribed to the state that changed. I think we can get around this by providing custom hooks that memoize the state though. I'll do more discovery on how we should structure soon. May also be that we only really need a MessagesProvider and that the conversation stuff doesn't really need it's own context.
jedrazb
left a comment
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! followup to reorganize chat hooks and improve passing error messages/bodies to client
| }) | ||
| ); | ||
| }; | ||
| const removeNewConversationQuery = () => { |
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.
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 oddly enough caused by not having the analytics eventType registered properly. Caused a silent error to be thrown in the useMutation onError callback which caused onSettled to not be called at all. I'm registering the event type properly now.
One caveat to note here: if there is an error occurs in the first message of a conversation, it will throw away the whole conversation. This will be fixed in the follow up state management issue. I don't believe this is a regression, the production code should also behave this way.
| if (isLoading) { | ||
| return ( | ||
| <div css={loadingStyles}> | ||
| <EuiLoadingElastic size="l" aria-label={loadingLabel} /> |
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.
nitpick: can use FormattedMessage here since the label is only used once?
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.
How does that work? I thought FormattedMessage was only for things displayed to the DOM? Doesn't aria-label only accept a string and not a React Node?
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.
aaah i didn't spot its also used in the aria-label and now makes sense why you have it as a variable. my bad. Yes your approach is fine :)
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.
Although now that you mention it, I should i18n-ize the EuiAvatar names, right?
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.
Yes good spot - we should be as used as a title tag.
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
💚 Build Succeeded
Metrics [docs]Module Count
Async chunks
Page load bundle
History
|
## Summary Add conversation round errors - Error details panel - Round retry button <img width="1674" height="1476" alt="image" src="https://github.com/user-attachments/assets/802a619d-aa25-4f7a-bf70-39e6a8cda2ca" /> ### Checklist Check the PR satisfies following conditions. Reviewers should verify this PR satisfies this list as well. - [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [ ] This was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The `release_note:breaking` label should be applied in these situations. - [ ] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [ ] The PR description includes the appropriate Release Notes section, and the correct `release_note:*` label is applied per the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) - [ ] Review the [backport guidelines](https://docs.google.com/document/d/1VyN5k91e5OVumlc0Gb9RPa3h1ewuPE705nRtioPiTvY/edit?usp=sharing) and apply applicable `backport:*` labels. --------- Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
## Summary Add conversation round errors - Error details panel - Round retry button <img width="1674" height="1476" alt="image" src="https://github.com/user-attachments/assets/802a619d-aa25-4f7a-bf70-39e6a8cda2ca" /> ### Checklist Check the PR satisfies following conditions. Reviewers should verify this PR satisfies this list as well. - [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [ ] This was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The `release_note:breaking` label should be applied in these situations. - [ ] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [ ] The PR description includes the appropriate Release Notes section, and the correct `release_note:*` label is applied per the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) - [ ] Review the [backport guidelines](https://docs.google.com/document/d/1VyN5k91e5OVumlc0Gb9RPa3h1ewuPE705nRtioPiTvY/edit?usp=sharing) and apply applicable `backport:*` labels. --------- Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
## Summary Add conversation round errors - Error details panel - Round retry button <img width="1674" height="1476" alt="image" src="https://github.com/user-attachments/assets/802a619d-aa25-4f7a-bf70-39e6a8cda2ca" /> ### Checklist Check the PR satisfies following conditions. Reviewers should verify this PR satisfies this list as well. - [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [ ] This was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The `release_note:breaking` label should be applied in these situations. - [ ] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [ ] The PR description includes the appropriate Release Notes section, and the correct `release_note:*` label is applied per the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) - [ ] Review the [backport guidelines](https://docs.google.com/document/d/1VyN5k91e5OVumlc0Gb9RPa3h1ewuPE705nRtioPiTvY/edit?usp=sharing) and apply applicable `backport:*` labels. --------- Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>


Summary
Add conversation round errors
Checklist
Check the PR satisfies following conditions.
Reviewers should verify this PR satisfies this list as well.
release_note:breakinglabel should be applied in these situations.release_note:*label is applied per the guidelinesbackport:*labels.