Add more debugging and debugging granularity

[pprindeville]

Now complete with must and when debugging

[pprindeville]

@olofhagsand Where does Y_WHEN handling happen?

[olofhagsand]

Primarily here: https://github.com/clicon/clixon/blob/7b58c1c5629e4e5f4e20e15fb2c0dafe2719368e/lib/src/clixon_validate.c#L1273 But also when applying augments and some other places.

[olofhagsand]

So if I get the gist of it, for example if you declare a debug print as: clixon_debug(CLIXON_DBG_YANG | CLIXON_DBG_DETAIL, "My debug"); In order to get that debug printout, you need to run the program as: -D 64 Running the program with anything else (such as -D 64) will not produce that debug output?

Maybe that is "least surprise", I am not sure?

[pprindeville]

So if I get the gist of it, for example if you declare a debug print as: clixon_debug(CLIXON_DBG_YANG | CLIXON_DBG_DETAIL, "My debug"); In order to get that debug printout, you need to run the program as: -D 64 Running the program with anything else (such as -D 64) will not produce that debug output?

You'd need to run it with -D 68 (4 + 64).

[olofhagsand]

Yes, right -D 68 (typo). And it is also backward-compatible in the sense that a declared debug-level of, say 2, is printed with -D 3, -D 6 etc?

[olofhagsand]

I was also thinking about which flags to actually use, I see now:

#define CLIXON_DBG_DEFAULT 1 /* Default logs */
 #define CLIXON_DBG_MSG     2 /* In/out messages and datastore reads */
 #define CLIXON_DBG_DETAIL  4 /* Details: traces, parse trees, etc */
 #define CLIXON_DBG_EXTRA   8 /* Extra Detailed logs */
#define CLIXON_DBG_XML    16 /* XML processing */
#define CLIXON_DBG_XPATH  32 /* XPath processing */
#define CLIXON_DBG_YANG   64 /* YANG processing */

With your change it seems like it is now "subject" + "severity" , where subject is MSG, XML, XPATH, YANG. But levels 1 and 8 are somewhat strange, maybe one would want a "subject" + detail-level or something

[pprindeville]

Yes, right -D 68 (typo). And it is also backward-compatible in the sense that a declared debug-level of, say 2, is printed with -D 3, -D 6 etc?

That's right. Whatever bits are in the clixon_debug() message have to match (all of) the same bits in the -D argument. A lot of messages only have a single bit set.

[pprindeville]

I was also thinking about which flags to actually use, I see now:

#define CLIXON_DBG_DEFAULT 1 /* Default logs */
 #define CLIXON_DBG_MSG     2 /* In/out messages and datastore reads */
 #define CLIXON_DBG_DETAIL  4 /* Details: traces, parse trees, etc */
 #define CLIXON_DBG_EXTRA   8 /* Extra Detailed logs */
#define CLIXON_DBG_XML    16 /* XML processing */
#define CLIXON_DBG_XPATH  32 /* XPath processing */
#define CLIXON_DBG_YANG   64 /* YANG processing */

With your change it seems like it is now "subject" + "severity" , where subject is MSG, XML, XPATH, YANG. But levels 1 and 8 are somewhat strange, maybe one would want a "subject" + detail-level or something

Yes only DETAIL and EXTRA are severity qualifiers... DEFAULT should perhaps be 'GENERAL' instead, since it's not scoped to XML, XPATH, MSG, or YANG...

[pprindeville]

I thought about reordering the bits, but people might have scripting they use that hardcodes the values, and then there's this in xml2cvec():

    if (clixon_debug_get() > CLIXON_DBG_DEFAULT){
        clixon_debug(CLIXON_DBG_XML | CLIXON_DBG_DETAIL, "%s cvv:", __FUNCTION__);

and in xpath_parse():

    if (clixon_debug_get() > CLIXON_DBG_MSG){
        if ((cb = cbuf_new()) == NULL){
            clixon_err(OE_XML, errno, "cbuf_new");
            goto done;
        }
        xpath_tree_print_cb(cb, xpy.xpy_top);
        clixon_debug(CLIXON_DBG_XPATH | CLIXON_DBG_DETAIL, "xpath parse tree:\n%s", cbuf_get(cb));
    }

It's not clear exactly what this test is trying to do....

[olofhagsand]

Use of ">" is historic when the thinking was of debug level was a only a "detailed/severity" level. Is obsoelete and should be removed. Instead those should check for "detailed" debugging in the subject area(XML vs MSG)

[olofhagsand]

Can you please also revise the text in the user guide to reflect this change here:

https://clixon-docs.readthedocs.io/en/latest/errors.html#debugging

[pprindeville]

Use of ">" is historic when the thinking was of debug level was a only a "detailed/severity" level. Is obsoelete and should be removed. Instead those should check for "detailed" debugging in the subject area(XML vs MSG)

Just did that.

[pprindeville]

Can you please also revise the text in the user guide to reflect this change here:

https://clixon-docs.readthedocs.io/en/latest/errors.html#debugging

Done:

https://github.com/clicon/clixon-docs/pull/5

[pprindeville]

This just occurred to me... do we also need CLIXON_DBG_APP to use in apps/? Indeed, should any code in apps/ be using CLIXON_DBG_DEFAULT? That seems misappropriated...

[olofhagsand]

Note I will merge this first https://github.com/clicon/clixon/tree/log-restruct, this PR then needs to be rebased agianst that.

[olofhagsand]

This just occurred to me... do we also need CLIXON_DBG_APP to use in apps/? Indeed, should any code in apps/ be using CLIXON_DBG_DEFAULT? That seems misappropriated...

This quickly goes out of control. "APPS" in that context (the app dir) would mean RESTCONF/NETCONF/CLI. But one can also think of actual applications where you would want to extend the debug masks with your own bitmasks.

I started this with the error categories. (see enum clixon_err). I regret that now.

I think the idea with a few basic features (MSG/XML/YANG/XPATH) and a detail/severity scale is sane in principle. But the complexity now comes in how to define a canonical set of such features.

[olofhagsand]

You need to rebase after I merged the log-restruct branch

[pprindeville]

You need to rebase after I merged the log-restruct branch

I did and things broke... fixing it.

[pprindeville]

But also when applying augments and some other places.

Well, I can circle back with a 2nd PR to address those other places if you call them out.

[olofhagsand]

I don't understand the logic of CLIXON_DBG_APP. In the declaration it says: "App-specific". But it rather seems to be debugs in the "app" directory, or more specifically, the netconf/restconf/cli/snmp client code. In retrospect, the "app/" subdir is misnamed, the thinking was that the "Lib" directory is the basic functions, while "apps" include the next level adding functionality of the basic clixon clients. In a sense they are "applications" of the lib. But it is misleading, since actual "applications" are the examples, tnsr, etc. A more correct term would be clientlib, frontends, extensions, or something. Therefore using CLIXON_DBG_APP as "application-specific" is misleading. I dont see enough motivation to distinguish from DEFAULT. An app-specific debug flag, as I would interpret it, would something like CLIXON_DBG_WIFI / CLIXON_DBG_TNSR, for example. Possibly one could differentiate between LIB and CLIENT. Or more clearly to define CLIXON_DBG_CLI, CLIXON_DBG_NETCONF and CLIXON_DBG_RESTCONF, although those would never occur in the same binary. Otherwise I think this is a good addition.

[olofhagsand]

Additionally, after this, I would want to remove the ubiquitous FUNCTION parameter., and use the way clixon_err/clixon_err_fn automatically adds FUNCTION and LINE. This would be backward-compatible in an API sense, but will print the FUNCTION twice until all calls are modified

[olofhagsand]

Another comment regarding the debug flags. For pedagogical reasons, I think one could group them in "subject area" and "detail level". The subject areas would be DEFAULT, MSG, XML, XPATH, YANG and detail would be ALWAYS, DETAIL, EXTRA. Or even allocate the two first bits to a four-level detail. Something like:

/* Detail level */
#define CLIXON_DBG_ALWAYS  0x0 /* Rarely used */
#define CLIXON_DBG_DETAIL    0x1 /* Details: traces, parse trees, etc */
#define CLIXON_DBG_DETAIL2     0x2 /* Extra Detailed logs */
#define CLIXON_DBG_ DETAIL3   0x3 /* Even more Detailed logs */
/* Subject area *"
#define CLIXON_DBG_DEFAULT 0x04 /* Default logs */
#define CLIXON_DBG_MSG         0x08 /* In/out messages and datastore reads */
#define CLIXON_DBG_XML       0x10 /* XML processing */
#define CLIXON_DBG_XPATH  0x20 /* XPath processing */
#define CLIXON_DBG_YANG   0x40 /* YANG processing */
#define CLIXON_DBG_APP      0x80 /* App-specific */

In this way, a debug mask is a combination of a detailed level 0-3 and a set of subject areas

[pprindeville]

I don't understand the logic of CLIXON_DBG_APP. In the declaration it says: "App-specific". But it rather seems to be debugs in the "app" directory, or more specifically, the netconf/restconf/cli/snmp client code. In retrospect, the "app/" subdir is misnamed, the thinking was that the "Lib" directory is the basic functions, while "apps" include the next level adding functionality of the basic clixon clients. In a sense they are "applications" of the lib. But it is misleading, since actual "applications" are the examples, tnsr, etc. A more correct term would be clientlib, frontends, extensions, or something. Therefore using CLIXON_DBG_APP as "application-specific" is misleading. I dont see enough motivation to distinguish from DEFAULT. An app-specific debug flag, as I would interpret it, would something like CLIXON_DBG_WIFI / CLIXON_DBG_TNSR, for example. Possibly one could differentiate between LIB and CLIENT. Or more clearly to define CLIXON_DBG_CLI, CLIXON_DBG_NETCONF and CLIXON_DBG_RESTCONF, although those would never occur in the same binary. Otherwise I think this is a good addition.

If you think the low-level libraries are working as expected, and just want to see if the application is working properly, you might want to debug the application itself.

It's a trivial change to globally rename CLIXON_DBG_APP to something else. How about CLIXON_DBG_CLIENT?

[pprindeville]

Additionally, after this, I would want to remove the ubiquitous FUNCTION parameter., and use the way clixon_err/clixon_err_fn automatically adds FUNCTION and LINE. This would be backward-compatible in an API sense, but will print the FUNCTION twice until all calls are modified

I can do that in a separate PR.

[pprindeville]

Another comment regarding the debug flags. For pedagogical reasons, I think one could group them in "subject area" and "detail level". The subject areas would be DEFAULT, MSG, XML, XPATH, YANG and detail would be ALWAYS, DETAIL, EXTRA. Or even allocate the two first bits to a four-level detail. Something like:

/* Detail level */
#define CLIXON_DBG_ALWAYS  0x0 /* Rarely used */
#define CLIXON_DBG_DETAIL    0x1 /* Details: traces, parse trees, etc */
#define CLIXON_DBG_DETAIL2     0x2 /* Extra Detailed logs */
#define CLIXON_DBG_ DETAIL3   0x3 /* Even more Detailed logs */
/* Subject area *"
#define CLIXON_DBG_DEFAULT 0x04 /* Default logs */
#define CLIXON_DBG_MSG         0x08 /* In/out messages and datastore reads */
#define CLIXON_DBG_XML       0x10 /* XML processing */
#define CLIXON_DBG_XPATH  0x20 /* XPath processing */
#define CLIXON_DBG_YANG   0x40 /* YANG processing */
#define CLIXON_DBG_APP      0x80 /* App-specific */

In this way, a debug mask is a combination of a detailed level 0-3 and a set of subject areas

Done