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

Proxy error reworking #6453

Merged
merged 9 commits into from
Feb 9, 2024
Merged

Proxy error reworking #6453

merged 9 commits into from
Feb 9, 2024

Conversation

conradludgate
Copy link
Contributor

@conradludgate conradludgate commented Jan 23, 2024

Problem

Taking my ideas from #6283 and doing a bit less radical changes. smaller commits.

We currently don't report error classifications in proxy as the current error handling made it hard to do so.

Summary of changes

  1. Add a ReportableError trait that all errors will implement. This provides the error classification functionality.
  2. Handle Client requests a strongly typed error
    • this error is a ReportableError and is logged appropriately
  3. The handle client error only has a few possible error types, to account for the fact that at this point errors should be returned to the user.

Checklist before requesting a review

  • I have performed a self-review of my code.
  • If it is a core feature, I have added thorough tests.
  • Do we need to implement analytics? if so did you add the relevant metrics to the dashboard?
  • If this PR requires public announcement, mark it with /release-notes label and add several sentences in this section.

Checklist before merging

  • Do not forget to reformat commit message to not include the above checklist

Copy link

github-actions bot commented Jan 23, 2024

2418 tests run: 2305 passed, 0 failed, 113 skipped (full report)


Flaky tests (1)

Postgres 14

Code coverage (full report)

  • functions: 54.8% (11816 of 21546 functions)
  • lines: 82.1% (66050 of 80467 lines)

The comment gets automatically updated with the latest test results
3bbc545 at 2024-02-09T15:38:41.835Z :recycle:

@conradludgate conradludgate force-pushed the flatten-proxy-flow branch 3 times, most recently from 3c2fdd1 to ce3f99f Compare January 29, 2024 11:35
Base automatically changed from flatten-proxy-flow to main January 29, 2024 17:38
@conradludgate conradludgate force-pushed the proxy-error-reworking branch 2 times, most recently from f22bf05 to 3db668a Compare January 30, 2024 11:59
@conradludgate conradludgate changed the base branch from main to proxy-more-refactors January 30, 2024 11:59
Base automatically changed from proxy-more-refactors to main February 2, 2024 16:07
@conradludgate conradludgate force-pushed the proxy-error-reworking branch 3 times, most recently from 7f5a17b to e676524 Compare February 8, 2024 13:10
remove some of the extra checks for sni hostnames that dont seem too necessary
@conradludgate conradludgate marked this pull request as ready for review February 8, 2024 16:51
@conradludgate conradludgate requested a review from a team as a code owner February 8, 2024 16:51
@conradludgate conradludgate requested review from khanova and removed request for a team February 8, 2024 16:51
Copy link
Contributor

@khanova khanova left a comment

Choose a reason for hiding this comment

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

Looks good! Left a few minor questions.

@conradludgate conradludgate merged commit 96d89cd into main Feb 9, 2024
49 checks passed
@conradludgate conradludgate deleted the proxy-error-reworking branch February 9, 2024 15:50
@hlinnaka hlinnaka mentioned this pull request Apr 22, 2024
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