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

Better error handling and reporting #1892

Open
dusansimic opened this issue Aug 24, 2022 · 5 comments
Open

Better error handling and reporting #1892

dusansimic opened this issue Aug 24, 2022 · 5 comments
Labels

Comments

@dusansimic
Copy link
Collaborator

So far, vast majority of patches (just subjectively) are related to either css fixes or fixing css selectors in Caprine logic (shortcuts or other systems). That's the thing that changes most ofthen. The one we can't really influcence much are css fixes because when they happen, the only thing we can do is create a new release with a fix. The one we can influcence are css selectors.

Once they change and break, most of the time we just patch it up and send out a new release. The issue reports are usually " is not working" and it's up to us to track down what the root cause of it is. It doesn't usually take much time but in case of #1881 it took quite a bit of time to track down what the actuall casue of the issue was (it was a css selector for something else altogether which blocked the execution of code).

Right now elements are queried with document.querySelector or elementReady functions and they could return a null or undefined respectively. Throughout the whole codebase, this is handled by using non-null assertation operator or optional chaining operator which either just "makes the code work" but could throw errors at some point (which happens) or doesn't throw anything altogether and just nothing happens which is painful to debug. It's not that use of these operators is bad, to me it just feels like it's implemented with an expectation that query selectors wouldn't change.

My recommendation for making maintenance and fixing issues easier is to log in some way what queries fail exactly so once an issue occurs, user could just paste the log into the issue report and maintainer could very quickly track down the issue and fix it. Caprine is already having trouble handling all the changes in Messenger website so this would help a lot imho.

The logging I had in mind is writing failed queries with timestamps and any other useful information to a file which would be cleared at every start (so it doesn't clog up the disk if users restart Caprine in sane intervals). On report an issue button in help menu, Caprine could upload or paste directly the log to the issue that's being opened so users wouldn't need to dig around Caprines local files. It seems like the easiest solution and it should add too much to users workflow. The whole refactoring would of course thake some effort but I think it would pay off.

@lefterisgar
Copy link
Collaborator

lefterisgar commented Aug 24, 2022

I agree with all your points. At its current state, Caprine is a total nightmare to debug, let alone fix the issues themselves.

My only concerns are:

  • Could this potentially affect Caprine's performance (which according to numerous reports is terrible already)?
  • How will users receive this? Is it going to be optional?

Also we will have to refactor the code and document everything so people can see what is being collected and how.

@dusansimic
Copy link
Collaborator Author

Could this potentially affect Caprine's performance (which according to numerous reports is terrible already)?

I'm guessing only if those kinds of errors we wan't to log occur frequently (multiple times a second) but that would by itself (the speed of queries) affect the performance, much more than writing logs onto the disk.

How will users receive this? Is it going to be optional?

I think it would be better that it's not optional. Caprine already has a lot of options and we would make sure that no personal data is collected by accident. If it would be optional, I'd prefer it is enabled by default since telling every user to enable this feature and open a new issue so logs would get published is time consuming for users as much for as for us.

Also we will have to refactor the code and document everything so people can see what is being collected and how.

Precicely. A good idea would be writing a document (a markdown file which would be linked in the readme) which describes why is this implemented, what exactly is collected, how it is collected, when it's published (when it could be accessed by other people) and how to disable it if we add that option.

@lefterisgar
Copy link
Collaborator

I'm guessing only if those kinds of errors we wan't to log occur frequently (multiple times a second) but that would by itself (the speed of queries) affect the performance, much more than writing logs onto the disk.

That's fine then. We have to ensure that a change on Messenger's side won't cause spontaneous writing to the disk as that would be problematic.

I think it would be better that it's not optional. Caprine already has a lot of options and we would make sure that no personal data is collected by accident. If it would be optional, I'd prefer it is enabled by default since telling every user to enable this feature and open a new issue so logs would get published is time consuming for users as much for as for us.

I agree. Just like automatic updates. Make it opt out so it's turned on by default but can be disabled if the user desires to do so.

@dusansimic
Copy link
Collaborator Author

Fantastic! I'll work on some ideas for those functionalities so they would be implemented in the cleanest way possible so it's easy to maintain both Caprine code and error handling and reporting code. Initially it will probably be just some written ideas for the code structure and after that I'll work on the implementation. The initial written proposal would need most feedback.

@dusansimic
Copy link
Collaborator Author

The most ergonomic way I can see this being done on the technical side is by using with functions (like the withMenu function ). That way we could make sure queries are executed and elements are passed to the callback function if they are found. What is complicated here is when logically and element could be missing but query fails because Messenger website is changes and the query selector no longer works. Then, we don't really know weather the element is just missing or we need to change the query selector. In that case a warning should be logged (just an indicator that an element is missing but we expect that as a valid result). If someone sees a better solution, please feel free to discuss it here 😁️. All help is welcome.

As discussed, logs would be written to a file which is erased every time Caprine starts and the logging could be disabled by user (it's enabled by default). When reporting issues, they would be notified that logs (if enabled) would be uploaded. We will make sure that they don't contain any sensitive information about the user, just the part of the code that fails.

I'd like to work on the main implementation. Once I have a basic working implementation, I'll open a pr so we can discuss it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants