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

Fix fetching Go server status code #1044

Merged
merged 26 commits into from
Jul 22, 2024

Conversation

grcevski
Copy link
Contributor

Our status code fetching for Go HTTP/HTTP2 was relying on a single interface implementation for the response writer. This works in most cases, but it doesn't work with custom response writers, which can be implemented by users and as I recently discovered, the GRPC server has another mode, which lets users implement a single handler for HTTP and gRPC.

This PR does few things:

  1. Instead of digging into the response object, we rely on the write status function to capture the status. This is better because we control which implementations we know of. HTTP/HTTP2 standard cases work now and if it's unknown implementation that doesn't delegate to the underlying HTTP/HTTP2, the worst that will happen is status 0, instead of getting high cardinality garbage value.
  2. I fixed the negative content size. In Go we read the content size from the headers, and perhaps surprisingly, this value can be -1. Fixes Crash in prom exporter with negative increment #1027. For example, my new test for GRPC in plaintext mode captures this HTTP2 wrapper (with size -1):
2024-07-21 19:48:40.72174840 (2.805882ms[2.597025ms]) SRV 200 PRI * [172.18.0.5 as 172.18.0.5:46108]->[172.18.0.4 as 172.18.0.4:8080] size:-1B svc=[grpc-http2-go/server go] traceparent=[00-b60d354b576686820357bd40ec4bddc2-0000000000000000-01]

I confirmed in the headers the value is indeed -1.

  1. I added a test for this new GRPC/HTTP2 Mux approach, for both plaintext and TLS.
  2. I cleaned up some stale unused Go offsets we had.

@grcevski grcevski requested review from mariomac and marctc as code owners July 21, 2024 20:08
@codecov-commenter
Copy link

codecov-commenter commented Jul 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.59%. Comparing base (9a5f90e) to head (ac22c13).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1044      +/-   ##
==========================================
+ Coverage   73.32%   81.59%   +8.26%     
==========================================
  Files         137      138       +1     
  Lines       11014    11028      +14     
==========================================
+ Hits         8076     8998     +922     
+ Misses       2306     1514     -792     
+ Partials      632      516     -116     
Flag Coverage Δ
integration-test 56.72% <100.00%> (+0.07%) ⬆️
k8s-integration-test 58.93% <86.66%> (-0.27%) ⬇️
oats-test 37.40% <46.66%> (+0.01%) ⬆️
unittests 50.85% <53.33%> (?)

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
Contributor

@mariomac mariomac left a comment

Choose a reason for hiding this comment

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

Current implementation is even more elegant and concise than previous one. Congrats and thanks!

@grcevski grcevski merged commit f9a8d4b into grafana:main Jul 22, 2024
6 checks passed
@grcevski grcevski deleted the fix_grpc_serve_status branch July 22, 2024 16:06
@grcevski
Copy link
Contributor Author

Thanks @mariomac !

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.

Crash in prom exporter with negative increment
3 participants