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.

Sign up for GitHub

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

Jump to bottom

bpf,metrics: add line/file information to drop metrics #30972

Merged
merged 3 commits into from Mar 22, 2024
Merged

bpf,metrics: add line/file information to drop metrics #30972

merged 3 commits into from Mar 22, 2024

Conversation

ti-mo
Copy link
Contributor

@ti-mo ti-mo commented Feb 26, 2024 •
edited

Quick summary of the changes here:

  • fix shellcheck warnings in check-sources.sh
  • add __MAGIC_LINE__ macro
  • move around the file->id lookup machinery and surrounding scripts
  • use reserved space in metricsmap.Key to store line/file numbers of all bpf metrics
  • (backport commit) prepare the metrics infrastructure for the resulting cardinality bump by summing metrics with a common prefix
  • various cleanups to remove type conversions and improve legibility
  • no changes to /metrics or cilium-dbg metrics list to avoid breaking users' Prometheus setups

Fixes: #27748

Example output:

~ cilium bpf metrics list
REASON                    DIRECTION   PACKETS   BYTES    LINE   FILE
Interface                 INGRESS     151       9368     1092   bpf_host.c
Interface                 INGRESS     731       139248   659    bpf_overlay.c
Success                   EGRESS      137       8212     1531   bpf_host.c
Success                   EGRESS      9         744      1286   bpf_lxc.c
Success                   INGRESS     11        926      86     l3.h
Success                   INGRESS     1403      299078   231    trace.h
Unsupported L3 protocol   EGRESS      6         460      1466   bpf_lxc.c

~ cilium bpf metrics list -o json
  {
    "reason": "Success",
    "direction": "egress",
    "packets": 406587,
    "bytes": 78023315,
    "line": 40,
    "file": "encap.h"
  },
  {
    "reason": "Unsupported L3 protocol",
    "direction": "egress",
    "packets": 167,
    "bytes": 12050,
    "line": 1466,
    "file": "bpf_lxc.c"
  }

Add line numbers and file names to all metrics in 'cilium-dbg bpf metrics list'

@ti-mo ti-mo added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Feb 26, 2024
@ti-mo ti-mo force-pushed the tb/missed-tail-call-lineinfo branch 4 times, most recently from 094d8e9 to e56f5c0 Compare February 28, 2024 15:05
@ti-mo
Copy link
Contributor Author

ti-mo commented Feb 28, 2024

/test

@ti-mo ti-mo added backport/author The backport will be carried out by the author of the PR. needs-backport/1.15 This PR / issue needs backporting to the v1.15 branch labels Feb 28, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in 1.15.2 Feb 28, 2024
@ti-mo ti-mo marked this pull request as ready for review February 28, 2024 15:23
@ti-mo ti-mo requested review from a team as code owners February 28, 2024 15:23
learnitall
learnitall approved these changes Feb 28, 2024
View reviewed changes
Copy link
Contributor

@learnitall learnitall left a comment

Choose a reason for hiding this comment

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

Looks good for API, thanks!

derailed
derailed approved these changes Feb 28, 2024
View reviewed changes
Copy link
Contributor

@derailed derailed left a comment

Choose a reason for hiding this comment

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

@ti-mo Nice improvement!

@ti-mo ti-mo force-pushed the tb/missed-tail-call-lineinfo branch from e56f5c0 to bfca4c4 Compare February 29, 2024 11:44
@ti-mo ti-mo requested a review from a team as a code owner February 29, 2024 11:44
@ti-mo ti-mo requested a review from bimmlerd February 29, 2024 11:44
@ti-mo
Copy link
Contributor Author

ti-mo commented Feb 29, 2024

/test

@ti-mo ti-mo force-pushed the tb/missed-tail-call-lineinfo branch from bfca4c4 to 1530425 Compare February 29, 2024 11:56
@ti-mo
Copy link
Contributor Author

ti-mo commented Feb 29, 2024

/test

@ti-mo ti-mo force-pushed the tb/missed-tail-call-lineinfo branch from 1530425 to ea34700 Compare February 29, 2024 12:40
@ti-mo ti-mo force-pushed the tb/missed-tail-call-lineinfo branch from d838984 to 8aa8df1 Compare March 5, 2024 09:18
@ti-mo
Copy link
Contributor Author

ti-mo commented Mar 5, 2024

/test

@ti-mo ti-mo force-pushed the tb/missed-tail-call-lineinfo branch from 8aa8df1 to 4c72db3 Compare March 6, 2024 16:14
@ti-mo
Copy link
Contributor Author

ti-mo commented Mar 6, 2024

/test

@ti-mo ti-mo mentioned this pull request Mar 6, 2024
Open
@jrajahalme jrajahalme removed this from Needs backport from main in 1.15.2 Mar 13, 2024
@ti-mo ti-mo force-pushed the tb/missed-tail-call-lineinfo branch from 4c72db3 to 135cf26 Compare March 13, 2024 13:44
@ti-mo
Copy link
Contributor Author

ti-mo commented Mar 13, 2024

/test

@ti-mo ti-mo force-pushed the tb/missed-tail-call-lineinfo branch from 135cf26 to 232b587 Compare March 19, 2024 15:33
@ti-mo
Copy link
Contributor Author

ti-mo commented Mar 19, 2024

/test

@ti-mo
metrics: prepare to increase cardinality of BPF metrics key
78e328b 
This commit is split off from subsequent commits to backport to 1.15.

In a follow-up commit, the reserved space in metricsmap.Key will be used for
storing line and file info. Since older versions of Cilium don't decode these
fields yet, this either causes duplicate metrics to be displayed, or causes
the last metricsmap entry with a given reason/direction combination to
overwrite the counters of other entries, resulting in wrong metrics.

metricsmapCollector.Collect() was believed to handle this correctly, but the
code turned out to be wrong. The updated implementation sums up all values
that resolve to the same label set.

Various cleanups were made to remove type conversions and improve legibility.

Signed-off-by: Timo Beckers <timo@isovalent.com>
@ti-mo
bpf: refactor source file name id lookup, always define __MAGIC_FILE__
dd3349f 
This commit moves the filename->id lookup machinery around a bit to conform
better to the existing structure of the Makefile and checker scripts.

bpf/source_names_to_ids.h was moved to bpf/lib/source_info.h, check-sources.sh
was moved out of the loader package and into the usual scripts directory in
contrib/, and no longer has its own Makefile target.

In addition to __MAGIC_FILE__, this commit adds __MAGIC_LINE__ that resolves
to 0 in BPF tests, for reasons documented in the source code. A follow-up
commit will make use of it.

Also fixes all shellcheck warnings in check-sources.sh.

Signed-off-by: Timo Beckers <timo@isovalent.com>
@ti-mo
bpf,metrics: add line/file information to cilium-dbg bpf metrics list
794f4a6 
Fixes cilium#27748

Example output:
```
~ cilium bpf metrics list
REASON                    DIRECTION   PACKETS   BYTES    LINE   FILE
Interface                 INGRESS     151       9368     1092   bpf_host.c
Interface                 INGRESS     731       139248   659    bpf_overlay.c
Success                   EGRESS      137       8212     1531   bpf_host.c
Success                   EGRESS      9         744      1286   bpf_lxc.c
Success                   INGRESS     11        926      86     l3.h
Success                   INGRESS     1403      299078   231    trace.h
Unsupported L3 protocol   EGRESS      6         460      1466   bpf_lxc.c
```

```
~ cilium bpf metrics list -o json
  {
    "reason": "Success",
    "direction": "egress",
    "packets": 406587,
    "bytes": 78023315,
    "line": 40,
    "file": "encap.h"
  },
  {
    "reason": "Unsupported L3 protocol",
    "direction": "egress",
    "packets": 167,
    "bytes": 12050,
    "line": 1466,
    "file": "bpf_lxc.c"
  }
```

Signed-off-by: Timo Beckers <timo@isovalent.com>
@ti-mo ti-mo force-pushed the tb/missed-tail-call-lineinfo branch from 232b587 to 794f4a6 Compare March 21, 2024 11:05
@ti-mo
Copy link
Contributor Author

ti-mo commented Mar 21, 2024

/test

@ti-mo ti-mo mentioned this pull request Mar 21, 2024
Closed
@ti-mo ti-mo added this pull request to the merge queue Mar 22, 2024
Merged via the queue into cilium:main with commit c0676e6 Mar 22, 2024
61 of 62 checks passed
@ti-mo ti-mo deleted the tb/missed-tail-call-lineinfo branch March 22, 2024 09:58
@ti-mo ti-mo mentioned this pull request Mar 22, 2024
Merged
1 task
@ti-mo ti-mo added backport-pending/1.15 The backport for Cilium 1.15.x for this PR is in progress. needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch and removed needs-backport/1.15 This PR / issue needs backporting to the v1.15 branch needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch labels Mar 22, 2024
@github-actions github-actions bot added backport-done/1.15 The backport for Cilium 1.15.x for this PR is done. and removed backport-pending/1.15 The backport for Cilium 1.15.x for this PR is in progress. labels Mar 27, 2024
@asauber asauber mentioned this pull request Apr 11, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@derailed derailed derailed approved these changes

@dylandreimerink dylandreimerink dylandreimerink approved these changes

@christarazi christarazi christarazi approved these changes

@aanm aanm aanm approved these changes

@markpash markpash markpash approved these changes

@bimmlerd bimmlerd bimmlerd approved these changes

@learnitall learnitall learnitall approved these changes

@tommyp1ckles tommyp1ckles Awaiting requested review from tommyp1ckles tommyp1ckles was automatically assigned from cilium/metrics

Assignees
No one assigned
Labels
backport/author The backport will be carried out by the author of the PR. backport-done/1.15 The backport for Cilium 1.15.x for this PR is done. release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
No open projects
1.15.2
Awaiting triage
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

datapath: Extend METRICS_MAP to store prog ID / ifindex / LOC upon DROP_MISSED_TAIL_CALL
8 participants
@ti-mo @derailed @dylandreimerink @christarazi @aanm @markpash @bimmlerd @learnitall