Topic: Patch to add chat logging output

[Ashley]

Little known thing I provide for my servers is chat logging to a website:

http://entropy.me.uk/simutrans/server/sim64/2012/03/05.html

I do this by piping the stdout/stderr output of Simutrans through the Unix utility logger and into syslog. I then have a syslog rule which matches the chat log network messages and puts them into a separate log file. This log file is then parsed by a python script into a CSV format file, which is then finally processed into HTML by another script.

Following Dwachs' patch which adds the concept of Player nicknames these logging messages seem to have vanished.

I think I'd like to simplify my life a bit by simply having the game log this information directly in CSV format. This would be output to stdout, and have some unique tag so that syslog can parse the data out. I *don't* want Simutrans to log to a file itself (at least not without making it either log to syslog, or be capable of responding to a SIGHUP by re-opening its log files).

At present I am going to implement the following messages (CSV formatted):

Code: [Select]

Chat messages (type 'chat'): (done)
Type, ClientID, IPAddress, Player Name, Company ID, Company Name, Message

Player changed name message (type 'namechange'): (done)
Type, ClientID, IPAddress, Old Player Name, New Player Name

Player Connected (type 'connect'): (done)
Type, ClientID, IPAddress, Player Name

Player Disconnected (type 'disconnect'): (done)
Type, ClientID, IPAddress, Player Name

Admin message (type 'adminmsg') - sent via nettool: (done)
Type, Message

Private message (type 'private') - (whisper, sent from one client to another): (done)
Type, ClientID, IPAddress, Player Name (from), Company ID, Company Name, Player Name (to), Message

All will be prefixed in log output by the string: __ChatLog__ followed by the message, to give me something to look for. Timestamping can be easily provided by syslog so I'm not going to include a timestamp myself in the output.

Any suggestions as I work on this?

[Dwachs]

Following Dwachs' patch which adds the concept of Player nicknames these logging messages seem to have vanished.

What messages did you parse exactly? Maybe fixing this would be a lot easier.

[Ashley]

What messages did you parse exactly? Maybe fixing this would be a lot easier.

To be honest I've been meaning to do this for a while, and this breaking has given me the motivation. I previously parsed:

Code: [Select]

Warning: nwc_tool_t::rdwr:#011rdwr id=6 client=0 plnr=255 pos=koord3d invalid wkzid=8224 defpar=Right now 2 clients are connected. init=1 flags=0
Warning: nwc_tool_t::rdwr:#011rdwr id=6 client=0 plnr=10 pos=260,177,0 wkzid=8224 defpar=ah ok init=1 flags=0

But to be honest this isn't a good approach as it is too fragile, for example recently the format of these messages broke, which broke the regular expression I used to parse them.

[Dwachs]

The only difference is that now the nickname is part of the message enclosed by [] brackets, followed by a linebreak, and then the regular message:

Code: [Select]

Warning: nwc_tool_t::rdwr:      rdwr id=7 client=2 plnr=0 pos=44,44,-1 wkzid=8224 defpar=[Client#2]
Hello world init=1 flags=0


[Ashley]

Oh I see it, missed that. I think there's still merit in improving this. E.g. at present there's no way to get information on name changes, switching between companies etc. (These things should go into the in-game chat log as well IMO).

[Dwachs]

Nickname changes are communicated.

Do you mean change of company names?

[Ashley]

Nickname changes, Company name changes, players switching between companies and player joining/leaving are I think the key information.

[Ashley]

Would it be a good idea perhaps to split the chat messages out from the general melange of NWC_TOOL, and into their own class? I can't see a good place to hook into to extract the messages easily, and sending the chat messages as pre-formatted WKZ_ADD_MESSAGE_TOOL calls doesn't lend itself to flexibility in the chat interface at all (since the message comes pre-formatted, with player name etc.)

[Dwachs]

Yes, splitting messages into their own class would be not a bad idea.

[Ashley]

Ok, working on that now. I'm going to implement private messaging too, since it isn't much more work.

[prissi]

Maybe I am slow on the uptake: What do you mean by "own class"? We worked a hard time to have every tool accesible via the werkzeug_t; why should not messages use this? It seem to me that your request can be easily fullfilled with the right debug messages. But I may have overlooked something, of course ...

[Ashley]

Chat messages are IMO not a tool, they should have their own mechanism (and their own window).

So:

Code: [Select]

class nwc_chat_t : public network_command_t

Is what I mean by their own class.

IMO it makes it easier to implement in-game chat, since chat messages are somewhat dissimilar to tool actions.

[Ashley]

Here's a patch to demonstrate what I mean. Having chat messages in their own class permits much easier formatting of the text, and makes the chat system more extensible as we can easily add additional information fields.

The existing wkz_add_message_t can be kept for system messages which have a fixed format of course.

[Dwachs]

We worked a hard time to have every tool accesible via the werkzeug_t; why should not messages use this?

The tools are there for syncronized exection. Messages do not need be syncronized in the same way. Timothy's approach looks good. It also enables private in-game messaging.

[prissi]

I see what you mean by that. And you are still useing the werkzeug_t for the messages. I just did not understood this correctly then. No problem with new class then.

Though I am not too fond of printf; this may be wanted for servers, but is not wanted for clients. The dbg system is for logging purposes.

(About CSV, I have to mention that your are not producing valid CSV for any german windows, as this is seperated by a colon and have different number seperators. So it is not very universal anyway. I one dioes this right, one have to use the nubmer seperators and CSV seperators of the system ... )

[Ashley]

The ChatLog messages should only be printed by the server. There's no good reason for these to appear on the client.

Private messages present a problem actually. They shouldn't be saved on the server as part of the savegame (as most messages are) since then following a reload they would appear on all the clients. Thus they have the message_t::local_flag set. But this means that following a restart a client would also lose their own local chat message history.

(About CSV, I have to mention that your are not producing valid CSV for any german windows, as this is seperated by a colon and have different number seperators. So it is not very universal anyway. I one dioes this right, one have to use the nubmer seperators and CSV seperators of the system ... )

This is really beyond the scope of what I was trying to do with the CSV parser. I just wanted a simple data interchange format which could be used for various things (e.g. nettool data, chat logs, server listing, etc.) I don't really think not having localisation for the CSV data is terribly useful, a German user could just as easily parse comma-separated data as colon-separated... Simplicity is why I avoided using XML or JSON for this afterall.

[prissi]

Well, SImutrans had already XML support anyway for all the rdwr functions (one just have to open the respective file in XML mode). But that is surely not most important.

But why not saving messages with the savegame. Only the last 2000 messages will be saved anyway. That way even new joining clients can see old chat messages. This is a good thing for me.

[Ashley]

The "XML" support for savegames is probably better not mentioned

Normal chat messages, e.g. ones which are seen by all players are still saved with the savegame. I mean there is a problem with private messages sent to a particular client. If these are stored in the savegame then they get sent to every client which joins (which could be considered insecure) and you then need a way to work out which client should see them when loaded.

I'm not going to solve this problem now though. It can wait for when a chat window is implemented.


Edit: Got authentication working now, chat messages are redirected to PLAYER_UNOWNED if sent from an un-authenticated client. Also switched to using dbg->warning for the chatlog output.

[Ashley]

Here's a patch.

Fixes nettool build, adds chat message subclass of network_command_t, uses that network chat class for sending chat messages (including the nettool say command, which now appears to have been said by the special reserved client name Admin as the public service player).

Any connected client can chat, and can do so while active as any player. If someone tries to chat as a player they are not unlocked for then the messages get redirected to appear to come from the unowned player (no company name displayed against the client).

Also adds the __ChatLog__ messages which appear at debug level 2 or above, in CSV format.

(Support is there for sending private messages in-game, but I haven't added any UI for this yet).

[Dwachs]

There is a seemingly unrelated change to the Makefile for MACs... What is the purpose of that ? I will happily commit it, if I know what it changes/fixes etc

Edit:  using the CVS class looks like overkill. Why not using printf-like formatted output (which the dbg->warning function accepts anyway) ?

[Dwachs]

in r5574

[Ashley]

There is a seemingly unrelated change to the Makefile for MACs... What is the purpose of that ? I will happily commit it, if I know what it changes/fixes etc

That was accidental, I have to make those changes to the Makefile in order to compile the SDL version on the Mac.

Edit:  using the CVS class looks like overkill. Why not using printf-like formatted output (which the dbg->warning function accepts anyway) ?

I wrote the CSV class to parse/escape CSV - why would we not use it for something which is meant to be CSV output? E.g. if a user puts a comma, or quote mark into their chat message it needs to be properly escaped so that the CSV output can be easily parsed by other applications.

[Dwachs]

I was not aware of the encoding stuff in the csv class when I asked the question.

[Ashley]

Ah, yes, CSV has to encode commas by escaping the entire string in double quotes, and escape double quotes by doubling them (e.g. " -> ""), you can also escape newlines by enclosing the string in quotes as well. I wouldn't have bothered with a class for it if it was trivial

[prissi]

In case of message logging, the solution could be trivial: Messages cannot have spaces => just print the message text last: Everything behind this option until newline would be message ...

By the way, entering "," into the field crashed the server in a feeble first test in line 186 in csv.cc Even the trivial test I said "Hello" killed the server by an error in the same line. (corrupted heap)

EDIT: You used a buffer of one element too tille for tstrncpy. Anyway, the function you wanted to get is already there in C, strdup(). (Incidentally, also the function you use would be there strtok does more or less what you loop does too ...

[Ashley]

Ok, sorry for the bug, thanks for fixing it.