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

feat: add package to handle end-user error messages. #13

Merged
merged 3 commits into from
Dec 19, 2023

Conversation

jvallesm
Copy link
Collaborator

@jvallesm jvallesm commented Dec 15, 2023

Because

  • Connector errors produce an unfriendly output in VDP.
  • In order to trigger a pipeline, several repositories are involved in
    the execution.

This commit

  • Implements a way to add and extract end-user messages to errors.

@jvallesm jvallesm self-assigned this Dec 15, 2023
Copy link

linear bot commented Dec 15, 2023

@jvallesm jvallesm force-pushed the jvalles/ins-2779-improve-error-message-in-connector branch from ca383a1 to d92a26f Compare December 15, 2023 08:22
@jvallesm jvallesm force-pushed the jvalles/ins-2779-improve-error-message-in-connector branch from d92a26f to 9c5fa1d Compare December 15, 2023 08:26
@jvallesm jvallesm force-pushed the jvalles/ins-2779-improve-error-message-in-connector branch 2 times, most recently from bda6d7e to ca196ff Compare December 15, 2023 08:33
Because

- Connector errors produce an unfriendly output in VDP.
- In order to trigger a pipeline, several repositories are involved in
  the execution.

This commit

- Implements a way to add and extract end-user messages to errors.
@jvallesm jvallesm force-pushed the jvalles/ins-2779-improve-error-message-in-connector branch from 03cb8b0 to 237e70e Compare December 15, 2023 08:36
Copy link

codecov bot commented Dec 15, 2023

Codecov Report

Attention: 7 lines in your changes are missing coverage. Please review.

Comparison is base (3236165) 70.81% compared to head (237e70e) 71.62%.

Files Patch % Lines
errmsg/errmsg.go 76.66% 7 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #13      +/-   ##
==========================================
+ Coverage   70.81%   71.62%   +0.81%     
==========================================
  Files           2        3       +1     
  Lines         185      215      +30     
==========================================
+ Hits          131      154      +23     
- Misses         48       55       +7     
  Partials        6        6              
Flag Coverage Δ
unittests 71.62% <76.66%> (+0.81%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@donch1989 donch1989 left a comment

Choose a reason for hiding this comment

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

LGTM

@donch1989
Copy link
Member

Hi @pinglin @xiaofei-du ,
Can you help merge this PR? Thanks

Copy link
Member

@xiaofei-du xiaofei-du left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@xiaofei-du xiaofei-du merged commit 6230a89 into main Dec 19, 2023
5 checks passed
@xiaofei-du xiaofei-du deleted the jvalles/ins-2779-improve-error-message-in-connector branch December 19, 2023 05:22
donch1989 pushed a commit to instill-ai/connector that referenced this pull request Dec 19, 2023
Console errorrs are unclear and expose implementation details to no-code
/ low-code users.

This PR proposes a way to bubble up errors from components in a way that
the end-user error is separated from the internal error information
(stack, low-level details). The `sendReq` method in `pkg/openai` is
modified to return human-friendly errors.
instill-ai/pipeline-backend#311 is needed to
test these changes.

~The [fault](https://github.com/Southclaws/fault) package is chosen to
carry out this task.~ While `fault` was chosen originally, some of the
features we were using were no longer needed after refactoring the code
to adapt to the use of `temporal` workflows to execute sync pipelines.

- A logger is injected in the `openai.Client` struct, so we no longer
need to pass the request variables as part of the error context.
- The client doesn't know the component ID and can't attach it to the
end-user message. However, because communication between temporal
processes prevents error unwrapping, we can only use temporal errors to
pass the end-user message.

Therefore, it was simpler to use [our own
implementation](instill-ai/x#13) of error
messages, at least while the required functionality doesn't get too
sofisticated.

### 🗒️ Notes

- Some coverage was added due to the rapid development in the `main`
branches, which made manual testing a bit unreliable. Let me know if
this is a potential change preventer and if we should focus on coverage
later.
- Additionally, the `frankban/quicktest` package was added in the tests.
I'm a fan of the package but I can rework the tests with `require` in
order to align with the current tests.
- Many integration tests were removed as they were outdated. They aren't
being run now, if there's a reason to keep them I can revert that
change.

![CleanShot 2023-12-15 at 14 45
30](https://github.com/instill-ai/pipeline-backend/assets/3977183/da89cd77-b4b8-401e-aae8-506c55ef0290)


![CleanShot 2023-12-15 at 14 44
32](https://github.com/instill-ai/pipeline-backend/assets/3977183/5512bf25-6bd6-4f64-abdd-d61ee3f35e33)
xiaofei-du pushed a commit that referenced this pull request Feb 9, 2024
🤖 I have created a release *beep* *boop*
---


##
[0.4.0-alpha](v0.3.0-alpha...v0.4.0-alpha)
(2023-12-19)


### Features

* add package to handle end-user error messages.
([#13](#13))
([6230a89](6230a89))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: 👋 Done
Development

Successfully merging this pull request may close these issues.

3 participants