uWebSockets icon indicating copy to clipboard operation
uWebSockets copied to clipboard

add logging interface

Open jf99 opened this issue 4 years ago • 10 comments

Currently, there is just a single logging line in uWebSockets which writes to std::cerr. I added two more such lines. More importantly, I introduced a function object uWS::log that can be overridden by users to accord with their logging library. For example, spdlog users might want to write something like this:

uWS::log = [](const std::string& message, const int logLevel) -> void {
    switch(logLevel) {
        case 0:
            SPDLOG_ERROR(message);
            break;
        case 1:
            SPDLOG_WARN(message);
            break;
        case 2:
            SPDLOG_INFO(message);
            break;
        case 3:
            SPDLOG_DEBUG(message);
    }
};

Nothing changes for users who do not configure anything.

jf99 avatar Jan 06 '21 12:01 jf99

Problem is that this PR introduces major performance regressions. Logging must not be added with a cost

ghost avatar Jan 06 '21 16:01 ghost

How about putting a macro around logging like this?

#if UWS_LOG_LEVEL <= 1
log("No handler for route " + std::string(httpRequest->getMethod()) + " " + std::string(httpRequest->getUrl()), 1);
#endif

The default value of UWS_LOG_LEVEL would be 0. That would let the user decide on the trade-off between performance and debuggability. spdlog does something similar.

jf99 avatar Jan 07 '21 07:01 jf99

Macro would work, but I don't want to put a bunch of text in the code - I would rather have it like so:

One header file contains all text -> you can either include it before App.h and then get logging enabled (or, let's say have WITH_LOG=1 build flag or something).

In the header you have macros like UWS_LOG_REQUEST(req) as functions, so that the calling (source code) remains clutter-free.

But it doesn't have to be C-style macros, you can use constexpr C++17 functions I mean you can have these "macros" as regular C++ code, but have an if constxpr (loglevel < 1) {

}

ghost avatar Jan 08 '21 20:01 ghost

Also, the logging has to be faster than just concatenating strings like that - use a pre-allocated array and memcpy or similar. One pre-allocated array per thread so mark it thread_local

ghost avatar Jan 08 '21 20:01 ghost

I have changed the code to fulfill your requirements:

  • String concatenations are as efficient as possible
  • One pre-allocated log buffer per thread
  • No function call is made at all if the verbosity does not allow for messages of that loglevel
  • Using the logger is a one-liner

It seemed overly complicated to me, but hey! My first real world problem solved with variadic templates.

I also made use of if constexpr. However a macro is still required, as parameters cannot be constexpr in C++17 and we need logLevel as constexpr. The only way around this would be a template argument, I think, but I don't see how this is cleaner.

One header file contains all text

Not sure if I understand you correctly, here. You want to move all log strings into a separate file like this?

const std::string noHandlerForRoute = "No handler for route ";
const std::string space = " ";
// ...
const std::string returningWithoutResponding = "Error: Returning from a request handler without responding or attaching an abort handler is forbidden!";

How would that be better?

jf99 avatar Jan 11 '21 15:01 jf99

@alexhultman Could you please leave a comment on the changes that I've made? Thanks.

jf99 avatar Jan 22 '21 07:01 jf99

#1132

ghost avatar Jan 22 '21 11:01 ghost

I added some more logging, including the case when us_create_socket_context fails. However, we cannot log any SSL specific stuff here, as this is handled by uSockets.

jf99 avatar Jan 25 '21 13:01 jf99

Then maybe logging should be part of uSockets

ghost avatar Jan 26 '21 11:01 ghost

@alexhultman Do we still need logging in uSockets, now that you added an error handler for SSL? And if yes, does it have to be C? If it's not C++, I'm out.

I might have some time in the next days.

jf99 avatar Apr 13 '21 13:04 jf99