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

Add support for keepalive request time #82

Merged
merged 1 commit into from
Apr 26, 2023

Conversation

grcevski
Copy link
Contributor

@grcevski grcevski commented Apr 25, 2023

When a client is using keepalive for a request connection, the Go http server code will keep the goroutine live since the connection doesn't drop. This slightly defeats our request time vs. service time tracking, because there might be delay on processing the request after new data arrives on the old connection.

To mitigate this, we add a probe on startBackroundRead which is the code that will process the incoming data. This function is called very soon after the connection sees new data, so we'll be able to capture the full request time again, even in keepalive mode.

  • Check what happens with Gorilla/mux
  • Check what happens with Gin

@grcevski grcevski added the enhancement New feature or request label Apr 25, 2023
@grcevski grcevski requested a review from mariomac April 25, 2023 18:05
@codecov-commenter
Copy link

Codecov Report

Merging #82 (2ceff1a) into main (dd00693) will decrease coverage by 0.16%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##             main      #82      +/-   ##
==========================================
- Coverage   46.74%   46.58%   -0.16%     
==========================================
  Files          15       15              
  Lines        1166     1172       +6     
==========================================
+ Hits          545      546       +1     
- Misses        572      577       +5     
  Partials       49       49              
Flag Coverage Δ
unittests 46.58% <50.00%> (-0.16%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/ebpf/nethttp/bpf_debug_bpfel_x86.go 0.00% <0.00%> (ø)
pkg/ebpf/nethttp/nethttp.go 0.00% <0.00%> (ø)
pkg/pipe/config.go 48.48% <ø> (ø)
pkg/pipe/instrumenter.go 80.00% <100.00%> (+0.37%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@grcevski
Copy link
Contributor Author

Confirmed it works equally well with mux and gin. gRPC should be good, because as soon as the call ends it should terminate the connection. gRPC has it's own streaming long connection protocol.

@grcevski grcevski merged commit 7ad75d7 into grafana:main Apr 26, 2023
2 checks passed
@grcevski
Copy link
Contributor Author

Thanks Mario!

@grcevski grcevski deleted the request_time_keepalive branch April 26, 2023 12:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants