Fix missing events from httpfilter #136
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
grcevski
commented
Jun 7, 2023
| ENTRYPOINT ["/greetings"] | ||
| #CMD [ "strace", "-f", "/greetings" ] |
There was a problem hiding this comment.
Keeping this commented out for now because it's handy for debugging the tests with the kprobes.
grcevski
commented
Jun 7, 2023
| @@ -15,7 +14,7 @@ async fn greeting(item: web::Json<MyObj>) -> HttpResponse { | |||
| } | |||
|
|
|||
| async fn smoke() -> HttpResponse { | |||
| HttpResponse::Ok().content_type(ContentType::plaintext()).body("hello") | |||
| HttpResponse::Ok().into() | |||
There was a problem hiding this comment.
Removed any content reporting, just send empty 200 OK response.
grcevski
commented
Jun 7, 2023
| network_mode: "service:rusttestserver" | ||
| pid: "host" | ||
| network_mode: "service:testserver" | ||
| pid: "service:testserver" |
There was a problem hiding this comment.
I had this to "host" by mistake, we should try harder :).
Codecov Report
@@ Coverage Diff @@
## main #136 +/- ##
===========================================
+ Coverage 40.36% 66.82% +26.45%
===========================================
Files 30 30
Lines 2056 2107 +51
===========================================
+ Hits 830 1408 +578
+ Misses 1169 584 -585
- Partials 57 115 +58
Flags with carried forward coverage won't be shown. Click here to find out more.
|
mariomac
approved these changes
Jun 8, 2023
|
Thanks Mario! |
mattdurham
pushed a commit
to mattdurham/beyla
that referenced
this pull request
Jan 22, 2025
Fix missing events from httpfilter
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While working on the integration tests for Rust I noticed a strange edge case, if the Rust server wasn't sending any data back on an API call, like our '/smoke' API, we weren't catching the smoke events. In my tests I had to add a fake plaintext response to make the tests pass.
After digging further I found out that the actix web framework has an optimisation, if you send multiple identical requests that don't have any response, it reuses the last accepted connection, so technically it doesn't call the accept4 syscall, nor it allocates a new socket, if the connection is not closed.
Since sometimes we might miss the first accept call based on timing, we weren't capturing the /smoke API calls, since we never found a filtered connection info. To mitigate this I added a kprobe on
tcp_rcv_establishedwhich will establish the filtered connection info if there's not one already. Accept and connect are allowed to overwrite this info, so technically it's there only if we never saw an accept happen. I don't think this can happen with a client call, but I'll experiment more and see if I have to worry about client calls and matching what kind of request is established. Right now if it's missing we assume a server request.I also reworked some of the docker compose files for Java and Rust. Naming things as rusttestserver was problematic when we use
make cleanup-integration-testwhich I had to do a lot of :). The cleanup uses the regular docker-compose.yaml, and if the names don't match we don't bring down all of the containers.