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(error): add feature-gated stacktrace to error received from API #1104

Merged
merged 2 commits into from
May 13, 2023

Conversation

NishantJoshi00
Copy link
Member

@NishantJoshi00 NishantJoshi00 commented May 10, 2023

Type of Change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring
  • Dependency updates
  • Documentation
  • CI/CD

Description

This change provides a feature-gated functionality for exporting the stacktrace in the API response, for easier debugging and faster RCAs

Additional Changes

  • This PR modifies the API contract
  • This PR modifies the database schema
  • This PR modifies application configuration/environment variables

This change only affects the outbound error response, adding a optional field stacktrace which is only provided when the specific feature is enabled.

Motivation and Context

How did you test it?

via. postman

Screenshot 2023-05-10 at 1 33 25 PM

Checklist

  • I formatted the code cargo +nightly fmt --all
  • I addressed lints thrown by cargo clippy
  • I reviewed submitted code
  • I added unit tests for my changes where possible
  • I added a CHANGELOG entry if applicable

@NishantJoshi00 NishantJoshi00 added S-waiting-on-review Status: This PR has been implemented and needs to be reviewed M-api-contract-changes Metadata: This PR involves API contract changes A-errors Area: error messages, structure & logging labels May 10, 2023
@NishantJoshi00 NishantJoshi00 added this to the May 2023 Release milestone May 10, 2023
@NishantJoshi00 NishantJoshi00 self-assigned this May 10, 2023
@NishantJoshi00 NishantJoshi00 requested review from a team, jarnura and ashokkjag as code owners May 10, 2023 08:05
Copy link
Member

@SanchithHegde SanchithHegde left a comment

Choose a reason for hiding this comment

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

One more request: Can we manage to un-escape the strings in the list of errors returned? It would be great if it can be done, it would improve readability by a great extent. Let me know if it involves too much effort and/or performance hits, I'll approve this PR.

crates/router/src/core/errors.rs Show resolved Hide resolved
@SanchithHegde SanchithHegde added S-waiting-on-author Status: This PR is incomplete or needs to address review comments and removed S-waiting-on-review Status: This PR has been implemented and needs to be reviewed labels May 10, 2023
@NishantJoshi00
Copy link
Member Author

One more request: Can we manage to un-escape the strings in the list of errors returned? It would be great if it can be done, it would improve readability by a great extent. Let me know if it involves too much effort and/or performance hits, I'll approve this PR.

It is definitely a performance hit, but regardless there is one more concern, the context as shown in the example won't always be a json, in some cases it would just be a simple string. I should have mentioned a example for this scenario. Please refer the following:

Screenshot 2023-05-10 at 5 24 21 PM

@SanchithHegde
Copy link
Member

It is definitely a performance hit, but regardless there is one more concern, the context as shown in the example won't always be a json, in some cases it would just be a simple string.

Even when it's a string, don't you think it would improve readability if it's un-escaped? As for the performance hit, we're expecting to enable this for debug and/or testing builds, so I believe it is acceptable?

@NishantJoshi00 NishantJoshi00 added the S-waiting-on-review Status: This PR has been implemented and needs to be reviewed label May 11, 2023
@NishantJoshi00
Copy link
Member Author

It is definitely a performance hit, but regardless there is one more concern, the context as shown in the example won't always be a json, in some cases it would just be a simple string.

Even when it's a string, don't you think it would improve readability if it's un-escaped? As for the performance hit, we're expecting to enable this for debug and/or testing builds, so I believe it is acceptable?

Hey, this side effect of having escaped characters isn't because of my implementation is handling this behaviour, it's how it is being stored. That's what causing this issue

@NishantJoshi00 NishantJoshi00 removed the S-waiting-on-author Status: This PR is incomplete or needs to address review comments label May 11, 2023
@jarnura jarnura added this pull request to the merge queue May 12, 2023
@jarnura jarnura added S-ready-for-merge and removed S-waiting-on-review Status: This PR has been implemented and needs to be reviewed labels May 12, 2023
@SanchithHegde SanchithHegde removed this pull request from the merge queue due to a manual request May 12, 2023
@SanchithHegde SanchithHegde added this pull request to the merge queue May 12, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks May 12, 2023
@SanchithHegde SanchithHegde added this pull request to the merge queue May 13, 2023
Merged via the queue into main with commit bf2352b May 13, 2023
@SanchithHegde SanchithHegde deleted the stacktrace-in-api branch May 13, 2023 09:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-errors Area: error messages, structure & logging M-api-contract-changes Metadata: This PR involves API contract changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants