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

Vhost user blk metrics #4226

Merged
merged 7 commits into from
Nov 14, 2023

Conversation

sudanl0
Copy link
Contributor

@sudanl0 sudanl0 commented Nov 8, 2023

Changes

  • Add below metrics for vhost user device:
    1. to report activate failures.
    2. to report failure to read config space.
    3. to report time taken to init the device in micro seconds.
    4. to report time taken to activate the device in micro seconds.
  • Add test to validate breaking change in vhost device metrics.

Reason

Vhost user block is a new device which doesn't have any metrics.

License Acceptance

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license. For more information on following
Developer Certificate of Origin and signing off your commits, please check
CONTRIBUTING.md.

PR Checklist

  • [ ] If a specific issue led to this PR, this PR closes the issue.
  • The description of changes is clear and encompassing.
  • Any required documentation changes (code and docs) are included in this PR.
  • [ ] API changes follow the Runbook for Firecracker API changes.
  • User-facing changes are mentioned in CHANGELOG.md.
  • All added/changed functionality is tested.
  • [ ] New TODOs link to an issue.
  • Commits meet contribution quality standards.

  • This functionality cannot be added in rust-vmm.

Copy link

codecov bot commented Nov 8, 2023

Codecov Report

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

Comparison is base (0950b82) 81.74% compared to head (10af235) 81.76%.

Files Patch % Lines
.../vmm/src/devices/virtio/vhost_user_block/device.rs 81.25% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4226      +/-   ##
==========================================
+ Coverage   81.74%   81.76%   +0.01%     
==========================================
  Files         237      238       +1     
  Lines       29179    29223      +44     
==========================================
+ Hits        23853    23894      +41     
- Misses       5326     5329       +3     
Flag Coverage Δ
4.14-c7g.metal 77.21% <92.85%> (+0.02%) ⬆️
4.14-m5d.metal 79.07% <92.85%> (+0.01%) ⬆️
4.14-m6a.metal 78.20% <92.85%> (+0.02%) ⬆️
4.14-m6g.metal 77.21% <92.85%> (+0.02%) ⬆️
4.14-m6i.metal 79.07% <92.85%> (+0.02%) ⬆️
5.10-c7g.metal 80.10% <92.85%> (+0.02%) ⬆️
5.10-m5d.metal 81.74% <92.85%> (+0.01%) ⬆️
5.10-m6a.metal 80.95% <92.85%> (+0.02%) ⬆️
5.10-m6g.metal 80.10% <92.85%> (+0.02%) ⬆️
5.10-m6i.metal 81.72% <92.85%> (+0.01%) ⬆️
6.1-c7g.metal 80.10% <92.85%> (+0.02%) ⬆️
6.1-m5d.metal 81.74% <92.85%> (+0.01%) ⬆️
6.1-m6a.metal 80.95% <92.85%> (+0.02%) ⬆️
6.1-m6g.metal 80.10% <92.85%> (+0.02%) ⬆️
6.1-m6i.metal 81.72% <92.85%> (+0.02%) ⬆️

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.

@sudanl0 sudanl0 force-pushed the vhost_user_blk_metrics branch 8 times, most recently from ec7f735 to aebf91d Compare November 10, 2023 21:40
@sudanl0 sudanl0 marked this pull request as ready for review November 10, 2023 22:24
@sudanl0 sudanl0 self-assigned this Nov 12, 2023
@sudanl0 sudanl0 added the Status: Awaiting review Indicates that a pull request is ready to be reviewed label Nov 12, 2023
src/vmm/src/devices/virtio/vhost_user_block/device.rs Outdated Show resolved Hide resolved
tests/host_tools/metrics.py Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
src/vmm/src/logger/metrics.rs Outdated Show resolved Hide resolved
@sudanl0 sudanl0 force-pushed the vhost_user_blk_metrics branch 10 times, most recently from 5ecf5de to df5c176 Compare November 13, 2023 22:03
src/vmm/src/devices/virtio/vhost_user_metrics.rs Outdated Show resolved Hide resolved
src/vmm/src/devices/virtio/vhost_user_metrics.rs Outdated Show resolved Hide resolved
src/vmm/src/devices/virtio/vhost_user_metrics.rs Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@sudanl0 sudanl0 force-pushed the vhost_user_blk_metrics branch 3 times, most recently from 2c5e163 to 9da8967 Compare November 14, 2023 11:55
kalyazin
kalyazin previously approved these changes Nov 14, 2023
Copy link
Contributor

@bchalios bchalios left a comment

Choose a reason for hiding this comment

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

Some minor comments inline and also:

First commit: feat(metrics): Add vhost user metrics

Suggestion for re-wording the commit message:

feat(metrics): add vhost-user metrics

Define metrics for vhost-user devices. These are metrics that are
independent of the device type. They refer to events during
initialization, configuration and activation of a vhost-user device.

src/vmm/src/devices/virtio/vhost_user_block/device.rs Outdated Show resolved Hide resolved
src/vmm/src/devices/virtio/vhost_user_block/device.rs Outdated Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
Define metrics for vhost-user devices. These are metrics that are
independent of the device type. They refer to events during
initialization, configuration and activation of a vhost-user device.

Signed-off-by: Sudan Landge <sudanl@amazon.com>
Use 3 metrics for vhost user block device:
1. to report activate failures.
2. to report failure to read config space.
3. to report time taken to init the device in micro seconds.
4. to report time taken to activate the device in micro seconds.

Signed-off-by: Sudan Landge <sudanl@amazon.com>
Report addition of vhost user device metrics with an example.

Signed-off-by: Sudan Landge <sudanl@amazon.com>
Flush vhost user device metrics as part of METRICS flush
using a proxy serializer.

Signed-off-by: Sudan Landge <sudanl@amazon.com>
Make FirecrackerMetrics schema local to validate function so that
vhost-user schema added by different tests don't conflict with
each other.
Move timestamp validation up so that the check doesn't generate any
false results.

Signed-off-by: Sudan Landge <sudanl@amazon.com>
Add schema to validate breaking change in vhost-user metrics.
Re-organize metrics validation function to accommodate vhost-user
which has per device metrics but does not have aggregate metrics.
Use existing vhost-user tests to check that there is no breaking
change in vhost-user-block device metrics.

Signed-off-by: Sudan Landge <sudanl@amazon.com>
Add basic test to check shared inc and store metrics.

Signed-off-by: Sudan Landge <sudanl@amazon.com>
@sudanl0 sudanl0 merged commit 7febde3 into firecracker-microvm:main Nov 14, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Awaiting review Indicates that a pull request is ready to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants