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

Improved stanza logger flags and added log rotation #488

Merged
merged 4 commits into from
Nov 16, 2021

Conversation

jmwilliams89
Copy link
Contributor

@jmwilliams89 jmwilliams89 commented Nov 16, 2021

Description of Changes

  • Removes the debug flag and replaces it with a more explicit flag for setting log level
  • Reduced complexity of file logging with the lumberjack library
  • Added file rotation for agent log file
  • Added 3 new flags for controlling log file rotation

Please check that the PR fulfills these requirements

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • Add a changelog entry (for non-trivial bug fixes / features)
  • CI passes

@djaglowski
Copy link
Member

Log Files Logs / Second CPU Avg (%) CPU Avg Δ (%) Memory Avg (MB) Memory Avg Δ (MB)
1 1000 1.5344429 +0.12063432 130.04176 +1.3476562
1 5000 5.379323 +0.20693111 139.5928 +0.27114868
1 10000 10.586423 -0.20689297 146.02707 -3.625
1 50000 53.38037 +3.1033173 179.717 -4.7817993
1 100000 101.60566 +10.519363 232.54553 -2.8273163
10 100 1.9655877 +0.05171144 133.66473 +1.3479309
10 500 6.000123 -0.17241192 145.36368 +1.9459839
10 1000 11.914081 -0.6550398 147.68251 -2.380661
10 5000 58.00047 -0.34495163 182.05212 +2.7731628
10 10000 108.67271 -1.5702896 231.38362 -5.0429688

@codecov
Copy link

codecov bot commented Nov 16, 2021

Codecov Report

Merging #488 (ab5ee8a) into master (18c95b8) will increase coverage by 0.16%.
The diff coverage is 91.94%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #488      +/-   ##
==========================================
+ Coverage   74.32%   74.48%   +0.16%     
==========================================
  Files         128      128              
  Lines        8225     8237      +12     
==========================================
+ Hits         6113     6135      +22     
+ Misses       1611     1609       -2     
+ Partials      501      493       -8     
Impacted Files Coverage Δ
cmd/stanza/graph.go 54.84% <37.50%> (+3.41%) ⬆️
cmd/stanza/logging.go 100.00% <100.00%> (+51.61%) ⬆️
cmd/stanza/root.go 44.04% <100.00%> (+2.75%) ⬆️
operator/builtin/input/stdin/stdin.go 72.73% <0.00%> (-4.55%) ⬇️
operator/builtin/output/forward/forward.go 60.49% <0.00%> (-3.70%) ⬇️
operator/builtin/input/file/file.go 77.16% <0.00%> (-3.55%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 18c95b8...ab5ee8a. Read the comment docs.

@jsirianni
Copy link
Member

Can we retain --debug for 1.x releases, perhaps log a warning that it will not be respected? I think it would be okay if --debug does nothing but allows Stanza to startup and run fine.

@jsirianni jsirianni self-requested a review November 16, 2021 04:17
cpheps
cpheps previously approved these changes Nov 16, 2021
Copy link
Contributor

@cpheps cpheps left a comment

Choose a reason for hiding this comment

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

LGTM. I Agree with @jsirianni though that we should keep the --debug flag for backwards compatibility.

Comment on lines +59 to +65
--plugin_dir The location of the plugins directory (default: ./plugins)
--database The location of the offsets database file. If this is not specified, offsets will not be maintained across agent restarts
--log_level The log level of the agent logger (default: INFO)
--log_file The location of the agent log file. If not specified, stanza will log to `stdout`
--max_log_size The maximum size of the agent log file in MB before rotating (default: 10)
--max_log_backups The maximum number of agent log files to retain when rotating (default: 5)
--max_log_age The maximum number of days to retain a rotated agent log file (default: 7)
Copy link
Member

Choose a reason for hiding this comment

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

I agree w/ @jsirianni's point about maintaining backwards compatibility for v1.x.

For v2, we should consider moving all of these into the config file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had the same thought about the config file, since we're ending up with quite a few flags now. I decided against it because it was such a substantial change, but I agree we could probably fit it in for v2.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we write a ticket for this in pivotal so it's captured?

Copy link
Contributor

Choose a reason for hiding this comment

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

Captured this feature request here

@djaglowski
Copy link
Member

Log Files Logs / Second CPU Avg (%) CPU Avg Δ (%) Memory Avg (MB) Memory Avg Δ (MB)
1 1000 1.5000249 +0.08621633 129.68373 +0.989624
1 5000 5.655214 +0.48282194 139.91136 +0.5897064
1 10000 10.414089 -0.37922668 143.96054 -5.6915283
1 50000 54.552288 +4.275234 183.62285 -0.87594604
1 100000 99.138 +8.051704 235.94464 +0.5717926
10 100 2.1206338 +0.20675755 136.11058 +3.7937775
10 500 6.155143 -0.017392159 139.14804 -4.2696533
10 1000 10.879359 -1.6897612 149.01387 -1.0493011
10 5000 57.5316 -0.8138199 181.78853 +2.5095673
10 10000 106.45858 -3.7844162 232.13295 -4.29364

Copy link
Member

@jsirianni jsirianni left a comment

Choose a reason for hiding this comment

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

👍

@jmwilliams89 jmwilliams89 merged commit e18abec into master Nov 16, 2021
@jmwilliams89 jmwilliams89 deleted the log-rotation branch November 16, 2021 18:06
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.

4 participants