Implement a logging function

[stcui007]

This PR creates an initial logging solution that mimics Python's logging functionality.

Additions

include/utilities/logging_utils.h
This header file contains the following logging functions as described in Issue#574:
void debug(char *)
void info(char *)
void warning(char *)
void error(char *)
void critical(char *)
that handle different types of output.

Also added a unit test code:
test/utils/logging_Test.cpp

Removals

Changes

As an illustration, Bmi_Module_Formulation.hpp is modified with some codes to use logging_utils.h to output various messages

A few CMakelist.txt files related to the logging functionalities and unit test have minor changes.

Testing

Run ngen test
Unit tests

Screenshots

Notes

Todos

Checklist

• PR has an informative and human-readable title
• [x ] Changes are limited to a single goal (no scope creep)
• [x ] Code can be automatically merged (no conflicts)
• [x ] Code follows project standards (link if applicable)
• [x ] Passes all existing automated tests
• [x ] Any change in functionality is tested
• [x ] New functions are documented (with a description, list of inputs, and expected output)
• [x ] Placeholder code is flagged / future todos are captured in comments
• [x ] Project documentation has been updated (including the "Unreleased" section of the CHANGELOG)
• [x ] Reviewers requested with the Reviewers tool ➡️

Testing checklist (automated report can be put here)

Target Environment support

• [x ] Linux

[mattw-nws]

Several different things here.

1. With the other change below to the method signature (const char*), all that should be required is this:

logging::critical(("case Real: Cannot pass non-numeric lists as a BMI parameter, skipping " + param.first + "\n").c_str());

So the logging_msg and log_msg_ptr variables can be removed and this simplifies the usage a lot.

2. This message is in the wrong place, I think you intended to add this example further down on line 688(old)/675(new)--what happens in this code path (for type Real) is expected and shouldn't generate any message.
3. This would not be a good use of critical, "critical" would usually be used right before an assert, ending the program. The condition down below, where you are "skipping" is probably a warning. Or, if it's that important, we probably shouldn't just skip it and keep going.

[mattw-nws]

As mentioned above, these are probably all warning conditions if the program is going to continue regardless.

[mattw-nws]

Did you mean to leave this in?

[mattw-nws]

Please go ahead and remove commented old code throughout.

[mattw-nws]

The purpose of MAX_STRING_SIZE is not to prevent clients from giving us messages larger than we want to read. Rather, it is to give a bound to C implementations that may need to allocate specific buffer sizes, and perhaps to specify a point at which reading should be stopped if the string is not properly null-terminated. If this latter is the case, std::string::size is probably not going to work anyway.

For a C++ implementation as we have here, what probably makes the most sense is to ignore it. If we get a string that's not properly terminated, we probably can't know what to do with it anyway. Just pass the input to std::cerr... someone else might disagree, the most we could do is check the C-string length with something like strnlen, but this has the same problem as std::string::size. I would probably just ignore the MAX_STRING_SIZE in this implementation.

This simplifies these functions significantly, allowing you to get rid of multiple variables and copies of the string.

[stcui007]

These are all reasonable suggestions. I used "CRITICALl" here just for illustration as only "CRITICAL" and "ERROR" are not "QUIET" so I can see the output actually works. I did test all other functions though. Future version will make appropriate changes.

…

On Tue, Sep 19, 2023 at 4:06 PM Matt Williamson ***@***.***> wrote: ***@***.**** requested changes on this pull request. ------------------------------ In include/utilities/logging_utils.h <#643 (comment)>: > @@ -0,0 +1,96 @@ +#ifndef NGEN_LOGGING_UTILS_H +#define NGEN_LOGGING_UTILS_H + +#include <iostream> +#include <cstring> +#include <string> We need to split the implementation into a separate (.cpp) file. Only the implementation file should have C++ code (e.g. calls to std::cerr) and things like #include <iostream> etc. because those are C++ includes. Note that this may require some CMakeLists.txt edit to get the implementation file to be compiled and the implementation available/linked where needed (e.g. for your Bmi_Module_Formulation example above)... you may want to follow the pattern in src/utilities/mdframe as an example... I think making it linked project-wide as it does for mdframe makes sense. ------------------------------ In include/utilities/logging_utils.h <#643 (comment)>: > + */ + inline void error(char* msg) + { + std::string msg_str = std::string(msg); + if (msg_str.size() > MAX_STRING_SIZE) { + std::cerr << "ERROR: the message string exceeds the maximum length limit" << std::endl; + } + + std::cerr<<"ERROR: " <<std::string(msg) << std::endl; + } + + /** + * Send critical message to std::cerr + * @param msg The variable carries the humanly readable critical message. + */ + inline void critical(char* msg) These all should actually be const char* msg instead of char* msg, that was an oversight in my text in #574 <#574> (I have edited #574 <#574> to be correct now)--this makes the use of these functions much easier (see other comments elsewhere), so please change all 5 functions. ------------------------------ In include/realizations/catchment/Bmi_Module_Formulation.hpp <#643 (comment)>: > + + logging_msg = "case Real: Cannot pass non-numeric lists as a BMI parameter, skipping " + param.first+"\n"; + log_msg_ptr = &logging_msg[0]; + logging::critical(log_msg_ptr); Several different things here. 1. With the other change below to the method signature (const char*), all that should be required is this: logging::critical(("case Real: Cannot pass non-numeric lists as a BMI parameter, skipping " + param.first + "\n").c_str()); So the logging_msg and log_msg_ptr variables can be removed and this simplifies the usage a lot. 1. This message is in the wrong place, I think you intended to add this example further down on line 688(old)/675(new)--what happens in this code path (for type Real) is expected and shouldn't generate any message. 2. This would not be a good use of critical, "critical" would usually be used right before an assert, ending the program. The condition down below, where you are "skipping" is probably a warning. Or, if it's that important, we probably shouldn't just skip it and keep going. ------------------------------ In include/realizations/catchment/Bmi_Module_Formulation.hpp <#643 (comment)>: > @@ -681,13 +688,22 @@ namespace realization { } catch (const std::exception &e) { - std::cout<<"Exception setting parameter value: "<< e.what()<<std::endl; - std::cout<<"Skipping parameter: "<<param.first<<std::endl; + std::cerr<<"Exception setting parameter value: "<< e.what()<<std::endl; + //std::cout<<"Skipping parameter: "<<param.first<<std::endl; + std::string logging_msg = "Cannot pass non-numeric lists as a BMI parameter, skipping "+param.first+"\n"; + char* log_msg_ptr = &logging_msg[0]; + logging::error(log_msg_ptr); As mentioned above, these are probably all warning conditions if the program is going to continue regardless. ------------------------------ In include/realizations/catchment/Bmi_Module_Formulation.hpp <#643 (comment)>: > @@ -681,13 +688,22 @@ namespace realization { } catch (const std::exception &e) { - std::cout<<"Exception setting parameter value: "<< e.what()<<std::endl; - std::cout<<"Skipping parameter: "<<param.first<<std::endl; + std::cerr<<"Exception setting parameter value: "<< e.what()<<std::endl; Did you mean to leave this in? ------------------------------ In include/realizations/catchment/Bmi_Module_Formulation.hpp <#643 (comment)>: > continue; } value_ptr = get_values_as_type(type, double_vec.begin(), double_vec.end()); break; default: - std::cout<<"Cannot pass parameter of type "<<geojson::get_propertytype_name(param.second.get_type())<<" as a BMI parameter, skipping "<<param.first<<std::endl; + //std::cout<<"Cannot pass parameter of type "<<geojson::get_propertytype_name(param.second.get_type())<<" as a BMI parameter, skipping "<<param.first<<std::endl; Please go ahead and remove commented old code throughout. ------------------------------ In include/utilities/logging_utils.h <#643 (comment)>: > +#define MAX_STRING_SIZE 2048 + +#ifdef __cplusplus +extern "C" { +#endif + +namespace logging { + /** + * Send debug output to std::cerr + * @param msg The variable carries the humanly readable debug text info. + */ + inline void debug(char* msg) + { + #ifndef NGEN_QUIET + std::string msg_str = std::string(msg); + if (msg_str.size() > MAX_STRING_SIZE) { The purpose of MAX_STRING_SIZE is not to prevent clients from giving us messages larger than we want to read. Rather, it is to give a bound to C implementations that may need to allocate specific buffer sizes, and perhaps to specify a point at which reading should be stopped if the string is not properly null-terminated. If this latter is the case, std::string::size is probably not going to work anyway. For a C++ implementation as we have here, what probably makes the most sense is to ignore it. If we get a string that's not properly terminated, we probably can't know what to do with it anyway. Just pass the input to std::cerr... someone else might disagree, the most we could do is check the C-string length with something like strnlen, but this has the same problem as std::string::size. I would probably just ignore the MAX_STRING_SIZE in this implementation. This simplifies these functions significantly, allowing you to get rid of multiple variables and copies of the string. — Reply to this email directly, view it on GitHub <#643 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACA4SRIQE7EB3I37HJ5BXZLX3ICORANCNFSM6AAAAAA46SIFOM> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[mattw-nws]

Additionally, really any new functionality added should have some unit tests. This is pretty simple so the unit test doesn't have to be huge, but something that puts a message through each function and makes sure it comes out with the expected text would be good. You can look at this approach from one of @program-- 's old commits for an example of how to do that:
https://github.com/program--/ngen/blob/4a1d137bff0ff3324cfb2239ac1eaec72b66705d/test/forcing/CsvPerFeatureForcingProvider_Test.cpp#L202-L216

[stcui007]

Agree. this should be done.

Team change