search

freakent / node-red-contrib-sunevents

A node for node-red that generates events based on the position of the Sun at the appropriate time of day.
Apache License 2.0
15 stars 13 forks source link

Node generates unwanted debug message #23

Closed michalk-k closed 1 year ago

michalk-k commented 3 years ago

The node generates unwanted debug message on each every call: "calculating sunevents for lat XXXX and lng YYYYY"

freakent commented 3 years ago

Unwanted in what sense? I wanted to see when it calculates and with what coordinates. In what way does this cause you a problem?

freakent commented 3 years ago

I should add that since the change to v3, this was a conscious decision made to ensure that we have a way to tell if users are sending a payload to the node at an appropriate frequency. At the moment, I don’t consider this unwanted. Maybe in a few months once updates have stabilised.

michalk-k commented 3 years ago

Debug messages are "DEBUG" ones. Are useful at time of debugging. In production are useless. Just flood logs making them unreadable. It's common practice (in node red nodes), if debug output is considered useful, then it's achievable by dedicated option added to the node.

with regards

freakent commented 3 years ago

To be precise, if you look at the code, you will see it's actually a warning message. Until I am happy that majority of people have upgraded successfully, I'm afraid it needs to stay.

freakent commented 3 years ago

Just one more observation, if this message is flooding your logs you are probably invoking this node way too frequently. Once a day is optimal unless your gps coordinates are changing substantially within the day.

michalk-k commented 3 years ago

you will see it's actually a warning message

Why should anyone be warned about this node is working properly?

I try to reword: it's the first node I know which generates anything but error to log without explicit consent. I'm not against this feature. Just please make option to disable/enable it.

Just one more observation, if this message is flooding your logs you are probably invoking this node way too frequently. Once a day is optimal unless your gps coordinates are changing substantially within the day.

Hmm. I initially understood that v3 has no internal loop/timer thus must be triggered periodically to output most recent value. Now I got it. Still not getting why it must be triggered anyway... But thank you for pointing out my mistake.

freakent commented 3 years ago

Why should anyone be warned about this node is working properly?

That's a fair question and one I did think about carefully when including it in the code. I know there are a lot of users of this node who use Home Assistant. Previously I have logged status messages to the console but Home Assistant users didn't understand how to view these messages. So this is a compromise to support the transition to V3.

Hmm. I initially understood that v3 has no internal loop/timer thus must be triggered periodically to output most recent value. Now I got it. Still not getting why it must be triggered anyway... But thank you for pointing out my mistake.

So it would seem that the warning message is doing exactly what I needed it to do, it "warned" you that it was being called too often :-)

There were lot's of reasons to move the scheduling outside the sunevents node. Most important was that the previous design was causing a problem for some people depending on their timezone. This way you have complete control over when it runs and how often to suit your use case. Externalising the execution also meant we can continuously vary the GPS coordinates which was a feature I definitely wanted to add.

freakent commented 3 years ago

I will leave this issue open as an enhancement request. In some future version I expect I will make logging an option (it was an option in previous versions), but for now it it doing it's job.

Onixarts commented 3 years ago

My vote for additional Debug option for that. Debug console is for debug purpose. I don't want any node put there any message without my will, especially when the node works fine and it is not an error message. Please consider simple Debug checkbox to disable this message.

Imagine any other node puts there messages, cause the debug window gets more messy.

freakent commented 3 years ago

I suggest you read the documentation for node.warn(...) and read my justification for this above. If it’s anymore than a minor inconvenience for you then perhaps you are also resetting the sunevents too often? Once a day is more than enough unless you are changing your GPS position substantially.

Onixarts commented 3 years ago

Well, then You should use node.log, not node.warn for that, because I don't want to know that node is doing its job as it should. Consider the inject node which would post a warning each time it sends a message. In any case it should be an optional if some users need this warning.

freakent commented 3 years ago

Surprisingly, node.log does not output to the sidebar. As explained previously, this is a temporary measure to support the transition from v2 to v3 and will be made optional in a future release.

freakent commented 3 years ago

I’m going to lock this conversation. The issue will remain open and marked as an enhancement. I am really surprised (and a little disappointed) that one simple logging message that will save me time trying to debug other people’s flow as they move to v3 has caused such a negative response. If you want to read the full explanation scroll up.

freakent commented 1 year ago

Version 3.1 has just been released. Now that we have a missed events output it is easier for users to see that the node is working. Plus I believe most people would have migrated to v3 by now. I have therefore removed the message.