log moderation stats in the uptime table #3215

ZeldaZach · 2018-04-25T21:40:55Z

Update the server stats logging to include mods / which mods are logged in. I find it useful as a server operator to know who on my staff is around.

ctrlaltca

The build failed because you need to update the database version in https://github.com/Cockatrice/Cockatrice/blob/master/servatrice/src/servatrice_database_interface.h#L12

While i like the c++11-ization of the code, reviewing the main change in the middle of a lot of unrelated changes is harder. I'll try to sum up them here to ease the review:

use of the auto specifier where possible
change for loops from index-based to range-based (aka "colon")
add explicit static_casts where an implicit conversion was previously used, mostly to convert to google::protobuf types
add the const attribute to class methods where applicable
rename variables to better express their content (eg. from "query" to "forgotPwQuery")

I don't see a good point for a few minor changes: see inline comments.

ctrlaltca · 2018-04-26T06:46:39Z

servatrice/src/servatrice.cpp

-        "insert into {prefix}_uptime (id_server, timest, uptime, users_count, games_count, tx_bytes, rx_bytes) "
-        "values(:id, NOW(), :uptime, :users_count, :games_count, :tx, :rx)");
+        "insert into {prefix}_uptime (id_server, timest, uptime, users_count, mods_count, mods_list, games_count, "
+        "tx_bytes, "


line splitting looks ugly, is this due to clang-format?

Yeah... I'll re-do this

ctrlaltca · 2018-04-26T07:03:24Z

common/server.h

-    void addExternalUser(const ServerInfo_User &userInfo);
-    void removeExternalUser(const QString &userName);
+    virtual void addExternalUser(const ServerInfo_User &userInfo) = delete;
+    virtual void removeExternalUser(const QString &userName) = delete;


These looks like leftovers as they have been implemented in Server_Room, maybe they can just be deleted.

ctrlaltca · 2018-04-26T07:04:03Z

common/server.cpp

-Server::~Server()
-{
-}
+Server::~Server() = default;


Can this be removed and = default added in the .h?

ctrlaltca · 2018-04-26T07:07:24Z

servatrice/migrations/servatrice_0024_to_0025.sql

+-- Servatrice db migration from version 24 to version 25
+
+ALTER TABLE `cockatrice_uptime` ADD COLUMN `mods_count` int(11) NOT NULL DEFAULT 0;
+ALTER TABLE `cockatrice_uptime` ADD COLUMN `mods_list` BLOB;


If you are storing text and not binary data, TEXT is a better choice.

Never knew the diff, thanks!

ctrlaltca · 2018-04-26T07:10:26Z

servatrice/src/servatrice.cpp

@@ -74,11 +74,11 @@ Servatrice_GameServer::~Servatrice_GameServer()
    }
 }

-void Servatrice_GameServer::incomingConnection(qintptr socketDescriptor)
+void Servatrice_GameServer::incomingConnection(const int &socketDescriptor)


The base Qt class is declaring a qintptr argument (http://doc.qt.io/qt-5/qtcpserver.html#incomingConnection), i see no reason to change this.

I didn't change this, which was weird... Clion didn't like the qintptr :P Will readdress this

ctrlaltca · 2018-04-26T07:25:26Z

servatrice/src/servatrice.cpp

-            result.append(static_cast<AbstractServerSocketInterface *>(clients[i]));
+    for (auto client : clients)
+        if (dynamic_cast<AbstractServerSocketInterface *>(client)->getPeerAddress() == address)
+            result.append(dynamic_cast<AbstractServerSocketInterface *>(client));


A client is a Server_ProtocolHandler *, and theAbstractServerSocketInterface is a subclass of Server_ProtocolHandler.
Since we can guarantee that the down-cast is safe, static_cast is a better choice.
dynamic_cast could be a safer choice if we are not sure about the type of client, but still we should check the cast result before dereferencing it.
Also, dynamic_cast has a runtime performance cost.

CLion didn't agree with this being static for some reason, but I'll listen

ctrlaltca

You need to fix the version number in the sql file, then it should be fine

ZeldaZach · 2018-04-26T20:10:10Z

Done!

ctrlaltca · 2018-04-26T20:17:05Z

servatrice/migrations/servatrice_0024_to_0025.sql

+ALTER TABLE `cockatrice_uptime` ADD COLUMN `mods_count` int(11) NOT NULL DEFAULT 0;
+ALTER TABLE `cockatrice_uptime` ADD COLUMN `mods_list` TEXT;
+
+UPDATE `cockatrice_schema_version` SET `version` = 25 WHERE `version` = 24;


The build script is checking for an exact string here; the check fails because the table and column names are surrounded by ` quotes.
Quick fix: remove them

This has been done with other sql migrations... oh well

ctrlaltca · 2018-04-26T20:18:35Z

common/server.h

-    Server(QObject *parent = 0);
-    ~Server();
+    Server(QObject *parent = nullptr);
+    Server::~Server() = default;


The Server:: qualifier is not needed here

ctrlaltca · 2018-04-27T13:28:48Z

servatrice/migrations/servatrice_0024_to_0025.sql

+ALTER TABLE cockatrice_uptime ADD COLUMN mods_count int(11) NOT NULL DEFAULT 0;
+ALTER TABLE cockatrice_uptime ADD COLUMN mods_list TEXT;
+
+UPDATE cockatrice_schema_version SET version = 25 WHERE version = 24;


The checker script expects the exact string:

UPDATE cockatrice_schema_version SET version=25 WHERE version=24;

Please remove spaces around the equal signs, we'll fix the script later.

ZeldaZach · 2018-05-02T20:06:54Z

@ctrlaltca I'm not sure why linux is failing... the function join() was introduced in qt4 so it should be in qt5.2.1...

ctrlaltca · 2018-05-02T20:08:40Z

servatrice/src/servatrice.cpp

@@ -566,6 +565,11 @@ void Servatrice::statusUpdate()
        return;

    const int uc = getUsersCount(); // for correct mutex locking order
+
+    const QList<QString> mods_info = getOnlineModeratorList();


use QStringList instead of QList<QString>, add the #include if needed

Seems silly as they should be equiv... but w/e i'll get that fixed

From https://doc.qt.io/qt-5/qlist.html :

Qt includes a QStringList class that inherits QList and adds a few convenience functions, such as QStringList::join() and QStringList::filter(). QString::split() creates QStringLists from strings.

Seems like they're equiv.. except for split(), join() and filter().

log moderation stats in the uptime table

5d53942

ctrlaltca requested changes Apr 26, 2018

View reviewed changes

ZeldaZach added 2 commits April 26, 2018 15:10

fixes

a6b1dfc

default sql

095c423

ctrlaltca approved these changes Apr 26, 2018

View reviewed changes

Update servatrice.sql

0412c75

ctrlaltca reviewed Apr 26, 2018

View reviewed changes

Update servatrice_0024_to_0025.sql

e6998b1

ctrlaltca reviewed Apr 26, 2018

View reviewed changes

Update server.h

7af2c61

ctrlaltca reviewed Apr 27, 2018

View reviewed changes

Update servatrice_0024_to_0025.sql

b79530d

ctrlaltca reviewed May 2, 2018

View reviewed changes

QStringList

bfeefeb

ZeldaZach merged commit 2520d07 into Cockatrice:master May 2, 2018

ZeldaZach deleted the log_mod_stats branch May 2, 2018 21:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

log moderation stats in the uptime table #3215

log moderation stats in the uptime table #3215

ZeldaZach commented Apr 25, 2018

ctrlaltca left a comment •

edited

Loading

ctrlaltca Apr 26, 2018

ZeldaZach Apr 26, 2018

ctrlaltca Apr 26, 2018

ZeldaZach Apr 26, 2018

ctrlaltca Apr 26, 2018

ctrlaltca Apr 26, 2018

ZeldaZach Apr 26, 2018

ctrlaltca Apr 26, 2018

ZeldaZach Apr 26, 2018

ctrlaltca Apr 26, 2018

ZeldaZach Apr 26, 2018

ctrlaltca left a comment •

edited

Loading

ZeldaZach commented Apr 26, 2018

ctrlaltca Apr 26, 2018

ZeldaZach Apr 26, 2018

ctrlaltca Apr 26, 2018

ctrlaltca Apr 27, 2018

ZeldaZach commented May 2, 2018

ctrlaltca May 2, 2018

ZeldaZach May 2, 2018

ctrlaltca May 2, 2018 •

edited

Loading

log moderation stats in the uptime table #3215

log moderation stats in the uptime table #3215

Conversation

ZeldaZach commented Apr 25, 2018

ctrlaltca left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ctrlaltca left a comment • edited Loading

Choose a reason for hiding this comment

ZeldaZach commented Apr 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZeldaZach commented May 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ctrlaltca May 2, 2018 • edited Loading

Choose a reason for hiding this comment

ctrlaltca left a comment •

edited

Loading

ctrlaltca left a comment •

edited

Loading

ctrlaltca May 2, 2018 •

edited

Loading