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

Add basic mysql logging support - WIP #506

Closed
wants to merge 12 commits into from

Conversation

willwh
Copy link

@willwh willwh commented Jul 15, 2016

Please add a do not merge label :)

Don't forget to npm install

There is a helper script at scripts/irclogs.sql that will create a database user, database, and a table for storing your logs. You should edit the password that is set in the script before running it.

You need to set the following in your config.js:

mysql: {
  enabled: true,
  database: 'database_name',
  dbuser: 'database_user',
  dbpass: 'database_password',
  dbhost: 'database_host',
  maxLogs: 100
}

If you haven't run the SQL helper script, you need to create a database, and then a table called logs like so (this will get moved to config):

create table logs (
    id INT(6) UNSIGNED AUTO_INCREMENT PRIMARY KEY,
    name VARCHAR(30) NOT NULL,
    host VARCHAR(64),
    channel VARCHAR(30),
    message VARCHAR(600),
    datetime TIMESTAMP
);

I chose VARCHAR(600) for messages, which is a bit bigger than the suggested max of an irc message defined in RFC 2812, which says:

IRC messages are always lines of characters terminated with a CR-LF (Carriage Return - Line Feed) pair, and these messages SHALL NOT exceed 512 characters in length, counting all characters including the trailing CR-LF. Thus, there are 510 characters maximum allowed for the command and its parameters.

Logs look like this:

mysql> select * from logs order by datetime desc limit 3;
+----+-------------+----------+-----------+---------------------------+---------------------+
| id | name        | host     | channel   | message                   | datetime            |
+----+-------------+----------+-----------+---------------------------+---------------------+
| 78 | lounge-user | Freenode | #bot_test | gfjhg                     | 2016-07-15 10:08:49 |
| 77 | lounge-user | Freenode | #bot_test | hi!                       | 2016-07-15 10:08:37 |
| 76 | lounge-user | Freenode | #bot_test | testing, testing, 1, 2, 5 | 2016-07-15 09:48:20 |
+----+-------------+----------+-----------+---------------------------+---------------------+

Would love feedback and help :)

@astorije astorije added Type: Feature Tickets that describe a desired feature or PRs that add them to the project. Meta: Do Not Merge This PR should not be merged. labels Jul 15, 2016
@spencerthayer
Copy link

This is awesome. Does it work?

@willwh
Copy link
Author

willwh commented Aug 15, 2016

All it does right now is actually log messages to the database, but that does work. I haven't had the time to do anything else yet :)

@spencerthayer
Copy link

If you can get this to work this will change everything!

@kode54
Copy link
Contributor

kode54 commented Aug 25, 2016

Don't forget, in case it's not already set for the server, utf8mb4 encoding. MySQL and probably also MariaDB need that for full character set range, or else you start chucking out things like emoji.

E: I also checked your code. Nice, simple, to the point. But it could use something for the future log reading function. Specifically, it should have a LIMIT directive, possibly the count of which would be supplied by the caller.

@richrd
Copy link
Member

richrd commented Sep 30, 2016

I thought I commented on database logging before, but can't find it now.

Anyway first off I fully support a database option. However I really hope it'll be an option that can be enabled or disabled (which ever the default will be). I don't want logs to be stored on disk, I just don't need it. I'm fine with having them in RAM (but with a configurable maximum limit for the amount of messages).

That said it does looks like this PR does include a config boolean for the db, that would be perfect! And as I mentioned this is a great feature to have 👍

@richrd richrd mentioned this pull request Sep 30, 2016
@willwh
Copy link
Author

willwh commented Oct 1, 2016

@spencerthayer so I have this working now, thanks to @maxpoulin64 and @xPaw; it still needs some pretty serious cleanup, although, it now stores and loads logs on boot.

i.e. There is a maxHistory property in config.js and we need something similar for the mysql fetching, so we're not returning 1000's of lines to the client on start etc ;)

To ease the setup a little bit, I've also added scripts/irclogs.sql which will create a user, and database, and table to store everything. Make sure you change the password in the script before running it ;)

