Server logging - #145
Conversation
- Add ConnectionTraceContext class for per-connection trace isolation - Update Tracer to support daemon mode tracing - Update DbWebsocketClient to initialize per-connection trace contexts - Fixes #136: server logging Key changes: - Each WebSocket connection now gets an isolated trace buffer - Trace data is automatically cleaned up on connection closure - Backward compatible with existing single-mode tracing - Thread-safe for multi-client scenarios - Resolves daemon mode logging limitations
ThePrez
left a comment
ThePrez
left a comment
There was a problem hiding this comment.
MAde some one-off comments throughout the code. To summarize, I have concerns about:
- whether we are truly honoring trace isolation between requests
- lack of clarity on global events vs. connection-level events
At a high level, a different approach would be to rework the main entry points and acquisition methods for the Tracer class. My thoughts:
Tracer.get()becomesTracer.getGlobalTracer()- new method
Tracer.getNew(String _connectionId)that is used within the SystemConnection constructor (or a similarly reasonable place). This returns aTracerobject specific for a connection. - Connection trace events are logged through non-static calls to the connection's
Tracerobject
Overall this may be cleaner and less likely to have unforeseen intermingling between trace contexts.
| } | ||
| } | ||
|
|
||
| private File getJtOpenFile() throws IOException { |
There was a problem hiding this comment.
same comments as on getFile()
There was a problem hiding this comment.
Incorporated the review comment.
| } | ||
|
|
||
| private static class InMemCache<T> { | ||
| public static class InMemCache<T> { |
There was a problem hiding this comment.
Perhaps a personal preference, but I don't like having static inner classes be public.
Since it's static, we could pull it out into its own part, make it package-private, and use from adjacent consuming classes
There was a problem hiding this comment.
Incorporated the review comment.
| super(); | ||
| SystemConnection conn = new SystemConnection(clientHost, clientAddress,host, user, pass); | ||
| // ✅ NEW: Generate unique connection ID | ||
| this.connectionId = UUID.randomUUID().toString(); |
There was a problem hiding this comment.
Please check the performance of this method. In the past, I have found randomUUID can take a long time (nearly a full second to complete). It may have gotten better since my distant memories were formed. I'm guessing this is the case, but please validate.
UUID is a great approach, but if it is problematic, perhaps an AtomicLong would do.
There was a problem hiding this comment.
Incorporated the review feedback.
| io = getDataStream(this, conn); | ||
|
|
||
| // ✅ NEW: Initialize per-connection trace context | ||
| Tracer.get().setConnectionId(connectionId); |
There was a problem hiding this comment.
This looks incorrectly handled.
get()returns a global static instance ofTracersetConnectionId()sets a member variable in that global instance- subsequent trace requests will use a context derived from the global instance's member var
- meaning that if another connection is initiated since the last time, we'd use the wrong trace context
(if I am mis-reading the code please explain)
Ideas for cleaner implementations would be:
Tracer.get()to take in the connection ID to get a Tracer object specific for that connection. Refactor accordingly.- (preferred) Initialize a
ConnectionTraceContextdirectly. It would probably be "owned" by theSystemConnectionclass. In other words, theSystemConnectionconstructor would generate its own connection ID (makes sense for the connection ID to exist inside that class anyway), then generate a ConnectionTraceContext and store as a member variable. This would remove the need to keep a map of all the trace contexts here
There was a problem hiding this comment.
Fixed, please review the latest commit.
| return s_instance; | ||
| } | ||
|
|
||
| public static void info(Object _data) { |
There was a problem hiding this comment.
Since get() is a static global instance, and there's nothing in this logic that delineates based on connection ID, I think this method should be named explicitly as a global thing. I'd propose globalInfo (and the same pattern would apply to peers (err, warn, etc)
| Entry entry = new Entry(_t, _data); | ||
|
|
||
| // ✅ NEW: Daemon mode - use per-connection context | ||
| if (!MapepireServer.isSingleMode() && m_connectionId != null) { |
There was a problem hiding this comment.
This needs to be reworked due to problems with m_connectionId
|
@ThePrez I have reworked and incorporated all your review feedback received, please review when you can and provide the feedback. Thanks |
|
@ThePrez can you please complete your review? |
3 participants
Added changes will enable robust logging and tracing for each client connection
when Mapepire Server operates in daemon mode (multi-client/production scenarios).