Skip to content

Commit

Permalink
counter ridiculous infolog spam
Browse files Browse the repository at this point in the history
consecutive duplicated messages are now
filtered out before they enter any sink
  • Loading branch information
rtri committed Mar 13, 2017
1 parent 8cda0f2 commit 85a036c
Show file tree
Hide file tree
Showing 18 changed files with 162 additions and 181 deletions.
2 changes: 1 addition & 1 deletion rts/ExternalAI/SAIInterfaceCallbackImpl.cpp
Expand Up @@ -174,7 +174,7 @@ EXPORT(void) aiInterfaceCallback_Log_log(int interfaceId, const char* const msg)
EXPORT(void) aiInterfaceCallback_Log_logsl(int interfaceId, const char* section, int loglevel, const char* const msg) {
CHECK_INTERFACE_ID(interfaceId);

log_frontend_record(section, loglevel, "%s", msg);
log_frontend_record(loglevel, section, "%s", msg);
}


Expand Down
3 changes: 1 addition & 2 deletions rts/Game/UI/InfoConsole.cpp
Expand Up @@ -165,8 +165,7 @@ int CInfoConsole::GetRawLines(std::deque<RawLine>& lines)
}


void CInfoConsole::RecordLogMessage(const std::string& section, int level,
const std::string& text)
void CInfoConsole::RecordLogMessage(int level, const std::string& section, const std::string& text)
{
std::lock_guard<spring::recursive_mutex> scoped_lock(infoConsoleMutex);

Expand Down
3 changes: 1 addition & 2 deletions rts/Game/UI/InfoConsole.h
Expand Up @@ -24,8 +24,7 @@ class CInfoConsole: public CInputReceiver, public CEventClient, public ILogSink
void Draw();
void PushNewLinesToEventHandler();

void RecordLogMessage(const std::string& section, int level,
const std::string& text);
void RecordLogMessage(int level, const std::string& section, const std::string& text) override;

bool WantsEvent(const std::string& eventName) {
return (eventName == "LastMessagePosition");
Expand Down
2 changes: 1 addition & 1 deletion rts/Lua/LuaUnsyncedCtrl.cpp
Expand Up @@ -2896,7 +2896,7 @@ int LuaUnsyncedCtrl::SetLogSectionFilterLevel(lua_State* L) {
}
}

log_frontend_register_runtime_section(luaL_checkstring(L, 1), logLevel);
log_frontend_register_runtime_section(logLevel, luaL_checkstring(L, 1));
return 0;
}

Expand Down
63 changes: 41 additions & 22 deletions rts/System/Log/Backend.cpp
@@ -1,12 +1,13 @@
/* This file is part of the Spring engine (GPL v2 or later), see LICENSE.html */

#include "Backend.h"

#include <cstdio>
#include <cstdarg>
#include <cstring>
#include <set>


#include "Backend.h"
#include "LogUtil.h"
#include "System/maindefines.h"


namespace {
Expand All @@ -21,50 +22,65 @@ namespace {
}
}


#ifdef __cplusplus
extern "C" {
#endif

extern char* log_formatter_format(const char* section, int level, const char* fmt, va_list arguments);

// note: no real point to TLS, sinks themselves are not thread-safe
static _threadlocal log_record_t cur_record = {NULL, "", "", 0, 0};
static _threadlocal log_record_t prv_record = {NULL, "", "", 0, 0};


extern void log_formatter_format(log_record_t* log, va_list arguments);

void log_backend_registerSink(log_sink_ptr sink) { log_formatter_getSinks().insert(sink); }
void log_backend_unregisterSink(log_sink_ptr sink) { log_formatter_getSinks().erase(sink); }

void log_backend_registerCleanup(log_cleanup_ptr cleanupFunc) { log_formatter_getCleanupFuncs().insert(cleanupFunc); }
void log_backend_unregisterCleanup(log_cleanup_ptr cleanupFunc) { log_formatter_getCleanupFuncs().erase(cleanupFunc); }


/**
* @name logging_backend
* ILog.h backend implementation.
*/
///@{

/// Eventually formats and routes the record to all sinks
void log_backend_record(const char* section, int level, const char* fmt, va_list arguments)
// formats and routes the record to all sinks
void log_backend_record(int level, const char* section, const char* fmt, va_list arguments)
{
const std::set<log_sink_ptr>& sinks = log_formatter_getSinks();

if (sinks.empty()) {
static bool warned = false;
if (sinks.empty())
return;

cur_record.sec = section;
cur_record.fmt = fmt;
cur_record.lvl = level;

if (!warned) {
warned = true;
// format the record
log_formatter_format(&cur_record, arguments);

fprintf(stderr,
"\nWARNING: A log message was recorded, but no sink is registered."
"\n (there will be no further warnings)\n\n");
}
} else {
// format the record
const char* record = log_formatter_format(section, level, fmt, arguments);
// check for duplicates after formatting; can not be
// done in log_frontend_record or log_filter_record
if (prv_record.msg != NULL && strcmp(cur_record.msg, prv_record.msg) == 0)
return;

// sink the record into each registered sink
for (auto si = sinks.begin(); si != sinks.end(); ++si) {
(*si)(section, level, record);
}
// sink the record into each registered sink
for (auto si = sinks.begin(); si != sinks.end(); ++si) {
(*si)(level, section, cur_record.msg);
}

delete[] record;
// reallocate buffer for copy if current is larger
if (cur_record.len > prv_record.len) {
delete[] prv_record.msg;
prv_record.msg = NULL;
prv_record.msg = new char[prv_record.len = cur_record.len];
}

strncpy(prv_record.msg, cur_record.msg, cur_record.len);
}

/// Passes on a cleanup request to all sinks
Expand All @@ -74,6 +90,9 @@ void log_backend_cleanup() {
for (auto si = cleanupFuncs.begin(); si != cleanupFuncs.end(); ++si) {
(*si)();
}

delete[] prv_record.msg; prv_record.msg = NULL; prv_record.len = 0;
delete[] cur_record.msg; cur_record.msg = NULL; cur_record.len = 0;
}

///@}
Expand Down
3 changes: 1 addition & 2 deletions rts/System/Log/Backend.h
Expand Up @@ -18,8 +18,7 @@ extern "C" {
*/
///@{

typedef void (*log_sink_ptr)(const char* section, int level,
const char* record);
typedef void (*log_sink_ptr)(int level, const char* section, const char* record);

/// Start routing log records to the supplied sink
void log_backend_registerSink(log_sink_ptr sink);
Expand Down
3 changes: 1 addition & 2 deletions rts/System/Log/ConsoleSink.cpp
Expand Up @@ -34,8 +34,7 @@ void log_console_colorizedOutput(bool enable) {


/// Records a log entry
static void log_sink_record_console(const char* section, int level,
const char* record)
static void log_sink_record_console(int level, const char* section, const char* record)
{
char framePrefix[128] = {'\0'};
log_framePrefixer_createPrefix(framePrefix, sizeof(framePrefix));
Expand Down
22 changes: 11 additions & 11 deletions rts/System/Log/DefaultFilter.cpp
Expand Up @@ -24,10 +24,10 @@ extern "C" {
#endif


bool log_frontend_isEnabled(const char* section, int level);
bool log_frontend_isEnabled(int level, const char* section);


extern void log_backend_record(const char* section, int level, const char* fmt, va_list arguments);
extern void log_backend_record(int level, const char* section, const char* fmt, va_list arguments);
extern void log_backend_cleanup();


Expand Down Expand Up @@ -128,7 +128,7 @@ int log_filter_section_getMinLevel(const char* section)

// called by LogOutput for each ENABLED section
// also by LuaUnsyncedCtrl::SetLogSectionFilterLevel
void log_filter_section_setMinLevel(const char* section, int level)
void log_filter_section_setMinLevel(int level, const char* section)
{
log_filter_checkCompileTimeMinLevel(level);

Expand Down Expand Up @@ -199,16 +199,16 @@ const char* log_filter_section_getRegisteredIndex(int index) {
return (*si);
}

static void log_filter_record(const char* section, int level, const char* fmt, va_list arguments)
static void log_filter_record(int level, const char* section, const char* fmt, va_list arguments)
{
assert(level > LOG_LEVEL_ALL);
assert(level < LOG_LEVEL_NONE);

if (!log_frontend_isEnabled(section, level))
if (!log_frontend_isEnabled(level, section))
return;

// format (and later store) the log record
log_backend_record(section, level, fmt, arguments);
log_backend_record(level, section, fmt, arguments);
}


Expand All @@ -218,7 +218,7 @@ static void log_filter_record(const char* section, int level, const char* fmt, v
*/
///@{

bool log_frontend_isEnabled(const char* section, int level) {
bool log_frontend_isEnabled(int level, const char* section) {
if (level < log_filter_global_getMinLevel())
return false;
if (level < log_filter_section_getMinLevel(section))
Expand All @@ -239,23 +239,23 @@ void log_frontend_register_section(const char* section) {
}
}

void log_frontend_register_runtime_section(const char* section_cstr_tmp, int level) {
void log_frontend_register_runtime_section(int level, const char* section_cstr_tmp) {
const char* section_cstr = log_filter_section_getSectionCString(section_cstr_tmp);

log_frontend_register_section(section_cstr);
log_filter_section_setMinLevel(section_cstr, level);
log_filter_section_setMinLevel(level, section_cstr);

}

void log_frontend_record(const char* section, int level, const char* fmt, ...)
void log_frontend_record(int level, const char* section, const char* fmt, ...)
{
assert(level > LOG_LEVEL_ALL);
assert(level < LOG_LEVEL_NONE);

// pass the log record on to the filter
va_list arguments;
va_start(arguments, fmt);
log_filter_record(section, level, fmt, arguments);
log_filter_record(level, section, fmt, arguments);
va_end(arguments);
}

Expand Down
2 changes: 1 addition & 1 deletion rts/System/Log/DefaultFilter.h
Expand Up @@ -61,7 +61,7 @@ int log_filter_global_getMinLevel();
* CAUTION: you may only use strings defined at compile-time.
* @see #log_filter_section_getMinLevel
*/
void log_filter_section_setMinLevel(const char* section, int level);
void log_filter_section_setMinLevel(int level, const char* section);

/**
* Returns the minimum level to log for a certain section.
Expand Down

13 comments on commit 85a036c

@ashdnazg
Copy link
Member

Choose a reason for hiding this comment

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

what?
You sometimes want consecutive identical messages for debugging!

@rtri
Copy link
Contributor

@rtri rtri commented on 85a036c Mar 13, 2017

Choose a reason for hiding this comment

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

I don't, and can not recall the last time it was ever useful to see the same message repeated 10 billion times.

It might be possible to insert a repeat-counter into the output though.

@ashdnazg
Copy link
Member

Choose a reason for hiding this comment

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

I do. 10 billion is indeed too much but for lower numbers it is useful since infolog contains the frame number.
Please don't make my debugging workflow harder, you'd agree that it's more important than some log spam.

@jamerlan
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe its possible to add "debug mode" switch?

@GoogleFrog
Copy link
Contributor

Choose a reason for hiding this comment

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

This is really bad. It hides bugs.

@ashdnazg
Copy link
Member

Choose a reason for hiding this comment

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

@rtri I implore you to revert this

@rtri
Copy link
Contributor

@rtri rtri commented on 85a036c Mar 14, 2017

Choose a reason for hiding this comment

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

A config option is in the works.

Personally, I'm not wasting my time anymore looking at unreadable spam-filled infologs.

@ashdnazg
Copy link
Member

Choose a reason for hiding this comment

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

@rtri agreed. This will be solved by forcing the games to spam less, not by hard coding it in the engine

@rtri
Copy link
Contributor

@rtri rtri commented on 85a036c Mar 14, 2017

Choose a reason for hiding this comment

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

sure, good luck with that.

@gajop
Copy link
Member

@gajop gajop commented on 85a036c Mar 14, 2017

Choose a reason for hiding this comment

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

Filtering duplicate consecutive messages probably won't help you much with Chobby+ZK infologs, who are large because they include server communication and often multiple game infologs concatanted into one huge monstrosity :p

@GoogleFrog
Copy link
Contributor

Choose a reason for hiding this comment

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

The spam is rarely duplicate messages. When it is duplicate then it is important to know.

@rtri
Copy link
Contributor

@rtri rtri commented on 85a036c Mar 14, 2017

Choose a reason for hiding this comment

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

You should perhaps also appreciate the importance of keeping infologs sane for the few people who still deal with them.

Not every conceivable piece of information-that-might-be-interesting-to-know actually needs to be logged.

@ashdnazg
Copy link
Member

Choose a reason for hiding this comment

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

I believe lobby logging should be moved into a separate file. This will solve most of the issue and everyone will be happy.

Please sign in to comment.