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

add logging interface #1163

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Conversation

jf99
Copy link

@jf99 jf99 commented Jan 6, 2021

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.

@ghost
Copy link

ghost commented Jan 6, 2021

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

@jf99
Copy link
Author

jf99 commented Jan 7, 2021

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.

@ghost
Copy link

ghost commented Jan 8, 2021

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
Copy link

ghost commented Jan 8, 2021

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

@jf99
Copy link
Author

jf99 commented Jan 11, 2021

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?

src/Log.h Outdated Show resolved Hide resolved
@jf99
Copy link
Author

jf99 commented Jan 22, 2021

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

@ghost
Copy link

ghost commented Jan 22, 2021

#1132

@ghost ghost added the v19 label Jan 23, 2021
@jf99
Copy link
Author

jf99 commented Jan 25, 2021

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.

@ghost
Copy link

ghost commented Jan 26, 2021

Then maybe logging should be part of uSockets

@jf99
Copy link
Author

jf99 commented Apr 13, 2021

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant