clicon / clixon

YANG-based toolchain including NETCONF and RESTCONF interfaces and an interactive CLI
http://www.clicon.org/
Other
216 stars 72 forks source link

Logging needs revamp #450

Open pprindeville opened 1 year ago

pprindeville commented 1 year ago

In particular, there are operational transitions (like major internal state changes) that should be logged as INFO, for instance, and other failing consistency checks should also be logged as INFO or NOTICE.

One should be able to diagnose misconfigurations, etc. without having to run things manually in debug mode and sift through the output.

A good rule of thumb is that debug is useful for developers troubleshooting.

Anything else should be done by logging.

And logging should be adequate so that if an anomalous situation arises, sufficient logging should be the norm so that you should have already have collected enough useful insight into the situation as it happened. It shouldn't require reproducing the situation from scratch, because you might not have enough information to even know how to go about that otherwise.

If not the why, then at least the what and when, and possibly the how.

As an example:

    if (ret == 0){ /* restconf disabled */
        clicon_debug(1, "restconf configuration not found or disabled");
        retval = 0;
        goto done;
    }

since his message is something that's of interest to users and not just developers, it should probably be logged. Also because it has to do with handling what may be an anomalous condition.

Also, there are messages such as:

            fprintf(stderr, "config: daemon");

and:

            perror("malloc");

which are ineffective because they're only ever seen when the daemon is foregrounded and running to a console with a human actually watching it. (And to be clear, running out of memory is a failure significant event...)

If the process is detached from a console (as it would be if it were started by systemd for instance), then this message will never been seen, so it doesn't matter how useful it is.