Ikabot-Collective / ikabot

A Python-based bot designed for automating tasks in the game Ikariam
https://discord.com/invite/3hyxPRj
MIT License
97 stars 65 forks source link

Extract logging from session #206

Closed pecataToshev closed 9 months ago

pecataToshev commented 9 months ago

Currently we have our own custom solution to provide the logs. Is't not very user friendly and is adding additional complexity of the whole application. We could use a more standard approach in handling the logs.

Problems with the current solution:

This code proposal implements more standart way of managing the logs. We start to use the default python logging package and we start to separate the logs into different files by date. ALso it removes the need to have session to be able to write logs.

Future proposals:

ikagod commented 9 months ago

Once again, I appreciate your work on ikabot but unfortunately, this won't make it into the main branch, but I will address some of the complaints made in the PR because some of them are valid:

If you want to write a log you must have a session

Yes this is a bit of an issue, but generally I think most issues that happen will usually happen during login, and all of us working on the login part of ikabot are very well acquainted with these issues (blackbox, captcha etc...), or they happen within a module, for which a session is a REQUIREMENT. I do agree that this is a limitation, but only a small one. I think instead of reworking the whole logging system, we could add a crash report somewhere in the temp directory if the OS allows it.

LogView in the application is very limited

What exactly do you mean by limited here? It allows users to sort logs however they want and read what the log is about. The logging system even allows for the appending of the last 5 network requests and replies, which I think is quite useful (most problems happen due to some miscommunication between the client and server). And lastly, it supports dumping the log to pastebin (the requests and responses are gzipped and base64ed because of their size). I think this is anything but limited.

Everything is in one file and if you need to read the logs, if the application has been running for a week is very hard to open and follow through it

This is true and I think some solution to it is necessary, like removing the log file every week or so, or alternatively we could just gzip it and leave it in the temp directory.

The use of the logging package is good, but it can be added as a feature (only log messages with it), not used as a complete replacement of the system we already have. It's enough to have the writeLog function write the log message to the logging module if you want to use it. We could also use it to log messages without a session object.

Setting log level is only applied to the current and newly created procceses (with and withouth the solution). This could be resolved in multiple ways and in My opinion the best one will be to be able to set the log level on application start (with some sort of command line argument).

The big issue with setting the log level at application start is that the user session data is encrypted, so if the log level is contained within it, it's unreadable at application start.

Again, I appreciate your work, but I think in the future you should open an issue first and explain why you think nuking an entire module from ikabot is a great idea and wait for some constructive criticism first so you don't waste time writing code that won't be merged. If you want to ensure your code gets merged, try to work on evaluated issues and try to refrain from making huge changes in the code base. I know that the basis of ikabot's codebase is not perfect but that's what we're working with. Try to fit your code in a little bit more with what's already there.

pecataToshev commented 9 months ago

@ikagod don't worry, I didn't waste my time since I've already forked and runnig my own version and have no plans in contributing more to this project.

