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: propagate cartservice exceptions #1744

Merged

Conversation

rogercoll
Copy link
Contributor

@rogercoll rogercoll commented Oct 18, 2024

Changes

Propagate Grpc expections; the EmptyCart method already sets the error in the span, but these two other methods aren't. A common propagated error is when the service cannot establish a connection with the Valkey database.

Merge Requirements

For new features contributions, please make sure you have completed the following
essential items:

  • CHANGELOG.md updated to document new feature additions
    * [] Appropriate documentation updates in the docs
    * [ ] Appropriate Helm chart updates in the helm-charts

Maintainers will not merge until the above have been completed. If you're unsure
which docs need to be changed ping the
@open-telemetry/demo-approvers.

@rogercoll rogercoll requested a review from a team as a code owner October 18, 2024 10:18
@julianocosta89
Copy link
Member

Not sure if this is a local issue, but it seems cartServiceFailure is not working at all.
I've tested the PR and I didn't get any error on the cartService, but then I've built main and it also didn't work.

I assume we have an error on the FF itself.

@julianocosta89
Copy link
Member

Opened #1747

@julianocosta89
Copy link
Member

@rogercoll, I've done some tests here and I don't see what this PR is actually changing 🤔
On the left I have the build from this PR, on the right I have the build from main.

image

@rogercoll
Copy link
Contributor Author

@julianocosta89 This PR should be propagating the "EmptyCart" error you are sharing in the screenshot but for the GetCart and AddItem transactions/methods. EmptyCart somehow already had it.

I have tested this PR in a scenario where the "valkey" database becomes unavailable (Crashloopback in k8s) after some minutes the Demo has started. But the PR is not tied to this specific scenario, in fact, any error that happens downstream those methods will be recorded in the transaction.

Copy link
Member

@julianocosta89 julianocosta89 left a comment

Choose a reason for hiding this comment

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

Thx for clarifying @rogercoll!
LGTM
image

@julianocosta89 julianocosta89 merged commit c79152c into open-telemetry:main Oct 22, 2024
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants