feat: Add per net device metrics#4145
Merged
Merged
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #4145 +/- ##
==========================================
+ Coverage 82.99% 83.00% +0.01%
==========================================
Files 221 222 +1
Lines 28567 28634 +67
==========================================
+ Hits 23709 23769 +60
- Misses 4858 4865 +7
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
wearyzen
added
the
Status: Awaiting review
roypat
previously requested changes
Oct 12, 2023
ShadowCurse
reviewed
Oct 12, 2023
roypat
reviewed
Oct 16, 2023
ShadowCurse
reviewed
Oct 16, 2023
Contributor
ShadowCurse
left a comment
ShadowCurse
left a comment
There was a problem hiding this comment.
It seems the implementation jumps all over the place for this task, but I guess it is ok.
added 12 commits
October 17, 2023 15:25
Emit aggregate and per device metrics for network devices: - Move NetDeviceMetrics from logger to Net module. - Use NET_DEV_METRICS_PVT instead of adding an entry of NetDeviceMetrics in Net so that metrics can be flushed even from signal handlers. Note: this is part 1 of 12 commits to add per net device metrics. Signed-off-by: Sudan Landge <sudanl@amazon.com>
Use individual instance of metrics instead of the METRICS.net. Since we do not use METRIC.net, modify report_net_event_fail to use self.metrics. Note: this is part 2 of 12 commits to add per net device metrics. Signed-off-by: Sudan Landge <sudanl@amazon.com>
Update CHANGELOGS to notify that Firecracker now emits per net device metrics. Note: this is part 3 of 12 commits to add per net device metrics. Signed-off-by: Sudan Landge <sudanl@amazon.com>
Create a unit test that allocates 19 NetDeviceMetrics (19 because it is the max number of irqs we have for net devices), update and verify a metric. Note: this is part 4 of 12 commits to add per net device metrics. Signed-off-by: Sudan Landge <sudanl@amazon.com>
Use RwLock to make accessing Metrics threadsafe. Use better name to allocate NetDeviceMetrics Note: this is part 5 of 12 commits to add per net device metrics. Signed-off-by: Sudan Landge <sudanl@amazon.com>
With Vec based approach, "net0" is metrics for the 1st net device
created. However, if devices are not created in the same order all
the time, the first device created could either be eth0 or eth1 and
then net0 could sometimes point to eth0 and sometimes to eth1 which
doesn't help with analysing the metrics.
So, use Map instead of Vec to provide more clarity on which interface
the metrics actually belongs to.
Also, instead of net0 name the metrics json object as net_$iface_id
e.g. net_eth0 or net_eth1 which should help in better analysis.
Use "net_$iface_id" for the metrics name instead of "net_$tap_name" to
be consistent with the net endpoint "/network-interfaces/{iface_id}"
Note: this is part 6 of 12 commits to add per net device metrics.
Signed-off-by: Sudan Landge <sudanl@amazon.com>
Use proper convention of lower case for macros. Make CHANHELOG less verbose. Note: this is part 7 of 12 commits to add per net device metrics. Signed-off-by: Sudan Landge <sudanl@amazon.com>
NetMetricsSerializer naming was confusing because its not a Serializer in serde's sense (it implements Serialize, not Serializer). So give it to a better name. Note: this is part 8 of 12 commits to add per net device metrics. Signed-off-by: Sudan Landge <sudanl@amazon.com>
NetDeviceMetrics was moved from logger which needed the new() because default didn't provide the const implementation that FirecrackerMetrics::new() needed. But since that is not a requirement anymore we rely on default to provide 0 initialized NetDeviceMetrics. NetMetricsSerializeProxy doesn't have any fields and so there is no need to define new() for it. Note: this is part 9 of 12 commits to add per net device metrics. Signed-off-by: Sudan Landge <sudanl@amazon.com>
Better implementation for flush_metrics as suggested in feedback. Note: this is part 10 of 12 commits to add per net device metrics. Signed-off-by: Sudan Landge <sudanl@amazon.com>
Make sure there is no breaking change for NetDeviceMetrics.
Add random number of net devices and validate that values of
"net" are aggregate of all "net_{iface_id}".
Note: this is part 11 of 12 commits to add per net device metrics.
Signed-off-by: Sudan Landge <sudanl@amazon.com>
Use Arc for NetDeviceMetrics since this allows us to store NetDeviceMetrics directly inside Net and get rid of the net_metrics! macro. Also, use better name for net metrics. Note: this is part 12 of 12 commits to add per net device metrics. Signed-off-by: Sudan Landge <sudanl@amazon.com>
ShadowCurse
approved these changes
Oct 18, 2023
roypat
approved these changes
Oct 18, 2023
kalyazin
reviewed
Oct 18, 2023
kalyazin
approved these changes
Oct 18, 2023
Contributor
kalyazin
left a comment
kalyazin
left a comment
There was a problem hiding this comment.
changelog update looks good to me
3 tasks
10 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes
Reason
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.[ ] API changes follow the Runbook for Firecracker API changes.CHANGELOG.md.[ ] NewTODOs link to an issue.rust-vmm.