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

fixed:Log level does not work on V2 logs #4300

Open
wants to merge 5 commits into
base: 6.0release
Choose a base branch
from

Conversation

xengine-qyt
Copy link
Contributor

No description provided.

@winlinvip winlinvip added the EnglishNative This issue is conveyed exclusively in English. label Mar 13, 2025
Copy link
Contributor

@suzp1984 suzp1984 left a comment

Choose a reason for hiding this comment

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

I don't think this PR make sense.

return SrsLogLevelWarn;
} else if ("error" == level) {
} else if ("ERROR" == level) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this PR don't make sense.
The SRS_LOG_LEVEL_V2 is enabled by default.

SRS_LOG_LEVEL_V2=YES

The SRS_LOG_LEVEL_V2, which means the log level names defined in https://github.com/apache/logging-log4j2/blob/2.x/log4j-api/src/main/java/org/apache/logging/log4j/Level.java
or
https://stackoverflow.com/questions/2031163/when-to-use-the-different-log-levels/2031209#2031209

This func is just a bridge between log level enum, defined by SRS, and srs.conf. So it's not necessary to redefine another SrsLogLevelXXX for log level 2.

The only fault of this func is log level fatal is missing, which can be mapping to SrsLogLevelError anyway.

If the level string are upper cases, the srs.conf and Unit test cases need to change to match this change, so I don't think its a good idea also.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

.o,it different from the documentation
image

Copy link
Contributor

Choose a reason for hiding this comment

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

upper cases terms, TRACE, DEBUG, INFO, WARN, ERROR, are print to logs, not the configurable claims, use lower-cases in conf file.

@@ -14,7 +14,7 @@
const char* srs_log_level_strings[] = {
#ifdef SRS_LOG_LEVEL_V2
// The v2 log level specs by log4j.
"FORB", "TRACE", "DEBUG", NULL, "INFO", NULL, NULL, NULL,
"FORB", "VERB", "TRACE", "DEBUG", "INFO", NULL, NULL, NULL,
Copy link
Contributor

Choose a reason for hiding this comment

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

https://stackoverflow.com/questions/2031163/when-to-use-the-different-log-levels/2031209#2031209
https://github.com/apache/logging-log4j2/blob/2.x/log4j-api/src/main/java/org/apache/logging/log4j/Level.java

VERB is not the term defined by log level v2.
And the log level sequence is incorrect.
srs_log_level_strings[1] = TRACE;
srs_log_level_strings[2] = DEBUG;
srs_log_level_strings[4] = INFO;
srs_log_level_strings[8] = WARN;
srs_log_level_strings[16] = ERROR;
srs_log_level_strings[32] = FATAL;
FATAL is not there, instead a [32] = OFF, maybe it is the only arguable point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

@@ -20,20 +20,36 @@

// The log level, see https://github.com/apache/logging-log4j2/blob/release-2.x/log4j-api/src/main/java/org/apache/logging/log4j/Level.java
// Please note that the enum name might not be the string, to keep compatible with previous definition.
#ifdef SRS_LOG_LEVEL_V2
Copy link
Contributor

Choose a reason for hiding this comment

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

SrsLogLevel is SRS's internal log level enum.
SRS_LOG_LEVEL_V2 controls the log print with the new standard, ref https://stackoverflow.com/questions/2031163/when-to-use-the-different-log-levels/2031209#2031209
then the log analysis tools can compatible with those new log level keywords.
So, redefine SrsLogLevel don't make sense here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

@duiniuluantanqin
Copy link
Member

duiniuluantanqin commented Mar 14, 2025

Do not replace spaces with tabs; it is uncomfortable to look at.

TRANS_BY_GPT4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
EnglishNative This issue is conveyed exclusively in English.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants