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

change the logger level, fix #296 #307

Merged
merged 1 commit into from
Oct 19, 2021
Merged

Conversation

nevill
Copy link
Contributor

@nevill nevill commented Oct 18, 2021

Check #296 for more details.

Copy link

@megaeasex megaeasex left a comment

Choose a reason for hiding this comment

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

[TASK:easegress-pr-test SUCCESS]megaease/easegress Pull Request 307 Deploy Test Success

@codecov-commenter
Copy link

codecov-commenter commented Oct 18, 2021

Codecov Report

Merging #307 (16e4d54) into main (265d91a) will decrease coverage by 0.04%.
The diff coverage is 16.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #307      +/-   ##
==========================================
- Coverage   80.32%   80.28%   -0.05%     
==========================================
  Files          53       53              
  Lines        5926     5929       +3     
==========================================
  Hits         4760     4760              
- Misses        906      909       +3     
  Partials      260      260              
Impacted Files Coverage Δ
pkg/filter/proxy/server.go 69.36% <16.66%> (-1.23%) ⬇️

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 265d91a...16e4d54. Read the comment docs.

@nevill nevill changed the title change the logger fix #296 change the logger level, fix #296 Oct 18, 2021
@@ -135,8 +135,11 @@ func (s *servers) handleEvent(event *serviceregistry.ServiceEvent) {
func (s *servers) tryUseService() {
serviceInstanceSpecs, err := s.serviceRegistry.ListServiceInstances(s.poolSpec.ServiceRegistry, s.poolSpec.ServiceName)

logger.Warnf("successfully use service %s/%s",
Copy link
Contributor

Choose a reason for hiding this comment

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

The level should be Info.

Copy link
Collaborator

Choose a reason for hiding this comment

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

And it is not good to put the message here: this is a success message, but it may log a warning later in useService.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, this message is not supposed to be committed. 🤔

if err != nil {
logger.Errorf("get service %s/%s failed: %v",
logger.Warnf("first try to use service %s/%s failed: %v, will try again when it's created",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
logger.Warnf("first try to use service %s/%s failed: %v, will try again when it's created",
logger.Warnf("try to use service %s/%s failed(will try again): %v",

Copy link

@megaeasex megaeasex left a comment

Choose a reason for hiding this comment

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

[TASK:easegress-pr-test SUCCESS]megaease/easegress Pull Request 307 Deploy Test Success

@xxx7xxxx xxx7xxxx merged commit 7c1b6d3 into easegress-io:main Oct 19, 2021
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