Forwards logs to the server if server supports it #361
Conversation
1. Adds a new class LogQueue which stores up to a fixed number of log messages in a circular buffer. 2. Whenever Logger is called, the log message is pushed into the LogQueue. 3. Whenever the Connection loop is run, it will try to send one pending message to the server. Note that sending logs is best-effort since it's over UDP.
src/logging/LogQueue.h
Outdated
| else | ||
| { | ||
| // Overwrite the last message | ||
| setMessageAt(m_Count - 1, OverflowMessage); |
There was a problem hiding this comment.
Would probably be a good idea to consider if overriding the first messages would make more sense in this context (tho honestly will never happen)
There was a problem hiding this comment.
You're probably right, it's better to keep the newest logs rather than the oldest logs.
There was a problem hiding this comment.
Do you think it's even necessary to send an [OVERFLOW] message? I'm worried once we get to overflow territory, every message we send to the server will just be [OVERFLOW].
Maybe if we can't keep up, we just silently drop (the oldest) messages?
There was a problem hiding this comment.
Addressed this by silently dropping earliest message.
There was a problem hiding this comment.
I don't think an overflow message would be particularly useful, and in theory it would override another message, would it not?
| void setMessageAt(int offset, const char* message); | ||
|
|
||
| static constexpr size_t MaxMessages = 100; | ||
| static constexpr size_t MaxMessageLength = std::numeric_limits<uint8_t>::max(); |
There was a problem hiding this comment.
I'd have this defined as a publicly available constexpr variable inside logger or something, that way it isn't duplicated in multiple cases of the codebase.
There was a problem hiding this comment.
It doesn't really feel duplicated. This constant here limits the size of the LogQueue. The check I added in Connections::sendShortString limits the string that can be sent on that wire format.
It feels weird for Connections to reference logging size, or for LogQueue to reference wire format size.
There was a problem hiding this comment.
E.g. If we wanted to send longer messages, then we'd have to switch to Connections::sendLongString, which has a very different limit. We wouldn't use std::numeric_limits<ulong_t>::max() for our MaxMessageLength.
src/logging/LogQueue.cpp
Outdated
| else | ||
| { | ||
| // Overwrite the last message | ||
| setMessageAt(m_Count - 1, OverflowMessage); |
There was a problem hiding this comment.
It's probably worth considering, if maybe overriding the oldest message would make more sense in this context (though it will never happen hopefully).
There was a problem hiding this comment.
Addressed this by silently dropping earliest message.
src/logging/LogQueue.h
Outdated
| static constexpr char OverflowMessage[] = "[OVERFLOW]"; | ||
|
|
||
| template <typename T, T Modulus> | ||
| class Modulo { |
There was a problem hiding this comment.
Modulo should probably move out of the logging namespace and into something more fitting for it.
There was a problem hiding this comment.
It's a private class at the moment. I didn't want to move it out because it doesn't support all the necessary functions, nor does it take care of general edge cases.
There was a problem hiding this comment.
I think I'll just get rid of the helper class and move the modulo logic into the LogQueue implementation. I was being too fancy.
There was a problem hiding this comment.
Addressed this by dropping the Modulo helper class.
|
I reduced the max messages from 100 to 30 because I was seeing some OOM panics. Have not seen any more OOM since then, but I'm curious how the memory budget is allocated. |
Note that sending logs is best-effort since it's over UDP.
Related PRs:
SlimeVR/SolarXR-Protocol#153
SlimeVR/SlimeVR-Server#1234