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 missing gRPC/HTTP2 events #867

Merged
merged 14 commits into from
May 27, 2024
Merged

Conversation

grcevski
Copy link
Contributor

This PR adds the ability to parse gRPC/HTTP2 events even if we missed the original connection event. Namely Beyla can start after the applications sends the PRI * HTTP/2.0 event and we will fail to register the http2 connection metadata. In this event Beyla will capture an unclassified TCP event.

We add a HTTP2/gRPC detector to the unclassified events and use a channel to add the missing metadata in the eBPF maps. Subsequent streams will see the metadata and correctly report the gRPC events.

@grcevski grcevski requested review from mariomac and marctc as code owners May 23, 2024 16:02
@codecov-commenter
Copy link

codecov-commenter commented May 23, 2024

Codecov Report

Attention: Patch coverage is 47.75161% with 244 lines in your changes are missing coverage. Please review.

Project coverage is 78.16%. Comparing base (b61e5c7) to head (189e687).
Report is 2 commits behind head on main.

Files Patch % Lines
pkg/internal/ebpf/bhpack/hpack.go 47.72% 95 Missing and 20 partials ⚠️
pkg/internal/ebpf/bhpack/huffman.go 40.83% 64 Missing and 7 partials ⚠️
pkg/internal/ebpf/common/http2grpc_transform.go 58.73% 15 Missing and 11 partials ⚠️
pkg/internal/ebpf/bhpack/tables.go 56.09% 16 Missing and 2 partials ⚠️
pkg/internal/ebpf/httpfltr/httpfltr.go 18.18% 8 Missing and 1 partial ⚠️
pkg/internal/ebpf/common/tcp_detect_transform.go 58.33% 3 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #867      +/-   ##
==========================================
- Coverage   79.86%   78.16%   -1.70%     
==========================================
  Files         117      120       +3     
  Lines        8054     8486     +432     
==========================================
+ Hits         6432     6633     +201     
- Misses       1217     1410     +193     
- Partials      405      443      +38     
Flag Coverage Δ
integration-test 56.00% <47.75%> (-0.37%) ⬇️
k8s-integration-test 61.91% <0.42%> (-3.24%) ⬇️
oats-test 34.15% <0.42%> (-1.74%) ⬇️
unittests 43.94% <0.00%> (-2.31%) ⬇️

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.

@@ -0,0 +1,525 @@
// Copyright 2014 The Go Authors. All rights reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why copying the code instead of just doing:

import "golang.org/x/net/http2/hpack"

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to vendor it because it was failing on unknown fields. We want to ignore the unknown so that we can continue parsing until we find :method and :path.

@grcevski grcevski merged commit 48b713e into grafana:main May 27, 2024
6 checks passed
@grcevski grcevski deleted the fix_missing_grpc_events branch May 27, 2024 14:33
@grcevski
Copy link
Contributor Author

The test for these missing metrics is involved, so I'm going to split this work in another PR.

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