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

Enhance and standardize logger Config setters #4051

Merged
merged 1 commit into from
Dec 19, 2023

Conversation

timj-hh
Copy link
Contributor

@timj-hh timj-hh commented Dec 4, 2023

Summary

This change enhances the access pattern for the Seelog configuration. Environment variables are currently used in InitSeelog when the agent is started to pass in logging values. After that, all changes to the logger should be managed by the Config struct.

This is a follow up to #4048.

Implementation details

  • Enhances existing setters by adding basic checks and mutex locks
  • Splits the SetLevel setter into driver and instance level functions
  • Calls reloadConfig in each setter
  • Abstracts logLevels mapping from ECS_LOGLEVEL to Seelog level options to a file wide variable
  • Adds a new variable to the Config, logToStdout that will decide whether we should duplicate output to stdout or not. Currently, we are outputting to a rolling log file as well as stdout. The latter should be optional.
  • Changes the seelogConfig method with this logToStdout option.
    • The stdout seelog config setting, and windows platform settings are wrapped in a tag. On Linux, we do not set the platform tag. If we do not output to stdout then the filter tag will be empty. This will cause logging to fail to apply to outputs. So, we should conditionally omit the filter tag if both of these tags are empty.
  • Adds SetRolloverType setter to modify the rollover type of the logs, if any. The default behavior has been kept as size in the agent module, and this also falls back to date based rolling. Passing in 'none' allows for no rollover policy.
  • Removes timestampFormat from the Config object. This var is special in that it is actively used in runtime when a custom log formatter is used. There is a potential data race with the mutex locked Config struct, so we can let this variable's setter directly access it without locks - knowing this will likely be used at startup and not runtime.
  • Adds and refactors tests as described below

Testing

Existing tests pass.

New unit tests TestSeeLogConfig_WithoutStdout, TestSeelogConfig_JSONNoStdout, and TestSeelogConfig_JSONNoRollover have been added to validate new logic.

New tests cover the changes: no

Description for the changelog

Update logger Config setter access pattern

Does this PR include breaking model changes? If so, Have you added transformation functions?
no model breaking changes

Licensing

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

ecs-agent/logger/log.go Show resolved Hide resolved
ecs-agent/logger/log.go Show resolved Hide resolved
ecs-agent/logger/log.go Show resolved Hide resolved
ecs-agent/logger/log.go Outdated Show resolved Hide resolved
@timj-hh timj-hh force-pushed the logger_setters branch 4 times, most recently from 3096b59 to 7e523fa Compare December 18, 2023 22:54
ecs-agent/logger/log.go Outdated Show resolved Hide resolved
ecs-agent/logger/log.go Outdated Show resolved Hide resolved
amogh09
amogh09 previously approved these changes Dec 19, 2023
@timj-hh timj-hh merged commit 13e1852 into aws:dev Dec 19, 2023
36 checks passed
@mye956 mye956 mentioned this pull request Jan 9, 2024
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.

5 participants