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

Update logger #4047

Conversation

JonathanWoollett-Light
Copy link
Contributor

@JonathanWoollett-Light JonathanWoollett-Light commented Aug 20, 2023

Changes

  • Adds support for reconfiguring the logger at runtime. You can now call the logger API root multiple times without error.
  • Greatly simplifies the logger implementation removing ~800 LoC.
  • Reduces the size of the Firecracker stripped release binary by ~20kb (~0.7%) (2681136 -> 2664816).

Reason

See above.

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.

@codecov
Copy link

codecov bot commented Aug 20, 2023

Codecov Report

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

Comparison is base (b4871b1) 83.10% compared to head (f35250e) 82.97%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4047      +/-   ##
==========================================
- Coverage   83.10%   82.97%   -0.13%     
==========================================
  Files         225      223       -2     
  Lines       28605    28452     -153     
==========================================
- Hits        23772    23609     -163     
- Misses       4833     4843      +10     
Flag Coverage Δ
4.14-c7g.metal 78.51% <33.87%> (-0.17%) ⬇️
4.14-m5d.metal 80.31% <34.27%> (-0.16%) ⬇️
4.14-m6a.metal 79.45% <33.87%> (-0.17%) ⬇️
4.14-m6g.metal 78.51% <33.87%> (-0.17%) ⬇️
4.14-m6i.metal 80.30% <33.87%> (-0.16%) ⬇️
5.10-c7g.metal 81.43% <33.87%> (-0.15%) ⬇️
5.10-m5d.metal 82.99% <34.27%> (-0.14%) ⬇️
5.10-m6a.metal 82.22% <33.87%> (-0.15%) ⬇️
5.10-m6g.metal 81.43% <33.87%> (-0.15%) ⬇️
5.10-m6i.metal 82.97% <33.87%> (-0.14%) ⬇️
6.1-c7g.metal 81.43% <33.87%> (-0.15%) ⬇️
6.1-m5d.metal 82.99% <34.27%> (-0.14%) ⬇️
6.1-m6a.metal 82.22% <33.87%> (-0.15%) ⬇️
6.1-m6g.metal 81.43% <33.87%> (-0.15%) ⬇️
6.1-m6i.metal 82.97% <33.87%> (-0.14%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
src/api_server/src/request/logger.rs 100.00% <100.00%> (ø)
src/jailer/src/env.rs 67.26% <100.00%> (-0.08%) ⬇️
src/vmm/src/arch/aarch64/cache_info.rs 96.41% <ø> (ø)
src/vmm/src/builder.rs 93.19% <ø> (ø)
...c/vmm/src/cpu_config/x86_64/cpuid/amd/normalize.rs 97.23% <ø> (ø)
...c/vmm/src/cpu_config/x86_64/custom_cpu_template.rs 100.00% <100.00%> (ø)
src/vmm/src/device_manager/mmio.rs 64.31% <ø> (ø)
src/vmm/src/devices/virtio/balloon/device.rs 88.34% <100.00%> (ø)
...rc/vmm/src/devices/virtio/balloon/event_handler.rs 72.72% <100.00%> (ø)
src/vmm/src/devices/virtio/balloon/util.rs 100.00% <100.00%> (ø)
... and 23 more

... and 4 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@JonathanWoollett-Light JonathanWoollett-Light force-pushed the update_logger branch 28 times, most recently from d033858 to 507cb99 Compare August 23, 2023 12:15
@JonathanWoollett-Light JonathanWoollett-Light marked this pull request as ready for review August 23, 2023 12:42
Jonathan Woollett-Light added 5 commits October 6, 2023 10:48
Runs updated `cargo fmt`.

Signed-off-by: Jonathan Woollett-Light <[email protected]>
Re-exports logging macros from `logger` to allow more atomic changes
in `logger`.

Signed-off-by: Jonathan Woollett-Light <[email protected]>
- Refactors the logger implementation, simplifying the implementation
  and reducing the binary size by ~20kb.
- Adds support for runtime reconfiguration.
- Updates the check for the `Running Firecracker` log to match.
- Updates the default log level to `Info`.

Signed-off-by: Jonathan Woollett-Light <[email protected]>
Adds a logger initialization function that allows removing the `log`
dependencies.

Signed-off-by: Jonathan Woollett-Light <[email protected]>
Moves argument passing before logging `Running Firecracker` and other
code that may not be required if certain arguments are passed causing
the process to exit early.

Signed-off-by: Jonathan Woollett-Light <[email protected]>
src/vmm/src/logger/logging.rs Show resolved Hide resolved
@JonathanWoollett-Light JonathanWoollett-Light merged commit fa4b299 into firecracker-microvm:main Oct 6, 2023
4 of 5 checks passed
JonathanWoollett-Light pushed a commit to JonathanWoollett-Light/firecracker that referenced this pull request Nov 15, 2023
firecracker-microvm#4047 updated
parsing the logger level filter. It removed all case variants outside
fully uppercase (e.g. `INFO`) and the 1st character being upper case
(e.g. `Info`) this removed support for other previously supported
variants e.g. `info`. This commit re-introduces this support such that
all variants should again be supported.

Signed-off-by: Jonathan Woollett-Light <[email protected]>
JonathanWoollett-Light pushed a commit to JonathanWoollett-Light/firecracker that referenced this pull request Nov 15, 2023
firecracker-microvm#4047 updated
parsing the logger level filter. It removed all case variants outside
fully uppercase (e.g. `INFO`) and the 1st character being upper case
(e.g. `Info`) this removed support for other previously supported
variants e.g. `info`. This commit re-introduces this support such that
all variants should again be supported.

Signed-off-by: Jonathan Woollett-Light <[email protected]>
wearyzen pushed a commit to JonathanWoollett-Light/firecracker that referenced this pull request Nov 23, 2023
firecracker-microvm#4047 updated
parsing the logger level filter. It removed all case variants outside
fully uppercase (e.g. `INFO`) and the 1st character being upper case
(e.g. `Info`) this removed support for other previously supported
variants e.g. `info`. This commit re-introduces this support such that
all variants should again be supported.

Signed-off-by: Jonathan Woollett-Light <[email protected]>
Signed-off-by: Sudan Landge <[email protected]>
wearyzen pushed a commit to JonathanWoollett-Light/firecracker that referenced this pull request Nov 23, 2023
firecracker-microvm#4047 updated
parsing the logger level filter. It removed all case variants outside
fully uppercase (e.g. `INFO`) and the 1st character being upper case
(e.g. `Info`) this removed support for other previously supported
variants e.g. `info`. This commit re-introduces this support such that
all variants should again be supported.

Signed-off-by: Jonathan Woollett-Light <[email protected]>
Signed-off-by: Sudan Landge <[email protected]>
wearyzen pushed a commit to JonathanWoollett-Light/firecracker that referenced this pull request Nov 23, 2023
firecracker-microvm#4047 updated
parsing the logger level filter. It removed all case variants outside
fully uppercase (e.g. `INFO`) and the 1st character being upper case
(e.g. `Info`) this removed support for other previously supported
variants e.g. `info`. This commit re-introduces this support such that
all variants should again be supported.

Signed-off-by: Jonathan Woollett-Light <[email protected]>
Signed-off-by: Sudan Landge <[email protected]>
wearyzen pushed a commit to JonathanWoollett-Light/firecracker that referenced this pull request Nov 24, 2023
firecracker-microvm#4047 updated
parsing the logger level filter. It removed all case variants outside
fully uppercase (e.g. `INFO`) and the 1st character being upper case
(e.g. `Info`) this removed support for other previously supported
variants e.g. `info`. This commit re-introduces this support such that
all variants should again be supported.

Signed-off-by: Jonathan Woollett-Light <[email protected]>
Signed-off-by: Sudan Landge <[email protected]>
wearyzen pushed a commit to JonathanWoollett-Light/firecracker that referenced this pull request Nov 24, 2023
firecracker-microvm#4047 updated
parsing the logger level filter. It removed all case variants outside
fully uppercase (e.g. `INFO`) and the 1st character being upper case
(e.g. `Info`) this removed support for other previously supported
variants e.g. `info`. This commit re-introduces this support such that
all variants should again be supported.

Signed-off-by: Jonathan Woollett-Light <[email protected]>
Signed-off-by: Sudan Landge <[email protected]>
wearyzen pushed a commit to JonathanWoollett-Light/firecracker that referenced this pull request Nov 24, 2023
firecracker-microvm#4047 updated
parsing the logger level filter. It removed all case variants outside
fully uppercase (e.g. `INFO`) and the 1st character being upper case
(e.g. `Info`) this removed support for other previously supported
variants e.g. `info`. This commit re-introduces this support such that
all variants should again be supported.

Signed-off-by: Jonathan Woollett-Light <[email protected]>
Signed-off-by: Sudan Landge <[email protected]>
wearyzen pushed a commit to JonathanWoollett-Light/firecracker that referenced this pull request Nov 24, 2023
firecracker-microvm#4047 updated
parsing the logger level filter. It removed all case variants outside
fully uppercase (e.g. `INFO`) and the 1st character being upper case
(e.g. `Info`) this removed support for other previously supported
variants e.g. `info`. This commit re-introduces this support such that
all variants should again be supported.
Fixes firecracker-microvm@332f218

Signed-off-by: Jonathan Woollett-Light <[email protected]>
Signed-off-by: Sudan Landge <[email protected]>
wearyzen pushed a commit to JonathanWoollett-Light/firecracker that referenced this pull request Nov 24, 2023
firecracker-microvm#4047 updated
parsing the logger level filter. It removed all case variants outside
fully uppercase (e.g. `INFO`) and the 1st character being upper case
(e.g. `Info`) this removed support for other previously supported
variants e.g. `info`. This commit re-introduces this support such that
all variants should again be supported.
Fixes #332f2184ae29265fbae1648469e073fc7f987378

Signed-off-by: Jonathan Woollett-Light <[email protected]>
Signed-off-by: Sudan Landge <[email protected]>
wearyzen pushed a commit to JonathanWoollett-Light/firecracker that referenced this pull request Nov 24, 2023
firecracker-microvm#4047 updated
parsing the logger level filter. It removed all case variants outside
fully uppercase (e.g. `INFO`) and the 1st character being upper case
(e.g. `Info`) this removed support for other previously supported
variants e.g. `info`. This commit re-introduces this support such that
all variants should again be supported.
Fixes commit 332f218

Signed-off-by: Jonathan Woollett-Light <[email protected]>
Signed-off-by: Sudan Landge <[email protected]>
wearyzen pushed a commit to JonathanWoollett-Light/firecracker that referenced this pull request Nov 24, 2023
firecracker-microvm#4047 updated
parsing the logger level filter. It removed all case variants outside
fully uppercase (e.g. `INFO`) and the 1st character being upper case
(e.g. `Info`) this removed support for other previously supported
variants e.g. `info`. This commit re-introduces this support such that
all variants should again be supported.
Fixes commit 332f218

Signed-off-by: Jonathan Woollett-Light <[email protected]>
Signed-off-by: Sudan Landge <[email protected]>
wearyzen pushed a commit to JonathanWoollett-Light/firecracker that referenced this pull request Nov 24, 2023
firecracker-microvm#4047 updated
parsing the logger level filter. It removed all case variants outside
fully uppercase (e.g. `INFO`) and the 1st character being upper case
(e.g. `Info`) this removed support for other previously supported
variants e.g. `info`. This commit re-introduces this support such that
all variants should again be supported.
Fixes commit 332f218

Signed-off-by: Jonathan Woollett-Light <[email protected]>
Signed-off-by: Sudan Landge <[email protected]>
wearyzen pushed a commit to JonathanWoollett-Light/firecracker that referenced this pull request Nov 24, 2023
firecracker-microvm#4047 updated
parsing the logger level filter. It removed all case variants outside
fully uppercase (e.g. `INFO`) and the 1st character being upper case
(e.g. `Info`) this removed support for other previously supported
variants e.g. `info`. This commit re-introduces this support such that
all variants should again be supported.
Fixes commit 332f218

Signed-off-by: Jonathan Woollett-Light <[email protected]>
Signed-off-by: Sudan Landge <[email protected]>
wearyzen pushed a commit that referenced this pull request Nov 24, 2023
#4047 updated
parsing the logger level filter. It removed all case variants outside
fully uppercase (e.g. `INFO`) and the 1st character being upper case
(e.g. `Info`) this removed support for other previously supported
variants e.g. `info`. This commit re-introduces this support such that
all variants should again be supported.
Fixes commit 332f218

Signed-off-by: Jonathan Woollett-Light <[email protected]>
Signed-off-by: Sudan Landge <[email protected]>
wearyzen pushed a commit that referenced this pull request Nov 27, 2023
#4047 updated
parsing the logger level filter. It removed all case variants outside
fully uppercase (e.g. `INFO`) and the 1st character being upper case
(e.g. `Info`) this removed support for other previously supported
variants e.g. `info`. This commit re-introduces this support such that
all variants should again be supported.
Fixes commit 332f218

Signed-off-by: Jonathan Woollett-Light <[email protected]>
Signed-off-by: Sudan Landge <[email protected]>
@pb8o pb8o mentioned this pull request Nov 28, 2023
9 tasks
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.

4 participants