cap-js / ord

Open Resource Discovery (ORD) is a protocol that allows applications and services to self-describe their exposed resources and capabilities. This plugin enables generation of ORD document for CAP based applications.
Apache License 2.0
3 stars 4 forks source link

feat: add cds logger #93

Closed aramovic79 closed 1 week ago

aramovic79 commented 1 week ago

@zongqichen @swennemers @Fannon : A logging is added to the ORD Plugin. For logging is re-used the logger @sap/cds. I thought it's sufficient to have only two levels of logging: DEBUG and WARN(including ERROR); the first one for development environment and the second one(s) for production. "Activating" logging in DEBUG mode can be done in two ways: by setting environment variable DEBUG or by setting the DEBUG property in the .cdsrc.json to some truthy value. Could you please review the changes? Thanks.

zongqichen commented 1 week ago

@zongqichen @swennemers @Fannon : A logging is added to the ORD Pluging. For logging is re-used the logger @sap/cds. I thought it's sufficient to have only two levels of logging: DEBUG and WARN(including ERROR); the first one for development environment and the second one(s) for production. "Activating" logging in DEBUG mode can be done in two ways: by setting environment variable DEBUG or by setting the DEBUG property in the .cdsrc.json to some truthy value. Could you please review the changes? Thanks.

Hi @aramovic79 , I agree to have only two levels, but the levels are the others. My opinion is level INFO and ERROR are important. When you debug or monitor logs in production env, INFO and ERROR can provide clear message to people who look into it. When I debug in dev env, the DEBUG level logger can't support me more than debug mode in vscode. Based on my experience, I always ignore warning message, since for me the program is either succeed or failed. So we need ERROR level message which can wake up admin at night shift when the program broke out.

aramovic79 commented 1 week ago

Hi @aramovic79 , I agree to have only two levels, but the levels are the others. My opinion is level INFO and ERROR are important. When you debug or monitor logs in production env, INFO and ERROR can provide clear message to people who look into it. When I debug in dev env, the DEBUG level logger can't support me more than debug mode in vscode. Based on my experience, I always ignore warning message, since for me the program is either succeed or failed. So we need ERROR level message which can wake up admin at night shift when the program broke out.

Please check here how the log levels are enumerated in cap cds(the lower number => the higher severity).

This is here-implemented logging setup:

In dev environment, the DEBUG level is set to some truthy value(in .cdsrc.json or by setting DEBUG env. variable), which results in showing in console(or log file or ...) all of the following logs:

In production, the default level is WARNING, which means that only the following logs will be shown:

@zongqichen : Is your proposal to set default level in production to be ERROR(so to "consider" only the logger.ERROR('Will be shown);)? If yes, I'm fine with that ...

zongqichen commented 1 week ago

Hi @aramovic79 , I agree to have only two levels, but the levels are the others. My opinion is level INFO and ERROR are important. When you debug or monitor logs in production env, INFO and ERROR can provide clear message to people who look into it. When I debug in dev env, the DEBUG level logger can't support me more than debug mode in vscode. Based on my experience, I always ignore warning message, since for me the program is either succeed or failed. So we need ERROR level message which can wake up admin at night shift when the program broke out.

This is the current setup:

In dev environment, the DEBUG level is set to some truthy value(in .cdsrc.json or by setting DEBUG env. variable), which results in showing all of the following logs:

  • logger.**DEBUG**('Will be shown');
  • logger.**INFO**('Will be shown');
  • logger.**WARN**('Will be shown');
  • logger.**ERROR**('Will be shown');

In production, the default level is WARNING, which means that only the following logs will be shown:

  • logger.**WARN**('Will be shown');
  • logger.**ERROR**('Will be shown');

@zongqichen : Is your proposal to set default level in production to be ERROR(so to "consider" only the logger.ERROR('Will be shown);)? If yes, I'm fine with that ...

My proposal is even simpler. We don't need to distinguish dev or production environment, we only use INFO and ERROR when it is really necessary. It will keep our code and console clean. And I visit other repos in cap-js, there are quite mix, some repo use debug and warn like cds-typer, some don't use at all like change-tracking. My preference is the second one. However, I don't strong opinion on it, what do you think @swennemers @Fannon ?

zongqichen commented 1 week ago

Hi @aramovic79 , I forget to mention it, could you revisit all codes and replace all, console.error, console.info or new Error with logger? If you think it's out of the scope, you could raise an new issue for it. Thx

aramovic79 commented 1 week ago

Hi @aramovic79 , I forget to mention it, could you revisit all codes and replace all, console.error, console.info or new Error with logger? If you think it's out of the scope, you could raise an new issue for it. Thx

Done as requested. Also, the issue #86 is fixed in this PR as well, since it was partially related to the logging. Could you please take a look? Thank you!

zongqichen commented 1 week ago

Hi @aramovic79 , thank you for replacing console log. There is still a small issue need to be adjust, https://cap.cloud.sap/docs/node.js/cds-log#recommendations. The construct format message in logger will be expensive for production env. The best practice is always use , to separate.

aramovic79 commented 1 week ago

g. There is still a small issue need to be adjust, https://cap.cloud.sap/docs/node.js/cds-log#recommendations. The construct format message in logger will be expensive for production env. The best practice is always use , to separate.

Good point. Changed as proposed.