Add integration test for Firecracker with clippy-tracing instrumentation #5256

demoncoder-crypto · 2025-06-09T15:20:12Z

Addresses Test instrumented binary #4215

kalyazin

@demoncoder-crypto thanks for your contribution!

I added some inline comments. Apart from that, could you please make changes to the commit message:

add the test: scope tag at the beginning of its title
add a (small) paragraph with a more detailed description of the commit
add the Signed-off-by line (can be generated via git commit -s)

I also triggered a CI build to see if tests pass.

kalyazin · 2025-06-09T15:57:23Z

tests/integration_tests/functional/test_firecracker_tracing.py

+def test_firecracker_tracing_with_vm_lifecycle(instrumented_firecracker_binary):
+    """Test that instrumented Firecracker works through a complete VM lifecycle with tracing."""
+    # Skip this test if we don't have the necessary resources
+    if not global_props.host_linux_version_tup >= (4, 14):


this is now 5.10: https://github.com/firecracker-microvm/firecracker/blob/main/docs/kernel-policy.md#host-kernel

kalyazin · 2025-06-09T15:58:38Z

tests/integration_tests/functional/test_firecracker_tracing.py

+        assert found_meaningful, f"Expected to find traces from meaningful functions, but traces were: {trace_matches[:10]}"
+
+
+def test_firecracker_tracing_with_vm_lifecycle(instrumented_firecracker_binary):


this test looks a bit like a subset of the test_firecracker_tracing_basic_functionality. What do you think it adds to that one?

kalyazin · 2025-06-09T15:59:30Z

tests/integration_tests/functional/test_firecracker_tracing.py

+
+        # Should complete within reasonable time (30 seconds is very generous)
+        # This is just to catch major performance regressions or hangs
+        assert elapsed < 30, f"Instrumented Firecracker took too long to start and handle API calls: {elapsed}s"


this looks like a good idea

kalyazin · 2025-06-09T16:00:49Z

tests/integration_tests/functional/test_firecracker_tracing.py

+
+            # The new log data should contain trace information
+            new_log_data = final_log_data[len(initial_log_data):]
+            assert "TRACE" in new_log_data, "Expected TRACE logs after setting log level to Trace"


shall we also verify that no TRACE is present in the initial_log_data?

kalyazin · 2025-06-09T16:01:18Z

tests/integration_tests/functional/test_firecracker_tracing.py

@@ -0,0 +1,305 @@
+# Copyright 2024 Amazon.com, Inc. or its affiliates. All Rights Reserved.


it is 2025 now

kalyazin · 2025-06-09T16:02:15Z

tests/integration_tests/functional/test_firecracker_tracing.py

