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

chore: cleanup http handlers #3767

Merged
merged 9 commits into from
Aug 25, 2023
Merged

Conversation

achettyiitr
Copy link
Member

@achettyiitr achettyiitr commented Aug 21, 2023

Description

  • Removing globals.
  • Pulled out http handler related code into a separate file and added test coverage.
  • Made healthTimeout and readerHeaderTimeout configurable.

Linear Ticket

Security

  • The code changed/added as part of this pull request won't create any security issues with how the software is being used.

"pgNotifier": %q,
"acceptingEvents": "TRUE",
"warehouseMode": %q,
"goroutines": "%d"
Copy link
Member Author

Choose a reason for hiding this comment

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

@fracasula @lvrach
Should we remove goroutines from the health payload? Since we removed it from the rudder server because of security concerns.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, I don't see any point in exposing it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes we can remove them. If we don't have the debug/pprof endpoint then we can create a task to add it.

Copy link
Member Author

Choose a reason for hiding this comment

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

We have pprof endpoint.

@codecov
Copy link

codecov bot commented Aug 21, 2023

Codecov Report

Patch coverage: 77.72% and project coverage change: +0.25% 🎉

Comparison is base (a7d7c74) 68.77% compared to head (11a7617) 69.02%.

❗ Current head 11a7617 differs from pull request most recent head 2403c04. Consider uploading reports for the commit 2403c04 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3767      +/-   ##
==========================================
+ Coverage   68.77%   69.02%   +0.25%     
==========================================
  Files         345      347       +2     
  Lines       51649    51643       -6     
==========================================
+ Hits        35521    35649     +128     
+ Misses      13844    13705     -139     
- Partials     2284     2289       +5     
Files Changed Coverage Δ
warehouse/jobs/utils.go 89.28% <ø> (-1.63%) ⬇️
warehouse/utils/utils.go 85.23% <ø> (ø)
warehouse/internal/api/http.go 91.46% <50.00%> (-4.74%) ⬇️
warehouse/warehouse.go 76.64% <60.86%> (+21.50%) ⬆️
warehouse/jobs/http.go 75.72% <75.72%> (ø)
warehouse/http.go 78.10% <78.10%> (ø)
warehouse/jobs/runner.go 61.46% <100.00%> (+1.27%) ⬆️
warehouse/mode.go 100.00% <100.00%> (ø)
warehouse/multitenant/manager.go 100.00% <100.00%> (ø)

... and 9 files with indirect coverage changes

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

warehouse/internal/api/http.go Show resolved Hide resolved
warehouse/jobs/http.go Outdated Show resolved Hide resolved
warehouse/jobs/http.go Show resolved Hide resolved
warehouse/jobs/http.go Outdated Show resolved Hide resolved
warehouse/jobs/http.go Outdated Show resolved Hide resolved
warehouse/jobs/http_test.go Show resolved Hide resolved
warehouse/jobs/http_test.go Outdated Show resolved Hide resolved
warehouse/jobs/http_test.go Outdated Show resolved Hide resolved
"pgNotifier": %q,
"acceptingEvents": "TRUE",
"warehouseMode": %q,
"goroutines": "%d"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes we can remove them. If we don't have the debug/pprof endpoint then we can create a task to add it.

warehouse/http.go Outdated Show resolved Hide resolved
Copy link
Member

@lvrach lvrach left a comment

Choose a reason for hiding this comment

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

Minor, non-blocking comments

warehouse/http.go Outdated Show resolved Hide resolved
warehouse/http.go Outdated Show resolved Hide resolved
warehouse/http.go Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Overall comment for the file:

non-5xx HTTP errors should be logged as warnings. Severity can be important in debugging, observability, and setting up alerts.

usage of structured logging will be nice as well

warehouse/http.go Outdated Show resolved Hide resolved
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.

None yet

3 participants