Run it like so: (MySQL 5.7.6+)

mysql < scripts/irclogs.sql

I would love to build in a command to do the initial mysql setup from lounge.

The last hurdle to make this really usable too, is to think long and hard about how we want to return "historical" logs..... I'd actually propose something like added a new button in the bottom left tool bar, "Search Logs" or something, that would allow you to search historical logs.

The way the client is currently written, I can forsee some challenges in trying to load things with the "Show more messages" type functionality we have now, which is why I am suggesting a search type interface.

Would love others to chime in with their thoughts, and thanks everyone for the hard work, thelounge is a sweet IRC client.

p.s. next on my agenda is Twilio integration for highlights to be SMS'd to a user in config, if they have no client attached :)

@AlMcKinlay
Copy link
Member

Left some discussion about this on #442

@AlMcKinlay
Copy link
Member

Hey, @willwh, are you still interested in working on this?

@willwh
Copy link
Author

willwh commented Apr 25, 2017

I've been running this for ages on my own branch; it's working really nicely for me. I'd love to tie in better log return, but just don't have time currently :)

I wrote a little log crawler interface but it should probably be rolled in to the client!

@AlMcKinlay
Copy link
Member

AlMcKinlay commented Apr 25, 2017

Would you have time to rebase and solve the conflicts? If not, would you be happy to let someone else clean it up? Would be good to have. Based on how this currently is, and the fact that it's an opt-in feature, I'd be happy with the state that it's in, but obviously will have to see how the other maintainers feel. At least as a first step. Other features will be useful, but this is good on it's own.

@willwh
Copy link
Author

willwh commented Apr 25, 2017

I'm happy for anyone to clean up the conflicts, I won't have time to do that until the weeekend :)

@xPaw
Copy link
Member

xPaw commented Apr 26, 2017

Can we default this to sqlite for easier testing and shipping? I think eventually we could use it for complete storage.

@@ -49,7 +49,8 @@
"fs-extra": "0.30.0",
"irc-framework": "2.5.0",
"lodash": "4.11.2",
"moment": "2.13.0",
"moment": "^2.13.0",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Revert this line.

@@ -0,0 +1,15 @@
-- This script requires MySQL 5.7.6+
CREATE DATABASE IF NOT EXISTS irclogs;
CREATE USER IF NOT EXISTS 'thelounge'@'localhost' IDENTIFIED BY 'WLJg123r0823r@!#RPI!H@RQ';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't include user stuff in SQL file, plan for minimum permissions.

@@ -84,5 +85,9 @@ module.exports = function(irc, network) {
highlight: highlight
});
chan.pushMessage(client, msg);
// Log to mysql
if (Helper.config.mysql.enabled && data.type === "message" && msg.from !== "") {
db.log(msg.from, network.name, chan.name, msg.text);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're losing a lot of information by not storing the entire msg object.

@@ -1,5 +1,7 @@
var Chan = require("../../models/chan");
var Msg = require("../../models/msg");
var Helper = require("../../helper");
var db = require("../../database.js");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove .js from requires.

@bews
Copy link
Contributor

bews commented May 1, 2017

Can we default this to sqlite for easier testing and shipping? I think eventually we could use it for complete storage.

Big +1 to this.

@spencerthayer
Copy link

SQlite would be ideal for me.

@AlMcKinlay
Copy link
Member

Hey, @willwh, would you have any time to switch this over to sqlite and fix conflicts, or should we close it if that's not going to happen? This is still something we'd love to have.

@astorije
Copy link
Member

Hey @willwh, sorry but we are going to close this for inactivity, and because some very recent work has started on that in #1529. Feel free to comment in this PR or the new one if you have questions or want to resume work on this :)

Either way, thanks a lot for your help and contributions, even if this one didn't make it!

@astorije astorije closed this Sep 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Meta: Do Not Merge This PR should not be merged. Type: Feature Tickets that describe a desired feature or PRs that add them to the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants