st-one-io / node-red-contrib-s7

A Node-RED node to interact with Siemens S7 PLCs
GNU General Public License v3.0
111 stars 58 forks source link

Logging Issue #12

Closed mscbpi closed 6 years ago

mscbpi commented 7 years ago

Hi,

nodeS7 library creates a lot of logs when using this node. I get 76 message a seconds and it's not going through nodered logging mecanism, I can't even filter out by critical or not.

Thanks

MArtin

mscbpi commented 7 years ago

I've seen the silentMode in nodeS7. You set it from

https://github.com/netsmarttech/node-red-contrib-s7/blob/bf7a4bc62311adc9c1cd5527b96f3e754834a3b8/red/s7.js#L84

https://github.com/netsmarttech/node-red-contrib-s7/blob/bf7a4bc62311adc9c1cd5527b96f3e754834a3b8/red/s7.js#L125-L128

I would set it from the node itself.

I'm investigating what !!RED.settings.get('verbose'); is supposed to be, I am at warn level in nodered and verbose is high.

gfcittolin commented 7 years ago

Hi,

the "verbose" mode from Node-RED is set when you execute it with -v or --verbose, or then when you have verbose: true in settings.js.

It's can be a good idea to put an option on the config node, we may add it in the future (or if you want to submit a PR, always welcomed :) ).

mscbpi commented 7 years ago

Thanks for the quick answer. I use nodered docker image. settings.js doesn't show any verbose. weird if it is launched by default with -v. I'll add a boolean in the config node if you may 👍

gfcittolin commented 7 years ago

Oh.. look at this: https://github.com/node-red/node-red-docker/blob/d36b1c671da1eaec23d3a3c19fd227f343aa26d3/package.json#L12-L14

Adding a checkbox is an option, but I don't like that much the idea of removing the command line option, as I think it's interesting to be able to debug it without needing to change/redeploy the flow. Just adding (or'ing) the checkbox here also wouldn't fix your issue, as the -v is permanently set.

Any idea, without breaking backwards compatibility?

mscbpi commented 7 years ago

Yep, seen that as well.

Well we're quite stuck with the fact log output is managed by dependent library and not the node itself.

mscbpi commented 7 years ago

Thanks for the quick feedback again!

If I just don't use verbose as a trigger for silent/debug but the setting in the config node, it will solve my issue. I break backward compatibility, but I don't get how better it is to stop nodered, relaunch it, rather than redeploying a flow with a modified setting when in debugging mode.

If you want to keep this logic, you might as well be based on debug level of node-red logging (if this can be requested from the node, I don't know).

mscbpi commented 7 years ago

In the config node we might have a triple choice, 'default' (fallback to verbose setting), then 'true' or 'false' that would override verbose setting when creating the S7 endpoint. If option is a string, something like:

var isVerbose = (config.verbose == 'false' || config.verbose == 'true') ? (config.verbose == 'true') : RED.settings.get('verbose');

mscbpi commented 7 years ago

@gfcittolin Pull Request open, what do you think ? Thanks.

gfcittolin commented 7 years ago

Hey, sorry, holidays here. I liked your solution, this way we're the most flexible. I've just included a code to set the config to "default" for old configurations without the parameter. Going to fix some other small things and I'll publish it the next days.

Thanks for it!

gfcittolin commented 6 years ago

This just landed at 1.4.0 with some other minor fixes, published just now. Thanks so much for the contribution!