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

Runtime test support for multi-line json output #3067

Merged
merged 1 commit into from
Mar 21, 2024

Conversation

jordalgo
Copy link
Contributor

bpftrace can output multiple lines of valid json
instead of one large valid json blob. This adds
test runner support to check multiple json lines.

Added a test for multiple map json output as an
example.

Related issue:
#2902

Checklist
  • Language changes are updated in man/adoc/bpftrace.adoc
  • User-visible and non-trivial changes updated in CHANGELOG.md
  • The new behaviour is covered by tests

@jordalgo
Copy link
Contributor Author

Here is what a multi-line failure looks like:

[  FAILED  ] json-output.multiple maps
	Command: /home/parallels/Desktop/shared/repos/bpftrace/build/src/bpftrace -q -f json -e 'BEGIN { @map1["key1"] = 2; @map2["key2"] = 3; exit(); }'
	Expected JSON:

{
  "type": "map",
  "data": {
    "@map1": {
      "key1": 2
    }
  }
}
{
  "type": "map",
  "data": {
    "@map2": {
      "key3": 3
    }
  }
}
	Found:

{
  "type": "map",
  "data": {
    "@map1": {
      "key1": 2
    }
  }
}
{
  "type": "map",
  "data": {
    "@map2": {
      "key2": 3
    }
  }
}

@jordalgo
Copy link
Contributor Author

I'm checking the file extension to determine if we just want a single line check but maybe we should be more explicit with different directive e.g. EXPECT_JSON_LINES or something

Copy link
Member

Choose a reason for hiding this comment

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

how about a .ndjson extension?

Copy link
Member

Choose a reason for hiding this comment

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

we could even tighten up the man page to say bpftrace outputs ndjson

@ajor
Copy link
Member

ajor commented Mar 18, 2024

Wasn't part of the original problem that one particular test case (#3035) was returning multiple JSON blobs when only one was expected?

I think the JSON parser will still fail in the same way for this scenario. Would it make sense to always try parsing bpftrace output as NDJSON, in order to handle unexpected extra data?

@@ -92,6 +92,8 @@ Valid values are::
*json* +
*text*

Note: the json output is ndjson, meaning each new line of the streamed output is a single blob of valid json.
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
Note: the json output is ndjson, meaning each new line of the streamed output is a single blob of valid json.
Note: the json output is ndjson, meaning each line of the streamed output is a single blob of valid json.

@danobi
Copy link
Member

danobi commented Mar 18, 2024

The original test should probably be fixed to only output a single json blob. While treating all expected output as ndjson makes sense, I think there could be value in having nicer formatting in the checked in json files. If it was all ndjson, then it'd get smushed onto a single line

@jordalgo
Copy link
Contributor Author

If it was all ndjson, then it'd get smushed onto a single line

This is exactly why I didn't make it all ndjson - if you look at the histogram json tests, it would be impossible to read if it was all on one line.

@jordalgo
Copy link
Contributor Author

Wasn't part of the original problem that one particular test case (#3035) was returning multiple JSON blobs when only one was expected?

I was thinking of fixing this in a follow-up but I think I have an idea, which I can implement in this PR.

Comment on lines 48 to 50
# PROG i:ms:1 { @ = hist(@n++,3); if (@n > 1023) { delete(@n); exit(); }}
# EXPECT_JSON runtime/outputs/hist_2args.json
# when the race-condition for map deletion is fixed
Copy link
Member

Choose a reason for hiding this comment

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

delete() is a synchronous function, so will always complete before exit() is run. I think the only race condition that can happen in this test is that it doesn't run 1024 times before the TIMEOUT expires (say if it takes 5 seconds for bpftrace to start up for some reason). Increasing the timeout to 10 seconds like you've done might be all that's needed to fix it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

delete() is a synchronous function, so will always complete before exit() is run. I think the only race condition that can happen in this test is that it doesn't run 1024 times before the TIMEOUT expires

This is not what I was experiencing locally. The output was including the map e.g. {"type": "map", "data": {"@n": 3}} - and the value of @n changed often. This was happening with a more frequent interval e.g. i:us:100, which is why I changed it to ms:1. That being said, I was still occasionally seeing the map in the output (after deletion) even if this less frequent interval, which is why I just set it to 1 - this strategy seemed to work best. So perhaps there are multiple issues at play here.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I think another possible problem is a race condition with @n++ if the probes fire too close together. That would explain increasing the interval having helped.

But whatever the cause of us not getting @n = 1024, I'd expect the data in the histogram to also be incorrect. I've not reproduced this failure myself, but #3035 does show an incomplete histogram (192 entries instead of 1024) along with @n = 202

How about this as a fix for the race condition:

BEGIN { $i = 0; while ($i < 1024) { @ = hist($i, 3); $i++; } exit(); }

Copy link
Member

Choose a reason for hiding this comment

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

(I'm getting a bit off track for this PR, sorry)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BEGIN { $i = 0; while ($i < 1024) { @ = hist($i, 3); $i++; } exit(); }

Ah good idea! This worked very well.

bpftrace can output multiple lines of valid json
instead of one large valid json blob. This adds
test runner support to check multiple json lines.

- Added a test for multiple map json output as an
example.
- Fixed a currently flakey test: histogram-finegrain

Related issues:
bpftrace#2902
bpftrace#3035
@jordalgo
Copy link
Contributor Author

@ajor Ok to merge?

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.

Yep!

@jordalgo jordalgo merged commit 3691e71 into bpftrace:master Mar 21, 2024
19 checks passed
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