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.

Sign up for GitHub

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

Jump to bottom

Integrate with lumberjack for log rotation #1504

Merged
merged 13 commits into from Nov 24, 2023
Merged

Integrate with lumberjack for log rotation #1504

merged 13 commits into from Nov 24, 2023

Conversation

ashmeenkaur
Copy link
Collaborator

@ashmeenkaur ashmeenkaur commented Nov 16, 2023 •
edited

Description

  1. This PR enables log rotation on GCSFuse logs by integrating with lumberjack.
  2. This PR also cleans up internal/logger/logger.go and corresponding changes to internal/fs/fs_test.go to remove unused logger methods.

Rotated file format: <log-file-name>-<date in YYYY-MM-DD format>T<time in HH-MM-SS.millisecond format>.<file extension>.<gz if compressed>
Example: log-2023-11-22T23-22-58.487.txt.gz

In case of remount with no change in the log file name, old logs from the previous mount will be retained and counted against the log rotation limits specified in config file.

Link to the issue in case of a bug fix.

NA

Testing details

  1. Manual - Manually verified on VM and docker image that log rotation works as expected.
  2. Unit tests - NA
  3. Integration tests - Added

Perf Test Results:
log-enabled-perf

@ashmeenkaur ashmeenkaur added execute-perf-test Execute performance test in PR execute-integration-tests Run only integration tests labels Nov 16, 2023
@ashmeenkaur ashmeenkaur marked this pull request as ready for review November 16, 2023 08:41
@ashmeenkaur ashmeenkaur changed the base branch from log_rotation_configs to dec_2023_release November 17, 2023 09:56
@ashmeenkaur ashmeenkaur removed the execute-perf-test Execute performance test in PR label Nov 21, 2023
@raj-prince
Copy link
Collaborator

As discussed offline:

  1. Please update the PR description with rotated-file names format, and also what happens in case of remount.
  2. Generate the logs by running concurrent worker (around 10) to reduce the time to generate logs.
  3. Please check the GCSFuse performance with and without this changes by enabling all the logs.
  4. Please add the integration test for the uncompressed rotated file.

@ashmeenkaur ashmeenkaur added the execute-perf-test Execute performance test in PR label Nov 23, 2023
@ashmeenkaur ashmeenkaur force-pushed the integrate_with_lumberjack branch 6 times, most recently from 916f3eb to 22b2886 Compare November 23, 2023 05:17
@ashmeenkaur ashmeenkaur removed the execute-perf-test Execute performance test in PR label Nov 23, 2023
raj-prince
raj-prince reviewed Nov 24, 2023
View reviewed changes
tools/integration_tests/log_rotation/log_rotation_test.go Outdated
// Set up test directory.
setup.SetUpTestDirForTestBucketFlag()

logDirPath = setup.SetUpLogDir(logDirName)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just curious, for mounted directly (line: 64) we didn't create directory (logDirPath), here we need to create the directory (logDirPath).

Anyway we should create the path using join here only. Then we should create the directory via utility method.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The problem here was that log file is generated at the time of mount. So for mounted directory tests, the directory should be there beforehand. I've added it to run_tests_mounted_directory script and added a check to fail the test if directory is not present.

The path is different for both the tests. One is in test directory and the other in /tmp, hence we need to do this twice. This might be significant refactoring work to move the logic together so I would say we should do it in a separate PR if necessary. (Did some refactoring to make it cleaner though.)

raj-prince
raj-prince requested changes Nov 24, 2023
View reviewed changes
Copy link
Collaborator

@raj-prince raj-prince left a comment

Choose a reason for hiding this comment

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

Minor comments.

@ashmeenkaur ashmeenkaur merged commit d657c59 into dec_2023_release Nov 24, 2023
8 checks passed
gargnitingoogle pushed a commit that referenced this pull request Nov 28, 2023
@ashmeenkaur @gargnitingoogle
Integrate with lumberjack for log rotation (#1504)
dfe5281 
* integrate with lumberjack

* run perf test

* fix fio error

* fix operations tests

* reduce number of  log rotations to reduce integration test execution time

* review comments

* temp commit to run perf tests with trace logs

* fix lint issues

* temp commit

* review comments

* fix lint

* revert perf changes

* refactoring based on review comments
gargnitingoogle pushed a commit that referenced this pull request Nov 28, 2023
@ashmeenkaur @gargnitingoogle
Integrate with lumberjack for log rotation (#1504)
ac2ac4d 
* integrate with lumberjack

* run perf test

* fix fio error

* fix operations tests

* reduce number of  log rotations to reduce integration test execution time

* review comments

* temp commit to run perf tests with trace logs

* fix lint issues

* temp commit

* review comments

* fix lint

* revert perf changes

* refactoring based on review comments
gargnitingoogle pushed a commit that referenced this pull request Nov 28, 2023
@ashmeenkaur @gargnitingoogle
Integrate with lumberjack for log rotation (#1504)
6965726 
* integrate with lumberjack

* run perf test

* fix fio error

* fix operations tests

* reduce number of  log rotations to reduce integration test execution time

* review comments

* temp commit to run perf tests with trace logs

* fix lint issues

* temp commit

* review comments

* fix lint

* revert perf changes

* refactoring based on review comments
gargnitingoogle pushed a commit that referenced this pull request Nov 29, 2023
@ashmeenkaur @gargnitingoogle
Integrate with lumberjack for log rotation (#1504)
ac813f1 
* integrate with lumberjack

* run perf test

* fix fio error

* fix operations tests

* reduce number of  log rotations to reduce integration test execution time

* review comments

* temp commit to run perf tests with trace logs

* fix lint issues

* temp commit

* review comments

* fix lint

* revert perf changes

* refactoring based on review comments
raj-prince pushed a commit that referenced this pull request Nov 29, 2023
@ashmeenkaur @raj-prince
Integrate with lumberjack for log rotation (#1504)
0288949 
* integrate with lumberjack

* run perf test

* fix fio error

* fix operations tests

* reduce number of  log rotations to reduce integration test execution time

* review comments

* temp commit to run perf tests with trace logs

* fix lint issues

* temp commit

* review comments

* fix lint

* revert perf changes

* refactoring based on review comments
@ashmeenkaur ashmeenkaur deleted the integrate_with_lumberjack branch December 4, 2023 05:57
@p-linnane p-linnane mentioned this pull request Dec 18, 2023
Merged
gargnitingoogle pushed a commit that referenced this pull request Jan 4, 2024
@ashmeenkaur @gargnitingoogle
Integrate with lumberjack for log rotation (#1504)
db3a710 
* integrate with lumberjack

* run perf test

* fix fio error

* fix operations tests

* reduce number of  log rotations to reduce integration test execution time

* review comments

* temp commit to run perf tests with trace logs

* fix lint issues

* temp commit

* review comments

* fix lint

* revert perf changes

* refactoring based on review comments
ashmeenkaur added a commit that referenced this pull request Jan 9, 2024
@ashmeenkaur
Integrate with lumberjack for log rotation (#1504)
7a054b5 
* integrate with lumberjack

* run perf test

* fix fio error

* fix operations tests

* reduce number of  log rotations to reduce integration test execution time

* review comments

* temp commit to run perf tests with trace logs

* fix lint issues

* temp commit

* review comments

* fix lint

* revert perf changes

* refactoring based on review comments
ashmeenkaur added a commit that referenced this pull request Jan 10, 2024
@ashmeenkaur
Integrate with lumberjack for log rotation (#1504)
db2be92 
* integrate with lumberjack

* run perf test

* fix fio error

* fix operations tests

* reduce number of  log rotations to reduce integration test execution time

* review comments

* temp commit to run perf tests with trace logs

* fix lint issues

* temp commit

* review comments

* fix lint

* revert perf changes

* refactoring based on review comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@raj-prince raj-prince raj-prince approved these changes

@sethiay sethiay Awaiting requested review from sethiay sethiay is a code owner

@vadlakondaswetha vadlakondaswetha Awaiting requested review from vadlakondaswetha vadlakondaswetha is a code owner

Assignees
No one assigned
Labels
logging improvements - log rotation Representative-PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@ashmeenkaur @raj-prince