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

[WIP] Fix logrus-entry-contents when it is in hooks. #7037

Closed

Conversation

yanggangtony
Copy link
Contributor

@yanggangtony yanggangtony commented Oct 31, 2023

Thank you for contributing to Velero!

Please add a summary of your change

In the issue #6187 , we found some strange behaviour of the logs output.
I add some test code for the logrus entry hook.
I found that when the first new the NewTempFileLogger and init the logrus.Fields , it works as expect.

But when i override the value of logrus.Fields , it can not be works.
Like the pics i test it.

image

Does your change fix a particular issue?

Fixes #6187

Please indicate you've done the following:

  • Accepted the DCO. Commits without the DCO will delay acceptance.
  • Created a changelog file or added /kind changelog-not-required as a comment on this pull request.
  • Updated the corresponding documentation in site/content/docs/main.

@yanggangtony yanggangtony changed the title [WIP] Add a unit test case for logrus-entry-contents [WIP] Fix logrus-entry-contents when it is in hooks. Oct 31, 2023
@codecov
Copy link

codecov bot commented Oct 31, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 61.65%. Comparing base (174c10f) to head (b1da01f).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7037      +/-   ##
==========================================
+ Coverage   61.64%   61.65%   +0.01%     
==========================================
  Files         262      262              
  Lines       28480    28480              
==========================================
+ Hits        17556    17559       +3     
+ Misses       9691     9689       -2     
+ Partials     1233     1232       -1     

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

@yanggangtony
Copy link
Contributor Author

image

@yanggangtony
Copy link
Contributor Author

yanggangtony commented Nov 1, 2023

ReTest the logrus's hooks and entry output.
I can roughly get the conclusion of the output logs when used .WithError( or logger.WithFields(. It maybe a logrus's design , when the entry is init , can not be changed anymore in hooks logic.

How do you think about that ? @Lyndon-Li @sseago
If i changed the code , delete the logger.WithFields( and logger.WithError( or what some will changed the logrus.Fields in sub method under func (b *backupReconciler) runBackup( ?
Just let the logs use the init logrus.Fields.

But this change will be very weird for code review, or not the clean code .

@yanggangtony yanggangtony deleted the fix-logrus-entry-contents branch March 11, 2024 01:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Output of backup describe doesn't include warnings printed in logs
1 participant