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

replace ClientThreadHandler "Debug" logs with NonSessionLog #830

Merged
merged 7 commits into from
Jun 26, 2024

Conversation

gbirchmeier
Copy link
Member

@gbirchmeier gbirchmeier commented Feb 8, 2024

A long writeup of what I did to rewrite the Debug logging

Preface

I'm not super happy with how this came out, but it could be worse. I wish it was more elegant, but this is the best I could figure out.

The situation

The entire QF/n log mechanism is session-based. To log a message, the code block needs to get the log handle from a reference to the session. Unfortunately, there are a few spots where we want to log, but the block of code is not directly associated with a session. There just isn't a natural method for logging in this situation, and never has been.

What had existed

For over a decade, we had an ugly hack in there that would write a "ClientHandlerThread-....-log" file every time a ClientHandlerThread was instantiated. It was originally added to help debug some socket setup stuff, but was never removed or improved. As the project grew, a few other places used it as well. In mostly only triggered for Acceptors, and after a few weeks of operation, developers of such apps would find hundreds of ClientHandlerThread logs in their log directory. It was horrible, and lots of folks rightly hated it.

My goal

I wanted to add a non-session-specific log that would:

  • not require additional configuration
  • not create files when nothing is logged
  • leverage the LogFactory and config that users are already using

What I came up with

I created a NonSessionLog class which takes the app's instantiated ILogFactory in its constructor, so that it can use the settings of that factory and write to the same place. Unfortunately, I had to extend the ILogFactory interface by adding method CreateNonSessionLog(), so anyone with a custom LogFactory will need to implement that method... but luckily that's not very difficult (see FileLogFactory for a good example)!

The commit that specifically adds the NonSessionLog is e9abea3.

There are some refactoring/cleanup commits in this PR also, but they are standalone.

A design crossroads

To implement NonSessionLog, I waffled over whether:

  • to pass an variable reference through a chain of constructors, or
  • to implement a static class so I could avoid that.

I ended up going with the former, which, while more annoying to write, seemed like the more-maintainable/less-"surprising" way to go.

Unfortunate side-effects (i.e. what I hate)

  • I had to extend ILogFactory (see above)
  • When using a FileLogFactory, NonSessionLog only needs to write to Non-Session-Log.events.log, but when it does, it will create an always-empty Non-Session-Log.messages.log also. I couldn't change this without further altering the ILog/ILogFactory interfaces. (I may revisit this in the future.)

Old comment that I wrote after the first commit (07d2d89):

This is phase 1: make it stop creating the Debug
log, and just generally getting my head around
the flow of the deep transport logic that writes
to this log.

(Current debug-log calls are either eliminated
or temporarily routed to console-writes)

The reason these log lines are weird is because
they happen in code that isn't specific to a
session, so it can't follow the existing paradigm
(where each session has its own logger).

But some of those logs are still valuable,
so phase 2 will be logging them in an alternate
way, likely in a way that will only create
the logs when absolutely necessary (as opposed
to the current impl which can create a ton
of empty logs).

@gbirchmeier
Copy link
Member Author

gbirchmeier commented Feb 13, 2024

SocketInitiator.cs has multiple places where a session-independent log should go:

  • SocketInitiatorThreadStart() (currently a File.AppendAllText() hack)
  • OnStart() (currently a File.AppendAllText() hack)
  • OnConfigure() an empty catch-block could use a log

This is phase 1: make it stop creating the Debug
log, and just generally getting my head around
the flow of the deep transport logic that writes
to this log.

(Current debug-log calls are either eliminated
or temporarily routed to console-writes)

The reason these log lines are weird is because
they happen in code that isn't specific to a
session, so it can't follow the existing paradigm
(where each session has its own logger).

But some of those logs are still valuable,
so phase 2 will be logging them in an alternate
way, likely in a way that will only create
the logs when absolutely necessary (as opposed
to the current impl which can create a ton
of empty logs).
no longer needed after I renamed
QuickFix.Dictionary to QuickFix.SettingsDictionary

This commit makes no other changes to these files
@gbirchmeier gbirchmeier force-pushed the xdebuglog branch 2 times, most recently from 6c72c53 to 0bad6ed Compare June 10, 2024 21:10
@gbirchmeier gbirchmeier changed the title remove Debug log (an alt handling is yet to come) replace DebugLog with NonSessionLog (also I broke the SSL connectivity but I'll fix it soon) Jun 10, 2024
@gbirchmeier gbirchmeier changed the title replace DebugLog with NonSessionLog (also I broke the SSL connectivity but I'll fix it soon) replace ClientThreadHandler "Debug" logs with NonSessionLog (also I broke the SSL connectivity but I'll fix it soon) Jun 10, 2024
@gbirchmeier
Copy link
Member Author

gbirchmeier commented Jun 10, 2024

resolves #684
resolves #736

Initialized and passed through the stream/socket
hierarchy so that logging can be performed
for messages that cannot be tied to a session
@gbirchmeier
Copy link
Member Author

Hey @mgatny , I don't know if you'd be interested in looking this over. It's kind of overwhelming.

@gbirchmeier gbirchmeier marked this pull request as ready for review June 10, 2024 23:01
gbirchmeier added a commit to gbirchmeier/quickfixn that referenced this pull request Jun 10, 2024
@gbirchmeier gbirchmeier changed the title replace ClientThreadHandler "Debug" logs with NonSessionLog (also I broke the SSL connectivity but I'll fix it soon) replace ClientThreadHandler "Debug" logs with NonSessionLog Jun 26, 2024
@gbirchmeier gbirchmeier merged commit de1898f into connamara:master Jun 26, 2024
2 checks passed
@gbirchmeier gbirchmeier deleted the xdebuglog branch June 26, 2024 18:43
gbirchmeier added a commit to gbirchmeier/quickfixn that referenced this pull request Jun 27, 2024
Should have been in PR connamara#830, I missed it
hlibman-connamara pushed a commit to hlibman-connamara/quickfixn that referenced this pull request Dec 6, 2024
hlibman-connamara pushed a commit to hlibman-connamara/quickfixn that referenced this pull request Dec 6, 2024
replace ClientThreadHandler "Debug" logs with NonSessionLog
hlibman-connamara pushed a commit to hlibman-connamara/quickfixn that referenced this pull request Dec 6, 2024
Should have been in PR connamara#830, I missed it
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant