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

#3040 - Provide additional logging #3375

Merged

Conversation

nanoblit
Copy link
Collaborator

@nanoblit nanoblit commented Sep 29, 2023

How the feature works? / How did you fix the issue?

(Screenshots, videos, or GIFs, if applicable)

Adds a KetcherLogger class, which can be used to log in dev and prod with specific flags in window.ketcher enabled.
Adds logs to throws so we can see where they happened in prod.

Check list

  • unit-tests written
  • e2e-tests written
  • documentation updated
  • PR name follows the pattern #1234 – issue name
  • branch name doesn't contain '#'
  • PR is linked with the issue
  • base branch (master or release/xx) is correct
  • task status changed to "Code review"
  • reviewers are notified about the pull request

@nanoblit nanoblit linked an issue Sep 29, 2023 that may be closed by this pull request
@nanoblit nanoblit marked this pull request as ready for review September 29, 2023 11:19
window.console.warn(warning);
}

static error(error: unknown): void {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you, please, clarify the difference between error and showExceptionLocation? What is the difference between error and exception?
My proposal is to have only one method error, which can work like this:
KetcherLogger.error('someFile::someFunction', error); – please, note that error can be an object

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@@ -26,7 +27,11 @@ export function identifyStructFormat(
if (JSON.parse(sanitizedString)) {
return SupportedFormat.ket;
}
} catch (er) {} // eslint-disable-line
} catch (er) {
KetcherLogger.showExceptionLocation(
Copy link
Collaborator

Choose a reason for hiding this comment

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

General comment for everywhere – to pass exception to logger. If someone has any error, then you'll be able to find the place, where error has occurred, but won't be able to understand the error it self.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@@ -71,6 +73,13 @@ const App = () => {
structServiceProvider={structServiceProvider}
onInit={(ketcher: Ketcher) => {
window.ketcher = ketcher;

KetcherLogger.settings = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

My proposal is to remove this from here. Actually, for 99% of users logger won't be required. It is needed for cases, when we will directly ask customer like "Open the console, write this command, and send logs to us".
So, can you additionally add to description, how logger can be enabled via Dev Tools console?

Copy link
Collaborator Author

@nanoblit nanoblit Sep 29, 2023

Choose a reason for hiding this comment

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

To enable logger type in the console:
window.ketcher.logger.enabled = true

The default log level is "ERROR" or 0.
To change log level type in the console:
window.ketcher.logger.level = 1 to enable warnings
2 to enable infos
3 to enable logs

To show traces on logs and infos, type in the console:
window.ketcher.logger.showTrace = true

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

export enum LogLevel {
ERROR = 0,
WARN = 1,
LOG = 2,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we add a method info here? It can be called this way console.info(). This is just additional method for the same logging, but it will allow you to reduce number of logs by setting proper log level

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@Nitvex Nitvex changed the title #3040 - Add KetcherLogger #3040 - Provide additional logging Sep 29, 2023
@Nitvex Nitvex merged commit 077e78c into master Sep 29, 2023
@Nitvex Nitvex deleted the 3040-provide-informative-errors-for-users-and-developers branch September 29, 2023 14:37
AnastasiyaPiatrovaKlu pushed a commit that referenced this pull request Oct 2, 2023
* #3040 - Add KetcherLogger

* #3040 - Add default logInfo

* #3040 - Raname variables and add default logging settings

* #3040 - Rename variables

* #3040 - Apply suggestions from reviews
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide additional logging
3 participants