Add SysLogger for new implementation. #17171

xincunli-sonic · 2023-11-14T20:03:25Z

This PR introduces the SysLogger to address an issue where the original Logger.py could override the SYSLOG_IDENTIFIER if a new Logger instance is created.

Why I did it?
This change was made because the SysLog was displaying NOTICE pmon#CCmisApi: instead of NOTICE pmon#ycable:. This discrepancy led to confusion during debugging and incorrect component identification in log analysis.

How I did it
I implemented an instance-level syslog using the logging.handlers module to prevent the SYSLOG_IDENTIFIER from being overridden.

How did you verify/test it?

Run code snippet

import SysLogger
from daemon_base import DaemonBase

# 1. SysLogger
SYSLOG_IDENTIFIER = "TestSysLogger"

# Global logger instance
log = SysLogger.SysLogger(SYSLOG_IDENTIFIER)

log.log_info("This is a info message")
log.log_notice("This is a notice message")
log.log_warning("This is a warning message")
log.log_debug("This is a debug message")
log.log_error("This is a error message")

log.log_info("This is a info message", also_print_to_console=True)
log.log_notice("This is a notice message", also_print_to_console=True)
log.log_warning("This is a warning message", also_print_to_console=True)
log.log_debug("This is a debug message", also_print_to_console=True)
log.log_error("This is a error message", also_print_to_console=True)


# 2. Daemonbase with default syslog.

log2 = DaemonBase(SYSLOG_IDENTIFIER)

log2.log_info("log2: This is a info message")
log2.log_notice("log2: This is a notice message")
log2.log_warning("log2: This is a warning message")
log2.log_debug("log2: This is a debug message")
log2.log_error("log2: This is a error message")

log2.log_info("log2: This is a info message", also_print_to_console=True)
log2.log_notice("log2: This is a notice message", also_print_to_console=True)
log2.log_warning("log2: This is a warning message", also_print_to_console=True)
log2.log_debug("log2: This is a debug message", also_print_to_console=True)
log2.log_error("log2: This is a error message", also_print_to_console=True)

# 3. Daemonbase with SysLogger.

log3 = DaemonBase(SYSLOG_IDENTIFIER, use_syslogger=True)

log3.log_info("log3: This is a info message")
log3.log_notice("log3: This is a notice message")
log3.log_warning("log3: This is a warning message")
log3.log_debug("log3: This is a debug message")
log3.log_error("log3: This is a error message")

log3.log_info("log3: This is a info message", also_print_to_console=True)
log3.log_notice("log3: This is a notice message", also_print_to_console=True)
log3.log_warning("log3: This is a warning message", also_print_to_console=True)
log3.log_debug("log3: This is a debug message", also_print_to_console=True)
log3.log_error("log3: This is a error message", also_print_to_console=True)

Sample output

Jan  5 20:28:18 xincun-dev-vm TestSysLogger: This is a info message
Jan  5 20:28:18 xincun-dev-vm TestSysLogger: This is a notice message
Jan  5 20:28:18 xincun-dev-vm TestSysLogger: This is a warning message
Jan  5 20:28:18 xincun-dev-vm TestSysLogger: This is a error message
Jan  5 20:28:18 xincun-dev-vm TestSysLogger: This is a info message
Jan  5 20:28:18 xincun-dev-vm TestSysLogger: This is a notice message
Jan  5 20:28:18 xincun-dev-vm TestSysLogger: This is a warning message
Jan  5 20:28:18 xincun-dev-vm TestSysLogger: This is a error message
Jan  5 20:28:18 xincun-dev-vm TestSysLogger[967382]: log2: This is a notice message
Jan  5 20:28:18 xincun-dev-vm TestSysLogger[967382]: log2: This is a warning message
Jan  5 20:28:18 xincun-dev-vm TestSysLogger[967382]: log2: This is a error message
Jan  5 20:28:18 xincun-dev-vm TestSysLogger[967382]: log2: This is a notice message
Jan  5 20:28:18 xincun-dev-vm TestSysLogger[967382]: log2: This is a warning message
Jan  5 20:28:18 xincun-dev-vm TestSysLogger[967382]: log2: This is a error message
Jan  5 20:28:18 xincun-dev-vm TestSysLogger: log3: This is a info message
Jan  5 20:28:18 xincun-dev-vm TestSysLogger: log3: This is a notice message
Jan  5 20:28:18 xincun-dev-vm TestSysLogger: log3: This is a warning message
Jan  5 20:28:18 xincun-dev-vm TestSysLogger: log3: This is a error message
Jan  5 20:28:18 xincun-dev-vm TestSysLogger: log3: This is a info message
Jan  5 20:28:18 xincun-dev-vm TestSysLogger: log3: This is a notice message
Jan  5 20:28:18 xincun-dev-vm TestSysLogger: log3: This is a warning message
Jan  5 20:28:18 xincun-dev-vm TestSysLogger: log3: This is a error message

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

202205
202305
202311

Signed-off-by: Xincun Li xincun.li@microsoft.com

prgeor · 2023-11-15T15:11:27Z

@nazariig could you help review?

Fix path issue.

src/sonic-py-common/sonic_py_common/daemon_base.py

src/sonic-py-common/sonic_py_common/SysLogger.py

nazariig · 2023-11-21T16:29:07Z

src/sonic-py-common/sonic_py_common/daemon_base.py

@@ -48,6 +66,20 @@ def __init__(self, log_identifier):
        if not signal.getsignal(signal.SIGTERM):
            signal.signal(signal.SIGTERM, self.signal_handler)

+    def log(self, priority, msg, also_print_to_console=False):


@xincunli-sonic IMHO, this is a hack and in general a bad architecture. The method you override, was not designed to be overridable. Also, the logger class is used in many other places, not just only a various python demons implementation. This means, that the issue appears to be in many places. Ideally, there should be two different implementations, which share the same interface (e.g., LoggerBase[defines logger interface API], SystemLogger[uses native syslog], SyslogLogger[uses syslog handler], FileLogger[uses file handler]). But that will lead up to a major code changes. I would suggest to abandon having a separate SyslogLogger implementation and just fix the original one, preserving all the defined set of APIs.

This is intending to be reduce the scope of new change. Here is the consideration

Most of daemon process will inherit from Daemon_Base with old logger implementation. If we use the new SysLogger, then it could be used for all daemons.

What we want to do would be only apply new SysLogger to some new Daemon process, like ycabled etc.

The override function, you can think it like factory pattern, which depends on use_syslogger flag as optional parameter, if it is true then it will use new SysLogger, otherwise it keeps the old logger and behavior will not change.

src/sonic-py-common/sonic_py_common/syslogger.py

src/sonic-py-common/sonic_py_common/daemon_base.py

bingwang-ms · 2024-02-13T23:21:38Z

/azp run

azure-pipelines · 2024-02-13T23:21:44Z

Commenter does not have sufficient privileges for PR 17171 in repo sonic-net/sonic-buildimage

qiluo-msft · 2024-02-13T23:25:52Z

/azp run Azure.sonic-buildimage

azure-pipelines · 2024-02-13T23:26:06Z

Azure Pipelines successfully started running 1 pipeline(s).

src/sonic-py-common/sonic_py_common/syslogger.py

xincunli-sonic · 2024-04-23T18:55:17Z

/azpw run

mssonicbld · 2024-04-23T18:55:19Z

/AzurePipelines run

azure-pipelines · 2024-04-23T18:55:33Z

Azure Pipelines successfully started running 1 pipeline(s).

xincunli-sonic · 2024-05-01T16:46:20Z

/azpw run

mssonicbld · 2024-05-01T16:46:23Z

/AzurePipelines run

azure-pipelines · 2024-05-01T16:46:36Z

Azure Pipelines successfully started running 1 pipeline(s).

xincunli-sonic · 2024-05-02T00:24:31Z

/azpw run

mssonicbld · 2024-05-02T00:24:33Z

/AzurePipelines run

azure-pipelines · 2024-05-02T00:24:47Z

Azure Pipelines successfully started running 1 pipeline(s).

xincunli-sonic · 2024-05-02T17:29:39Z

/azpw run

mssonicbld · 2024-05-02T17:29:41Z

/AzurePipelines run

azure-pipelines · 2024-05-02T17:29:56Z

Azure Pipelines successfully started running 1 pipeline(s).

liat-grozovik · 2024-05-06T06:49:10Z

@xincunli-sonic could you please help to prioritize this PR feedback and checkers?
this infra is required for additional PR related here as well and we should have it on 202405

Junchao-Mellanox · 2024-05-06T08:34:37Z

Hi @xincunli-sonic , I suppose the checker failure is related to the code change. The unit test tried to connect to a unix socket of a logger server, but the unix socket does not exist.

--- Logging error ---
Traceback (most recent call last):
  File "/usr/lib/python3.11/logging/handlers.py", line 1007, in emit
    self.socket.send(msg)
OSError: [Errno 9] Bad file descriptor

Junchao-Mellanox · 2024-05-09T01:13:06Z

Hi @prgeor , could you please kindly review this?

prgeor · 2024-05-15T00:19:23Z

@liat-grozovik can you merge?

liat-grozovik · 2024-05-15T05:50:23Z

i removed the backport ask for old branches as 202205 and 202305. if this is must lets discuss.
i assume this is not needed for 202311 as well.

Add SysLogger.

af3353e

xincunli-sonic requested a review from prgeor November 14, 2023 20:12

Fix SysLogger Path

ce22e71

prgeor requested a review from nazariig November 15, 2023 15:11

xincunli-sonic and others added 2 commits November 15, 2023 16:46

improve format.

cfbdcec

Update daemon_base.py

72e0524

Fix path issue.

nazariig reviewed Nov 16, 2023

View reviewed changes

src/sonic-py-common/sonic_py_common/daemon_base.py Outdated Show resolved Hide resolved

nazariig reviewed Nov 16, 2023

View reviewed changes

src/sonic-py-common/sonic_py_common/SysLogger.py Outdated Show resolved Hide resolved

xincunli-sonic and others added 4 commits November 16, 2023 10:11

Update daemon_base.py

badb1e8

remove syslogger

e1b211d

add back sysloger for lower case file name.

c1a1813

fix mapping.

c163d58

nazariig reviewed Nov 21, 2023

View reviewed changes

nazariig approved these changes Dec 11, 2023

View reviewed changes

xincunli-sonic added 4 commits December 16, 2023 15:58

Update daemon_base.py

6d29d33

Update daemon_base.py

46028ae

Update daemon_base.py

28d1de4

Update syslogger.py

d918a64

prgeor reviewed Feb 1, 2024

View reviewed changes

src/sonic-py-common/sonic_py_common/syslogger.py Outdated Show resolved Hide resolved

prgeor reviewed Feb 1, 2024

View reviewed changes

src/sonic-py-common/sonic_py_common/daemon_base.py Outdated Show resolved Hide resolved

xincunli-sonic added 2 commits February 2, 2024 12:03

Update daemon_base.py to change use_syslogger default as true.

47e42ec

Update syslogger.py

42619fd

xincunli-sonic added 2 commits February 14, 2024 09:59

Remove address.

a0fb65e

fix parameter name

1f50042

prgeor reviewed Feb 15, 2024

View reviewed changes

src/sonic-py-common/sonic_py_common/syslogger.py Outdated Show resolved Hide resolved

Junchao-Mellanox approved these changes Apr 22, 2024

View reviewed changes

Update syslogger.py

26b5de3

Update daemon_base.py

0a0bd01

xincunli-sonic and others added 2 commits May 6, 2024 09:02

Update syslogger.py

d4982f1

Fix UT

3607f8f

xincunli-sonic requested a review from lguohan as a code owner May 6, 2024 21:12

xincunli-sonic and others added 3 commits May 6, 2024 15:18

Merge branch 'master' into xincun/add-new-syslogger

d9d9675

Update test_system_health.py

78c8359

Fix UT

3471d48

prgeor approved these changes May 15, 2024

View reviewed changes

liat-grozovik merged commit 166a9fb into sonic-net:master May 15, 2024
19 checks passed

xincunli-sonic mentioned this pull request May 20, 2024

Fix CCmisApi identifier overrides ycable identifier in syslog. sonic-net/sonic-platform-common#409

Closed

3 tasks

ZhaohuiS mentioned this pull request Dec 27, 2024

Set default syslog level to INFO instead of NOTICE #21291

Open

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add SysLogger for new implementation. #17171

Add SysLogger for new implementation. #17171

xincunli-sonic commented Nov 14, 2023 •

edited by liat-grozovik

Loading

prgeor commented Nov 15, 2023

nazariig Nov 21, 2023

xincunli-sonic Nov 30, 2023

bingwang-ms commented Feb 13, 2024

azure-pipelines bot commented Feb 13, 2024

qiluo-msft commented Feb 13, 2024

azure-pipelines bot commented Feb 13, 2024

xincunli-sonic commented Apr 23, 2024

mssonicbld commented Apr 23, 2024

azure-pipelines bot commented Apr 23, 2024

xincunli-sonic commented May 1, 2024

mssonicbld commented May 1, 2024

azure-pipelines bot commented May 1, 2024

xincunli-sonic commented May 2, 2024

mssonicbld commented May 2, 2024

azure-pipelines bot commented May 2, 2024

xincunli-sonic commented May 2, 2024

mssonicbld commented May 2, 2024

azure-pipelines bot commented May 2, 2024

liat-grozovik commented May 6, 2024

Junchao-Mellanox commented May 6, 2024

Junchao-Mellanox commented May 9, 2024

prgeor commented May 15, 2024

liat-grozovik commented May 15, 2024 •

edited

Loading

Add SysLogger for new implementation. #17171

Add SysLogger for new implementation. #17171

Conversation

xincunli-sonic commented Nov 14, 2023 • edited by liat-grozovik Loading

prgeor commented Nov 15, 2023

nazariig Nov 21, 2023

Choose a reason for hiding this comment

xincunli-sonic Nov 30, 2023

Choose a reason for hiding this comment

bingwang-ms commented Feb 13, 2024

azure-pipelines bot commented Feb 13, 2024

qiluo-msft commented Feb 13, 2024

azure-pipelines bot commented Feb 13, 2024

xincunli-sonic commented Apr 23, 2024

mssonicbld commented Apr 23, 2024

azure-pipelines bot commented Apr 23, 2024

xincunli-sonic commented May 1, 2024

mssonicbld commented May 1, 2024

azure-pipelines bot commented May 1, 2024

xincunli-sonic commented May 2, 2024

mssonicbld commented May 2, 2024

azure-pipelines bot commented May 2, 2024

xincunli-sonic commented May 2, 2024

mssonicbld commented May 2, 2024

azure-pipelines bot commented May 2, 2024

liat-grozovik commented May 6, 2024

Junchao-Mellanox commented May 6, 2024

Junchao-Mellanox commented May 9, 2024

prgeor commented May 15, 2024

liat-grozovik commented May 15, 2024 • edited Loading

xincunli-sonic commented Nov 14, 2023 •

edited by liat-grozovik

Loading

liat-grozovik commented May 15, 2024 •

edited

Loading