+
+"""Test that Firecracker works correctly when instrumented with tracing and trace level logs are enabled.
+
+This test addresses GitHub issue about adding a test that checks if simple integration tests work 


I don't think we should refer to a Github issue here. It should be possible to trace it back via commit -> PR -> GH issue.

kalyazin · 2025-06-09T16:03:00Z

tests/integration_tests/functional/test_firecracker_tracing.py

+This test addresses GitHub issue about adding a test that checks if simple integration tests work 
+when Firecracker is instrumented with src/clippy-tracing and trace level logs are enabled.
+
+The existing test_log_instrument.py only tests the log-instrument crate with example programs.


We can probably omit this paragraph as well as comments should describe the present state (when the PR is already merged).

demoncoder-crypto · 2025-06-10T07:35:42Z

@kalyazin I have pushed the changes as requested

codecov · 2025-06-10T10:40:03Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.90%. Comparing base (e36e774) to head (ac6cfa0).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5256      +/-   ##
==========================================
+ Coverage   82.84%   82.90%   +0.05%     
==========================================
  Files         250      250              
  Lines       26967    26967              
==========================================
+ Hits        22342    22356      +14     
+ Misses       4625     4611      -14

Flag	Coverage Δ
5.10-c5n.metal	`83.33% <ø> (+<0.01%)`	⬆️
5.10-m5n.metal	`83.33% <ø> (+<0.01%)`	⬆️
5.10-m6a.metal	`82.54% <ø> (ø)`
5.10-m6g.metal	`79.16% <ø> (ø)`
5.10-m6i.metal	`83.33% <ø> (+<0.01%)`	⬆️
5.10-m7a.metal-48xl	`82.53% <ø> (?)`
5.10-m7g.metal	`79.16% <ø> (ø)`
5.10-m7i.metal-24xl	`83.29% <ø> (?)`
5.10-m7i.metal-48xl	`83.29% <ø> (?)`
5.10-m8g.metal-24xl	`79.16% <ø> (?)`
5.10-m8g.metal-48xl	`79.16% <ø> (?)`
6.1-c5n.metal	`83.38% <ø> (ø)`
6.1-m5n.metal	`83.38% <ø> (+<0.01%)`	⬆️
6.1-m6a.metal	`82.59% <ø> (ø)`
6.1-m6g.metal	`79.16% <ø> (ø)`
6.1-m6i.metal	`83.37% <ø> (ø)`
6.1-m7a.metal-48xl	`82.58% <ø> (?)`
6.1-m7g.metal	`79.16% <ø> (ø)`
6.1-m7i.metal-24xl	`83.38% <ø> (?)`
6.1-m7i.metal-48xl	`83.39% <ø> (?)`
6.1-m8g.metal-24xl	`79.16% <ø> (?)`
6.1-m8g.metal-48xl	`79.16% <ø> (?)`

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.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Verifies Firecracker works with clippy-tracing instrumentation and trace-level logging enabled. Signed-off-by: demoncoder-crypto <dhillonprabhjitsingh@gmail.com>

kalyazin · 2025-06-10T13:22:59Z

I ran the CI checks, it looks like there is a minor style issue and and a problem with calling clippy-tracing:

E   error: the argument '--path <PATH>' cannot be used multiple times

https://buildkite.com/firecracker/firecracker-pr/builds/13695#_

You should be able to run the tests locally via the devtool: https://github.com/firecracker-microvm/firecracker/blob/main/tests/README.md .

demoncoder-crypto mentioned this pull request Jun 9, 2025

Test instrumented binary #4215

Open

kalyazin reviewed Jun 9, 2025

View reviewed changes

demoncoder-crypto force-pushed the add-firecracker-tracing-integration-test branch from 0595ad0 to b8bf90e Compare June 10, 2025 07:34

demoncoder-crypto requested a review from kalyazin June 10, 2025 08:14

test: Add Firecracker tracing integration test

ac6cfa0

Verifies Firecracker works with clippy-tracing instrumentation and trace-level logging enabled. Signed-off-by: demoncoder-crypto <dhillonprabhjitsingh@gmail.com>

demoncoder-crypto force-pushed the add-firecracker-tracing-integration-test branch from b8bf90e to ac6cfa0 Compare June 10, 2025 12:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add integration test for Firecracker with clippy-tracing instrumentation #5256

Add integration test for Firecracker with clippy-tracing instrumentation #5256

demoncoder-crypto commented Jun 9, 2025

Uh oh!

kalyazin left a comment

Uh oh!

kalyazin Jun 9, 2025

Uh oh!

demoncoder-crypto Jun 10, 2025

Uh oh!

kalyazin Jun 9, 2025 •

edited

Loading

Uh oh!

kalyazin Jun 9, 2025

Uh oh!

kalyazin Jun 9, 2025

Uh oh!

demoncoder-crypto Jun 10, 2025

Uh oh!

kalyazin Jun 9, 2025

Uh oh!

demoncoder-crypto Jun 10, 2025

Uh oh!

kalyazin Jun 9, 2025

Uh oh!

kalyazin Jun 9, 2025

Uh oh!

demoncoder-crypto commented Jun 10, 2025

Uh oh!

codecov bot commented Jun 10, 2025 •

edited

Loading

Uh oh!

kalyazin commented Jun 10, 2025

Uh oh!

Uh oh!

		assert found_meaningful, f"Expected to find traces from meaningful functions, but traces were: {trace_matches[:10]}"


		def test_firecracker_tracing_with_vm_lifecycle(instrumented_firecracker_binary):

		@@ -0,0 +1,305 @@
		# Copyright 2024 Amazon.com, Inc. or its affiliates. All Rights Reserved.


		"""Test that Firecracker works correctly when instrumented with tracing and trace level logs are enabled.

		This test addresses GitHub issue about adding a test that checks if simple integration tests work

Add integration test for Firecracker with clippy-tracing instrumentation #5256

Are you sure you want to change the base?

Add integration test for Firecracker with clippy-tracing instrumentation #5256

Conversation

demoncoder-crypto commented Jun 9, 2025

Uh oh!

kalyazin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kalyazin Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

demoncoder-crypto commented Jun 10, 2025

Uh oh!

codecov bot commented Jun 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

kalyazin commented Jun 10, 2025

Uh oh!

Uh oh!

kalyazin Jun 9, 2025 •

edited

Loading

codecov bot commented Jun 10, 2025 •

edited

Loading