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

Consider adjusting default log level to improve developer experience #851

Closed
devleejb opened this issue Jun 15, 2024 · 10 comments · Fixed by #871
Closed

Consider adjusting default log level to improve developer experience #851

devleejb opened this issue Jun 15, 2024 · 10 comments · Fixed by #871
Assignees
Labels
enhancement 🌟 New feature or request good first issue 🐤 Good for newcomers

Comments

@devleejb
Copy link
Member

devleejb commented Jun 15, 2024

Description:
Currently, various events such as PushPull are being logged, which may not be necessary for developers using the yorkie-js-sdk. Additionally, error messages produced periodically by WatchDocument may be difficult for front-end developers to differentiate between intentional errors and actual issues.

Why:

  • Developers using the yorkie-js-sdk may not be concerned with events like PushPull.
  • Front-end developers may find it challenging to identify whether errors produced by WatchDocument are intended or actual issues.
@devleejb devleejb added enhancement 🌟 New feature or request good first issue 🐤 Good for newcomers labels Jun 15, 2024
@devleejb devleejb moved this to Backlog in Yorkie Project - 2024 Jun 22, 2024
@gwbaik9717
Copy link
Contributor

I'm interested in issue. can I give it a shot?

@devleejb
Copy link
Member Author

@gwbaik9717
Of course. If you have any questions, feel free to ask.

@gwbaik9717
Copy link
Contributor

Hi, I'm about to make a commit which lowers verbosity by adjusting default LogLevel from Info to Error.

However, I'm not really sure that my commit will fix the second issue which is "Additionally, error messages produced periodically by WatchDocument may be difficult for front-end developers to differentiate between intentional errors and actual issues."

In fact, I couldn't find where the error log periodically logs in my examples in Yorkie-JS-SDK so far...

Is there any way to reproduce the situation?

@krapie
Copy link
Member

krapie commented Jul 16, 2024

@gwbaik9717 WatchDocument's periodic error log can be checked only when yorkie server is in cluster mode.

Since our hosted server is in cluster mode, all the applications using our API will get the error.
In the other hand, when you are testing on your own local environment with single server, it will not emit WatchDocument error log because it is not in cluster mode.

So you need to change the url to our Yorkie SaaS API, which is https://api.yorkie.dev to see the error.

Edit: We are working on fixing this issue in our cluster mode.

@gwbaik9717
Copy link
Contributor

gwbaik9717 commented Jul 16, 2024

Thanks for the comment! I was able to reproduce the issue.

Image

I have one more question.

I've tried to adjust default log to higher level (such as debug, error), but WatchDocument error log still remains.

Apparently, that is because WatchDocument is not using custom logger to manage its logging. (it seems to be using default error logging such as console.error.)

Therefore, I do not get the relationship between the logging configuration and the WatchDocument logs.

@krapie
Copy link
Member

krapie commented Jul 16, 2024

I think we can skip WatchDocument error log for now.
This is different kind of issue that yorkie server cluster have to handle.

@gwbaik9717
Copy link
Contributor

Thanks for the comment.

One last question: I'm not sure if this is the right place to ask, but as I read through the yorkie-sdk documentation about the logger, I noticed that the description for LogLevel.Trivial is "Lowest level of verbosity". However, I believe it should be Highest verbosity instead of Lowest, since it logs the most among other log levels.

Is there something I'm missing?

Screenshot 2024-07-17 at 8 03 41 AM

@krapie
Copy link
Member

krapie commented Jul 17, 2024

@devleejb @chacha912 Could you check above description?

@chacha912
Copy link
Contributor

chacha912 commented Jul 17, 2024

Hi @gwbaik9717 🙌

I've tried to adjust default log to higher level (such as debug, error), but WatchDocument error log still remains. Apparently, that is because WatchDocument is not using custom logger to manage its logging. (it seems to be using default error logging such as console.error.) Therefore, I do not get the relationship between the logging configuration and the WatchDocument logs.

I noticed that the description for LogLevel.Trivial is "Lowest level of verbosity". However, I believe it should be "Highest verbosity" instead of "Lowest", since it logs the most among other log levels.

  • @devleejb, Could you review the documentation for LogLevel.Trivial?

@devleejb
Copy link
Member Author

@chacha912 @gwbaik9717

I noticed that the description for LogLevel.Trivial is "Lowest level of verbosity". However, I believe it should be "Highest verbosity" instead of "Lowest", since it logs the most among other log levels.

I think it is incorrect and confusing.
I have created a GitHub issue with it.
yorkie-team/yorkie-team.github.io#148

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement 🌟 New feature or request good first issue 🐤 Good for newcomers
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants