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

Add 'get_helper_error_msg' utility #2905

Merged
merged 1 commit into from
Jan 15, 2024
Merged

Add 'get_helper_error_msg' utility #2905

merged 1 commit into from
Jan 15, 2024

Conversation

jordalgo
Copy link
Contributor

@jordalgo jordalgo commented Jan 7, 2024 •
edited
Loading

This is for additional helpful messaging for specific errors e.g. for when the number of bpf map entries is exceeded.

Follow up to: #2809

Sample output:

$ sudo BPFTRACE_MAP_KEYS_MAX=1 bpftrace -e 'BEGIN { @[1] = 1; @[2] = 2; exit() }' -k
Attaching 1 probe...
stdin:1:24-25: WARNING: Map full; can't update element. Try increasing MAP_KEYS_MAX.
Additional Info - helper: map_update_elem, retcode: -7
BEGIN { @[1] = 1; @[2] = 2; exit() }
                       ~
@[1]: 1

$ sudo BPFTRACE_MAP_KEYS_MAX=1 bpftrace -e 'BEGIN { @[1] = 1; @[2] = 2; exit() }' -k -f json
{"type": "attached_probes", "data": {"probes": 1}}
{"type": "helper_error", "msg": "Map full; can't update element. Try increasing MAP_KEYS_MAX.", "helper": "map_update_elem", "retcode": -7, "line": 1, "col": 24}

{"type": "map", "data": {"@": {"1": 1}}}
Checklist
  • Language changes are updated in man/adoc/bpftrace.adoc and if needed in docs/reference_guide.md
  • User-visible and non-trivial changes updated in CHANGELOG.md
  • The new behaviour is covered by tests

viktormalik
viktormalik approved these changes Jan 8, 2024
View reviewed changes
Copy link
Contributor

@viktormalik viktormalik 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! Do we know about other helpers that could use better error messages?

@jordalgo
Copy link
Contributor Author

jordalgo commented Jan 8, 2024

Do we know about other helpers that could use better error messages?

Good question. I'll take a look and see if anything jumps out.

danobi
danobi approved these changes Jan 10, 2024
View reviewed changes
Copy link
Member

@danobi danobi left a comment

Choose a reason for hiding this comment

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

nice!

@danobi
Copy link
Member

danobi commented Jan 10, 2024

Changelog entry?

ajor
ajor reviewed Jan 10, 2024
View reviewed changes
Copy link
Member

@ajor ajor left a comment

Choose a reason for hiding this comment

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

Good addition! While we're adding this extra detail to this specific error message, I'm thinking that we might also want to remove unnecessary info from it too. i.e. "Failed to map_update_elem: Argument list too long"

map_update_elem is an implementation detail and "Argument list too long" doesn't mean much.

Maybe we should say something more user friendly along the lines of: "Map full, can't update element (try ...)"

src/output.cpp Outdated
std::string msg = strerror(-retcode);
if (func_id == libbpf::BPF_FUNC_map_update_elem && retcode == -E2BIG)
{
msg.append(" (trying increasing MAP_KEYS_MAX)");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
msg.append(" (trying increasing MAP_KEYS_MAX)");
msg.append(" (try increasing MAP_KEYS_MAX)");

Typo

@jordalgo
Copy link
Contributor Author

jordalgo commented Jan 10, 2024 •
edited
Loading

map_update_elem is an implementation detail and "Argument list too long" doesn't mean much.

Yeah good call. Part of me wants to leave in 'map_update_elem' for devs debugging the issue even though it's an implementation detail. The json output separates this bpf function into a field called "helper". Maybe for non-json output we can just follow the error message with a similar format e.g.

Map full; can't update element. Try increasing MAP_KEYS_MAX. Additional Info - helper: map_update_elem, retcode: -7

Add 'get_helper_error_msg' utility
526e3b9 
This is for additional helpful messaging for specific errors e.g.
for when the number of bpf map entries is exceeded.

Follow up to: bpftrace#2809
@jordalgo
Copy link
Contributor Author

Do we know about other helpers that could use better error messages?

Added two more for map lookup and deletion.

danobi
danobi reviewed Jan 12, 2024
View reviewed changes
src/output.cpp
std::string msg;
if (func_id == libbpf::BPF_FUNC_map_update_elem && retcode == -E2BIG)
{
msg = "Map full; can't update element. Try increasing MAP_KEYS_MAX.";
Copy link
Member

Choose a reason for hiding this comment

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

This is the "old" name for w/ the config changes. Sounds like there's consensus on changing the env var names - should we preemptively use BPFTRACE_MAX_MAP_KEYS here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Depends on the merging order :) This is a smaller change so I imagine this will get merged first. Happy to change it with the config PR.

Copy link
Member

Choose a reason for hiding this comment

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

I think this is good to merge. Then the other PR can rebase on top

@viktormalik
Copy link
Contributor

@ajor can we merge this?

ajor
ajor approved these changes Jan 15, 2024
View reviewed changes
Copy link
Member

@ajor ajor 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 to me!

@ajor ajor merged commit 7d39d70 into bpftrace:master Jan 15, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danobi danobi danobi approved these changes

@dxuuu dxuuu dxuuu approved these changes

@viktormalik viktormalik viktormalik approved these changes

@ajor ajor ajor approved these changes

@fbs fbs Awaiting requested review from fbs fbs is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jordalgo @danobi @viktormalik @ajor @dxuuu