-
Notifications
You must be signed in to change notification settings - Fork 111
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
Network flows metrics #586
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #586 +/- ##
==========================================
- Coverage 79.19% 69.99% -9.20%
==========================================
Files 70 92 +22
Lines 6027 7633 +1606
==========================================
+ Hits 4773 5343 +570
- Misses 1023 2034 +1011
- Partials 231 256 +25
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! I had a few questions, but none are blocking. Great work Mario!
} | ||
} else { | ||
// Key does not exist in the map, and will need to create a new entry. | ||
flow_metrics new_flow = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we support this mode in the else statement at the moment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using a ring buffer will force us to go with 5.8 as a minimum, we should check if it's acceptable. If we don't use this mode perhaps the safest is to remove it and keep our backwards compatibility as far as we can go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good observation. I will merge this and in another PR I'll consider using a perf buffer or just removing it, assuming that we might miss some packets.
|
||
// StartBeyla in background | ||
func StartBeyla(ctx context.Context, cfg *beyla.Config) { | ||
if cfg.Enabled(beyla.FeatureAppO11y) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like how you separated this!
// This implementation is a derivation of the code in | ||
// https://github.com/netobserv/netobserv-ebpf-agent/tree/release-1.4 | ||
|
||
package k8s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wondering, did you by any chance fix the headless services issue with the k8s decoration we hit during our hackathon? I remember that the jaeger service or some other one from the OTel k8s demo caused us to crash. We fixed it by assigning it an ip.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code is taken directly from the hackathon, so I'd say so but I'll double check.
Provides
network_flow_bytes_total
metrics with information of connections between pods.This feature will be kept internal, undocumented, and unstable (might add breaking changes in the future).