I'll share my thoughts on some of the things that I've spotted in this project. You could look at them or just ignore them your choice.

  1. Session encryption is a completely useless feature. Most of the users have their own computers and do not share computer accounts. Even if they do, they have to clean the bash history file after each login; otherwise, their Ikariam's username/password is visible.

  2. There is no difference between function and a bot. E.g. account status is a function, auto-piracy is a bot. There should be a separate package for things that operate without user's interaction and a function which provides a configuration for them. This way it will be way easier to spot bugs.

  3. Bot function information - there is no way to understand what a bot is doing. Is it waiting for something, it it executing a job at the moment. No one knows.

  4. The process statuses - when I tried to build multiple buildings with multiple levels, I can't see any info about that in the process lits. Is this pid for my first city, is it for the third? And this just keeps me guessing am I killing the right one or I'll have to restart it. This is also relatable to the piracy, monitoring and so on. There should be a lot more info in the process table, since this is the only way the bot's could communicate to the user without telegram. E.g. there should be a task objective, last action timestamp, next acton timestamp, target city, etc.

  5. Logging

    • Logging view - I can't think of a reason when you should sort the logs in any other order, but in chronological. Sure, there might be some, but maintaining a whole feature for this is just a waste of time and resources. It's better to have filtering than sorting. In the end it's a file on your coputer. Also if you switch to the logging package, as I did, there are multiple log file rotators, which automatically changes the files when a certain criteria is met which will solve the problem with big log files.
    • Logging without user info - This makes debugging hell. I've ran the master version for a while with multiple accounts on the same machine and it was so hard to trace a single event, that I just gave up.
    • Automatic upload to pastebin - Well, it's a file on you computer, how often will you upload it to there. Also this feature can still be used even with the logging package.
    • No basic logging. E.g.
      • I've found an executing mission, will wait till I start executing mine (piracy related)
      • I'm starting the wine chek
      • I'm done with the wine chek
      • Etc. The whole concept for logs is just not right and there is a lot of possible improvements. Sure, mine might not be the best, but is a step in the better logging path.
  6. No Contributing guidelines - This is related to your issue with the package I've added. There is no way I've been able to understand this unless it's written somewhere. If you plan to continue with developing this (not sure about that) please add such thing to prevent such scenarios in the future. Also there should be some requirements for the code - copling, cohesia will be a good examples on how to add more code to this. Also a code formatter and static code analyzer.

  7. The thing with the additional additional package, sound more like you want to make the developers in pain. Particularly for this package, it looks like it will be a lot easier for the devs (since it was trying to be added more than once). No one is looking at packages sizes anymore. If it' less than 50 megabytes, it will not make any difference to the user. You comment on that left me with the feeking that you don't want to improve anything, just because there are some features that were written in other way. There is no common approach other than the regex between the features.

  8. Lots of dead code. Almost in every file there is a bunch of dead code, that is not used anywhere.

  9. Lot's of duplicated code with a little bit different function. E.g. asking user yes/no questions, printing a data to table. This could go to an common code and be reused.

Don't get me wrong, just the impression you left in me is that this is something you've created with a lot of care and now it's more like a debt to you. There are a great people that are contributing to this project and I hope that you'll manage to eveolve this to the next level.

Wish you all the best, Pecata

ikagod commented 9 months ago

I should let you know first that I'm not even the project owner. I'm a long time contributor and collaborator on this project so I'm also hesitant to make unwarranted changes to the codebase like removing the encryption from the session data. I agree with you it's useless for most people but that's how the project owner envisioned it working so I don't see a reason to switch it up.

Controlling running tasks is something that could just be added into the main menu. Like triggering certain functionalities of those tasks at the users command. I think this is a good idea but is kind of a nightmare to implement. Each task, because it's running in a separate process, would have to keep listening for events triggered by the main process.

Yes the process status feature could be more widely used across the project. I only implemented it recently. I think you're right about there being a need for more information (e.g. how long ago the status was updated). This is something that I've even mentioned in the commit #173 that introduced the process status message.

I don't understand why both logging features can't co-exist? I've already mentioned this, I'm not strictly against using the logging package at all, I'm just against replacing a more feature-rich logging system with the inbuilt logging package. I will 100% agree with you that the logging system in general is VERY underused in ikabot. There should be logging statements everywhere in the code where something of significance is about to happen. I just don't have enough time to work on this project to make it better so I do what I can and when I can. I just don't think removing large chunks of ikabot for the sake of it being "A step in the right direction" is right especially considering how slowly the development of ikabot has been going over the years.

And you're right about the dead and duplicated code, that could be fixed, but as I've already stated I don't have much time to go through all the modules (a good portion of which I had no hand in writing) and analyze them and extract common code into the helpers directory, especially because new contributors won't even bother to look into that directory when writing their own modules and end up writing their own versions of the functions anyway.

I do work on ikabot with a lot of care and will continue to do so. Without my infinitely wise guidance (especially on the package management front) this project today would be a console application that also has a GUI written in MS Visual Studio (but only for two features) with 5GB of additional packages and requirements containing God knows what kind of vulnerabilities and it would probably be running at least 5 instances of different browsers in the background, forming a pentagram for summoning demons that then do the actual "automated" work.

P.S. regex really isn't that bad, especially now with tools like ChatGPT, BingAI, Google Bard and various online regex playgrounds like https://regex101.com/