Skip to content
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

Cody: Update to log errors from embeddings #52780

Merged
merged 6 commits into from
Jun 1, 2023
Merged

Conversation

abeatrix
Copy link
Contributor

@abeatrix abeatrix commented Jun 1, 2023

Close #52696
RE https://sourcegraph.slack.com/archives/C04MSD3DP5L/p1685629520228399?thread_ts=1685626165.446539&cid=C04MSD3DP5L

Currently the error messages from the embedding services are being displayed in the UI, which can be disruptive to users using keyword contexts. We will log the errors in debug output instead.

Updated: on top of the connection status we are currently displaying as INDEXED and NON-INDEXED under chat box, I've updated the function to display search errors for users who are currently using 'embeddings', is connected to a indexed codebase, but getting search errors from the embeddings service.

Test plan

We can see the embeddings service error in our current test build since we don't have the embedding service mocked:
image

Added a test step to confirm the error message is not showing up in UI anymore

@cla-bot cla-bot bot added the cla-signed label Jun 1, 2023
@abeatrix abeatrix requested a review from a team June 1, 2023 14:47
@@ -375,13 +374,7 @@ export class ChatViewProvider implements vscode.WebviewViewProvider, vscode.Disp
this.sendTranscript()
void this.saveTranscriptToChatHistory()
void vscode.commands.executeCommand('setContext', 'cody.reply.pending', false)
if (!this.codebaseContext.checkEmbeddingsConnection()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need this check because this is the same function we used to display the NOT INDEXED and INDEXED status in the Chat UI

@sourcegraph-bot
Copy link
Contributor

sourcegraph-bot commented Jun 1, 2023

📖 Storybook live preview

@camdencheek
Copy link
Member

Do we show any indication to a user that something went wrong? Even just a little yellow exclamation point or something would be great. I'm concerned this will cause more users to get worse context with no way of knowing, and then they'll never reach out for support because they don't know something is wrong (they just think Cody is bad instead)

@abeatrix
Copy link
Contributor Author

abeatrix commented Jun 1, 2023

Do we show any indication to a user that something went wrong? Even just a little yellow exclamation point or something would be great. I'm concerned this will cause more users to get worse context with no way of knowing, and then they'll never reach out for support because they don't know something is wrong (they just think Cody is bad instead)

@camdencheek That's a good point. So on top of the connection status we are currently displaying as INDEXED and NON-INDEXED under chat box, I've updated the function to display search errors for users who are currently using 'embeddings', is connected to a indexed codebase, but getting search errors from the embeddings service. Do you think this would be good enough?

@camdencheek
Copy link
Member

Sounds good to me! As long as there is some indication of an error, I'm happy

@abeatrix
Copy link
Contributor Author

abeatrix commented Jun 1, 2023

@camdencheek can I have your review, please 🙇‍♀️

@abeatrix abeatrix enabled auto-merge (squash) June 1, 2023 16:02
@abeatrix abeatrix merged commit e382f54 into main Jun 1, 2023
14 of 15 checks passed
@abeatrix abeatrix deleted the bee/cody-log-errors branch June 1, 2023 16:18
@abeatrix abeatrix mentioned this pull request Jun 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cody Test: Remove errors from embedding server in test
3 participants