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

Support notice level log #19232

Merged
merged 2 commits into from
Aug 13, 2024

Conversation

Junchao-Mellanox
Copy link
Collaborator

Why I did it

SysLogger does not support NOTICE level log. The PR is to support it.

Work item tracking
  • Microsoft ADO (number only):

How I did it

Add a new log level NOTICE to python logging package.

How to verify it

Manual test

Jun  3 03:09:44.353536 sonic NOTICE test[364076]: This is a notice

Which release branch to backport (provide reason below if selected)

  • 201811
  • 201911
  • 202006
  • 202012
  • 202106
  • 202111
  • 202205
  • 202211
  • 202305

Tested branch (Please provide the tested image version)

Description for the changelog

Link to config_db schema for YANG module changes

A picture of a cute animal (not mandatory but encouraged)

Copy link

linux-foundation-easycla bot commented Jun 6, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@Junchao-Mellanox
Copy link
Collaborator Author

/eazycla

Copy link
Contributor

@xincunli-sonic xincunli-sonic left a comment

Choose a reason for hiding this comment

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

LGTM

@keboliu
Copy link
Collaborator

keboliu commented Jul 9, 2024

Hi @prgeor would you please help to merge?

@@ -4,13 +4,19 @@
import socket
import sys

# customize python logging to support notice logger
logging.NOTICE = logging.INFO + 1
Copy link
Contributor

Choose a reason for hiding this comment

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

@Junchao-Mellanox how did you arrive at the formula?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Python logging package support customizing log level. I read the source code of it and find that:

#---------------------------------------------------------------------------
#
# Default levels and level names, these can be replaced with any positive set
# of values having corresponding names. There is a pseudo-level, NOTSET, which
# is only really there as a lower limit for user-defined levels. Handlers and
# loggers are initialized with NOTSET so that they will log all messages, even
# at user-defined levels.
#

CRITICAL = 50
FATAL = CRITICAL
ERROR = 40
WARNING = 30
WARN = WARNING
INFO = 20
DEBUG = 10
NOTSET = 0

As you can see, there are 10 spare numbers between INFO and WARN, so I take INFO+1 as the NOTICE level.

prgeor
prgeor previously approved these changes Jul 23, 2024
@Junchao-Mellanox
Copy link
Collaborator Author

Hi @qiluo-msft , could you please review this?

qiluo-msft
qiluo-msft previously approved these changes Aug 5, 2024
@Junchao-Mellanox Junchao-Mellanox dismissed stale reviews from qiluo-msft and prgeor via 7f7286f August 6, 2024 02:13
@Junchao-Mellanox
Copy link
Collaborator Author

Hi @qiluo-msft @prgeor , I added a test case and all checker passed. Could you please help merge this?

@qiluo-msft qiluo-msft merged commit b535f28 into sonic-net:master Aug 13, 2024
22 checks passed
matiAlfaro pushed a commit to Marvell-switching/sonic-buildimage that referenced this pull request Aug 21, 2024
#### Why I did it

SysLogger does not support NOTICE level log. The PR is to support it.

#### How I did it

Add a new log level NOTICE to python logging package.

#### How to verify it

Manual test

```
Jun  3 03:09:44.353536 sonic NOTICE test[364076]: This is a notice
```
mssonicbld pushed a commit to mssonicbld/sonic-buildimage that referenced this pull request Aug 23, 2024
#### Why I did it

SysLogger does not support NOTICE level log. The PR is to support it.

#### How I did it

Add a new log level NOTICE to python logging package.

#### How to verify it

Manual test

```
Jun  3 03:09:44.353536 sonic NOTICE test[364076]: This is a notice
```
@mssonicbld
Copy link
Collaborator

Cherry-pick PR to 202405: #20010

mssonicbld pushed a commit that referenced this pull request Sep 7, 2024
#### Why I did it

SysLogger does not support NOTICE level log. The PR is to support it.

#### How I did it

Add a new log level NOTICE to python logging package.

#### How to verify it

Manual test

```
Jun  3 03:09:44.353536 sonic NOTICE test[364076]: This is a notice
```
mssonicbld added a commit to mssonicbld/sonic-host-services that referenced this pull request Jan 9, 2025
### **Issue**
Fix the issue sonic-net/sonic-buildimage#21290
No info log found in syslog on 202405 image for caclmgrd

### **Work item tracking**
- Microsoft ADO **(number only)**:
30611546

### Why did it happen
RP sonic-net/sonic-buildimage#17171, introduced a new Class SysLogger, DaemonBase will choose SysLogger by default
PR sonic-net/sonic-buildimage#19232, it added noticed level and make it to be default level which suppresses INFO logs.

`caclmgr.set_min_log_priority_info()` it sets min log priority to info, this function is in Logger class, SysLogger doesn't have this function. But DaemonBase still inherits Logger which implements set_min_log_priority_info, that's why even caclmgrd called this function, it didn't throw exception. But it didn't make INFO level effect in SysLogger which is actually used in caclmgrd

Even change to use Logger by setting `use_syslogger=False`, it still doesn't work.
The root cause is that it added a new instance for logger, `self.logger_instance`, any instance inherited from DaemonBase class can't change the debug level, the level they changed is their own instance, not the self.logger_instance's level.

### **How to fix**
The solution here for caclmgrd is to choose logger.Logger class instead of DaemonBase.

### **How to verify it**
Test it on 202405
bingwang-ms pushed a commit to sonic-net/sonic-host-services that referenced this pull request Jan 9, 2025
### **Issue**
Fix the issue sonic-net/sonic-buildimage#21290
No info log found in syslog on 202405 image for caclmgrd

### **Work item tracking**
- Microsoft ADO **(number only)**:
30611546

### Why did it happen
RP sonic-net/sonic-buildimage#17171, introduced a new Class SysLogger, DaemonBase will choose SysLogger by default
PR sonic-net/sonic-buildimage#19232, it added noticed level and make it to be default level which suppresses INFO logs.

`caclmgr.set_min_log_priority_info()` it sets min log priority to info, this function is in Logger class, SysLogger doesn't have this function. But DaemonBase still inherits Logger which implements set_min_log_priority_info, that's why even caclmgrd called this function, it didn't throw exception. But it didn't make INFO level effect in SysLogger which is actually used in caclmgrd

Even change to use Logger by setting `use_syslogger=False`, it still doesn't work.
The root cause is that it added a new instance for logger, `self.logger_instance`, any instance inherited from DaemonBase class can't change the debug level, the level they changed is their own instance, not the self.logger_instance's level.

### **How to fix**
The solution here for caclmgrd is to choose logger.Logger class instead of DaemonBase.

### **How to verify it**
Test it on 202405
mssonicbld added a commit to mssonicbld/sonic-host-services that referenced this pull request Feb 7, 2025
### **Issue**
Fix the issue sonic-net/sonic-buildimage#21290
No info log found in syslog on 202405 image for caclmgrd

### **Work item tracking**
- Microsoft ADO **(number only)**:
30611546

### Why did it happen
RP sonic-net/sonic-buildimage#17171, introduced a new Class SysLogger, DaemonBase will choose SysLogger by default
PR sonic-net/sonic-buildimage#19232, it added noticed level and make it to be default level which suppresses INFO logs.

`caclmgr.set_min_log_priority_info()` it sets min log priority to info, this function is in Logger class, SysLogger doesn't have this function. But DaemonBase still inherits Logger which implements set_min_log_priority_info, that's why even caclmgrd called this function, it didn't throw exception. But it didn't make INFO level effect in SysLogger which is actually used in caclmgrd

Even change to use Logger by setting `use_syslogger=False`, it still doesn't work.
The root cause is that it added a new instance for logger, `self.logger_instance`, any instance inherited from DaemonBase class can't change the debug level, the level they changed is their own instance, not the self.logger_instance's level.

### **How to fix**
The solution here for caclmgrd is to choose logger.Logger class instead of DaemonBase.

### **How to verify it**
Test it on 202405
mssonicbld added a commit to sonic-net/sonic-host-services that referenced this pull request Feb 7, 2025
### **Issue**
Fix the issue sonic-net/sonic-buildimage#21290
No info log found in syslog on 202405 image for caclmgrd

### **Work item tracking**
- Microsoft ADO **(number only)**:
30611546

### Why did it happen
RP sonic-net/sonic-buildimage#17171, introduced a new Class SysLogger, DaemonBase will choose SysLogger by default
PR sonic-net/sonic-buildimage#19232, it added noticed level and make it to be default level which suppresses INFO logs.

`caclmgr.set_min_log_priority_info()` it sets min log priority to info, this function is in Logger class, SysLogger doesn't have this function. But DaemonBase still inherits Logger which implements set_min_log_priority_info, that's why even caclmgrd called this function, it didn't throw exception. But it didn't make INFO level effect in SysLogger which is actually used in caclmgrd

Even change to use Logger by setting `use_syslogger=False`, it still doesn't work.
The root cause is that it added a new instance for logger, `self.logger_instance`, any instance inherited from DaemonBase class can't change the debug level, the level they changed is their own instance, not the self.logger_instance's level.

### **How to fix**
The solution here for caclmgrd is to choose logger.Logger class instead of DaemonBase.

### **How to verify it**
Test it on 202405
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.

7 participants