-
Notifications
You must be signed in to change notification settings - Fork 6
VPLAY-11732: Add sequential log numbering with function name for missing log detection #698
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
base: dev_sprint_25_2
Are you sure you want to change the base?
Changes from 15 commits
7a5a09a
199466a
599ad9a
f7e5089
f13fb59
22e2a38
cfce2c8
24ee6ec
1328546
3ef6bbb
128c6c5
b60dd6b
031d7b8
fd246c3
e9f2b67
4ae13d2
ac798fd
f34c045
4601520
03d6b09
4c08d27
c9d926b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,6 +26,8 @@ | |
| #include <algorithm> | ||
| #include <thread> | ||
| #include <sstream> | ||
| #include <atomic> | ||
| #include <cstring> | ||
| #include "priv_aamp.h" | ||
| using namespace std; | ||
|
|
||
|
|
@@ -79,11 +81,17 @@ bool AampLogManager::locked = false; | |
|
|
||
| thread_local int gPlayerId = -1; | ||
|
|
||
| // Sequential log counter for tracking missing log lines | ||
| static std::atomic<uint32_t> gLogCounter(0); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could use uint8_t (or uint16_t) and just let it wrap naturally without needing modulus operation?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it would be more confusing. If we used uint8_t, the counter would wrap after 255; and if we used uint16_t it would be even more confusing because it would wrap sometimes after 999 and sometimes after 535 (0xFFFF = 65535) |
||
|
|
||
| /** | ||
| * @brief Print logs to console / log file | ||
| */ | ||
| void logprintf(AAMP_LogLevel logLevelIndex, const char* file, int line, const char *format, ...) | ||
| void logprintf(AAMP_LogLevel logLevelIndex, const char* file, const char* func, int line, const char *format, ...) | ||
| { | ||
| // Increment log counter for each log line | ||
| uint32_t logSeqNum = gLogCounter.fetch_add(1, std::memory_order_relaxed); | ||
jfagunde marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| char timestamp[AAMPCLI_TIMESTAMP_PREFIX_MAX_CHARS]; | ||
| timestamp[0] = 0x00; | ||
| if( AampLogManager::disableLogRedirection ) | ||
|
|
@@ -98,12 +106,13 @@ void logprintf(AAMP_LogLevel logLevelIndex, const char* file, int line, const ch | |
| for( int pass=0; pass<2; pass++ ) | ||
| { // two pass: measure required bytes then populate format string | ||
| format_bytes = snprintf(format_ptr, format_bytes, | ||
| "%s[AAMP-PLAYER][%d][%s][%zx][%s][%d]%s\n", | ||
| "%s[AAMP-PLAYER][%03u][%d][%s][%zx][%s][%d]%s\n", | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. important to pad with leading zeros?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is not essential but helps keeping lines aligned: But I am happy to change it if you think that it is better to save a couple of characters (although I'd suggest replacing |
||
| timestamp, | ||
| logSeqNum, | ||
| gPlayerId, | ||
| mLogLevelStr[logLevelIndex], | ||
| GetPrintableThreadID(), | ||
| file, line, | ||
| func, line, | ||
| format ); | ||
| if( format_bytes<=0 ) | ||
| { // should never happen! | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -131,7 +131,7 @@ void DumpBinaryBlob(const unsigned char *ptr, size_t len); | |
| * @param[in] format - printf style string | ||
| * @return void | ||
| */ | ||
| extern void logprintf(MW_LogLevel logLevelIndex, const char* file, int line, const char *format, ...) __attribute__ ((format (printf, 4, 5))); | ||
| extern void logprintf(MW_LogLevel logLevelIndex, const char* file, const char* func, int line, const char *format, ...) __attribute__ ((format (printf, 5, 6))); | ||
|
|
||
| #define MW_CLI_TIMESTAMP_PREFIX_MAX_CHARS 20 | ||
| #define MW_CLI_TIMESTAMP_PREFIX_FORMAT "%u.%03u: " | ||
|
|
@@ -140,7 +140,7 @@ extern void logprintf(MW_LogLevel logLevelIndex, const char* file, int line, con | |
| do{\ | ||
| if( (LEVEL) >= PlayerLogManager::mwLoglevel ) \ | ||
| { \ | ||
| logprintf( LEVEL, __FUNCTION__, __LINE__, FORMAT, ##__VA_ARGS__); \ | ||
| logprintf( LEVEL, __FILE__, __FUNCTION__, __LINE__, FORMAT, ##__VA_ARGS__); \ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do we really need FILE included with every log? including also FILE will add many additional characters to existing logs, and inflate library size. |
||
| }\ | ||
| }while(0) | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we really need FILE included with every log?
In practice, FUNCTION is already mostly unique
including also FILE will add many additional characters to existing logs, and inflate library size.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FILE is not added to each log line.
Initially I did add FILE to each log line, but @cpc005 pointed out the same objections, so I removed it. However, I kept the parameter in logprintf() function (even though it is not printed) for a couple of reasons:
file, even though it was used to print the function name.