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

Network flows metrics #586

Merged
merged 24 commits into from
Feb 16, 2024
Merged

Network flows metrics #586

merged 24 commits into from
Feb 16, 2024

Conversation

mariomac
Copy link
Contributor

@mariomac mariomac commented Jan 30, 2024

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).

@mariomac mariomac added do-not-merge WIP or something that musn't be merged wip labels Jan 30, 2024
@codecov-commenter
Copy link

codecov-commenter commented Jan 30, 2024

Codecov Report

Attention: 1164 lines in your changes are missing coverage. Please review.

Comparison is base (6cb90b3) 79.19% compared to head (802927a) 69.99%.

Files Patch % Lines
pkg/internal/netolly/transform/k8s/informers.go 0.00% 250 Missing ⚠️
pkg/internal/netolly/ebpf/tracer.go 0.00% 224 Missing ⚠️
pkg/internal/netolly/agent/agent.go 0.00% 168 Missing ⚠️
pkg/internal/netolly/flow/tracer_map.go 11.90% 73 Missing and 1 partial ⚠️
pkg/internal/netolly/export/metrics.go 32.98% 65 Missing ⚠️
pkg/internal/netolly/flow/tracer_ringbuf.go 0.00% 65 Missing ⚠️
pkg/internal/netolly/transform/k8s/kubernetes.go 0.00% 52 Missing ⚠️
pkg/internal/netolly/agent/pipeline.go 0.00% 45 Missing ⚠️
pkg/internal/netolly/ebpf/net_bpfel_x86.go 0.00% 34 Missing ⚠️
pkg/internal/netolly/agent/ip.go 66.32% 25 Missing and 8 partials ⚠️
... and 15 more
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     
Flag Coverage Δ
integration-test 57.76% <1.34%> (-11.13%) ⬇️
unittests 40.84% <28.56%> (-3.43%) ⬇️

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.

@mariomac mariomac changed the title WIP: asserts integration Network flows metrics Feb 15, 2024
@mariomac mariomac removed the do-not-merge WIP or something that musn't be merged label Feb 15, 2024
@mariomac mariomac removed the wip label Feb 15, 2024
@mariomac mariomac requested a review from marctc February 15, 2024 11:49
Copy link
Contributor

@grcevski grcevski left a 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 = {
Copy link
Contributor

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?

Copy link
Contributor

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.

Copy link
Contributor Author

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) {
Copy link
Contributor

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:/netobserv/netobserv-ebpf-agent/tree/release-1.4

package k8s
Copy link
Contributor

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.

Copy link
Contributor Author

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.

@mariomac mariomac merged commit 9c103c2 into grafana:main Feb 16, 2024
4 checks passed
@mariomac mariomac deleted the asserts branch February 16, 2024 08:40
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