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

Introduce option for high volume request tracking #1192

Closed
wants to merge 3 commits into from

Conversation

grcevski
Copy link
Contributor

@grcevski grcevski commented Sep 22, 2024

Beyla tracks the full request completion time, this typically means we look to see if the application is responding with more data after the first HTTP response. One example would be a large file download, where the majority of the time is actually serializing the data on the wire. When the client uses keep-alive, we don't necessarily see the connection close event, but we tell by new pushed requests that we should terminate an earlier request.

This approach doesn't work well in when there's high volume of requests, e.g. beyond our current map sizing. The delayed requests will likely be booted out of the map before we have a chance to complete them. The ideal fix is to provide an option to resize the maps to match the volume of requests, but we need to finish the work of the tracer code restructuring for this to happen.

I'm adding an option to force Beyla to complete the request as soon as the response is finished. It will produce less accurate accounting for large file downlaods, but it will avoid no data for high volume of requests. We can also use this option to unblock clients if there are bugs in the delayed request accounting.

This PR is partial, since I accidentally pushed to main. Please review the following commits too:
7be9c33
and
bd3e5f4

@codecov-commenter
Copy link

codecov-commenter commented Sep 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.10%. Comparing base (bd3e5f4) to head (64d185d).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1192      +/-   ##
==========================================
+ Coverage   81.08%   81.10%   +0.01%     
==========================================
  Files         136      136              
  Lines       11501    11501              
==========================================
+ Hits         9326     9328       +2     
+ Misses       1642     1640       -2     
  Partials      533      533              
Flag Coverage Δ
integration-test 57.02% <ø> (+0.04%) ⬆️
k8s-integration-test 58.89% <ø> (+0.06%) ⬆️
oats-test 35.81% <ø> (+0.04%) ⬆️
unittests 53.34% <ø> (+0.01%) ⬆️

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

@rafaelroquetto rafaelroquetto left a comment

Choose a reason for hiding this comment

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

LGTM, nice!

I noticed the .o files are being directly tracked - when rebasing/solving conflicts, make sure they are being tracked with git-lfs instead. One of the many ways of doing that is:

  1. rebase onto main
  2. unstage the .o files from your current commit, for instance, and re-run make generate
  3. readd the .o files
  4. at this stage, both git status and git lfs-status should show the modified files
  5. git commit as usual
  6. to be 100% sure, you can do git cat-file -p :pkg/internal/path_to_file.o - it should output something like:
version https://git-lfs.github.com/spec/v1
oid sha256:71a96753d080b4641984f14c9b5615930a33d035d18febfceed706c779103485
size 28368

if it outputs a binary file, something is wrong.

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.

3 participants