Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement a logging function #643

Merged
merged 3 commits into from
Sep 27, 2023
Merged

Implement a logging function #643

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9b73fc4
Implement a logging function
stcui007 Sep 19, 2023
248b5bb
Add unit test for logging and other revisions
stcui007 Sep 25, 2023
b3d2c83
Fix a CMakelist.txt error.
stcui007 Sep 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 22 additions & 6 deletions include/realizations/catchment/Bmi_Module_Formulation.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#include <DataProvider.hpp>
#include <UnitsHelper.hpp>
#include "bmi_utilities.hpp"
#include "logging_utils.h"

using data_access::MEAN;
using data_access::SUM;
Expand Down Expand Up @@ -640,6 +641,8 @@ namespace realization {
//(and by extension, as_c_array) into the JSONProperty class
//then instead of the PropertyVariant visitor filling vectors
//it could fill the c-like array and avoid another copy.
std::string logging_msg;
char* log_msg_ptr;
switch( param.second.get_type() ){
case geojson::PropertyType::Natural:
param.second.as_vector(long_vec);
Expand All @@ -648,6 +651,10 @@ namespace realization {
case geojson::PropertyType::Real:
param.second.as_vector(double_vec);
value_ptr = get_values_as_type(type, double_vec.begin(), double_vec.end());

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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

break;
/* Not currently supporting string parameter values
case geojson::PropertyType::String:
Expand All @@ -665,13 +672,13 @@ namespace realization {
//TODO consider some additional introspection/optimization for this?
param.second.as_vector(double_vec);
if(double_vec.size() == 0){
std::cout<<"Cannot pass non-numeric lists as a BMI parameter, skipping "<<param.first<<std::endl;
//std::cout<<"Cannot pass non-numeric lists as a BMI parameter, skipping "<<param.first<<std::endl;
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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please go ahead and remove commented old code throughout.

continue;
}
try{
Expand All @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mean to leave this in?

//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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
catch (...)
{
std::cout<<"Unknown Exception setting parameter value: "<<std::endl;
std::cout<<"Skipping parameter: "<<param.first<<std::endl;
//std::cout<<"Unknown Exception setting parameter value: "<<std::endl;
//std::cout<<"Skipping parameter: "<<param.first<<std::endl;
std::string logging_msg = "Unknown Exception setting parameter value: \n";
char* log_msg_ptr = &logging_msg[0];
logging::error(log_msg_ptr);
logging_msg = "Skipping parameter: "+param.first+"\n";
log_msg_ptr = &logging_msg[0];
logging::error(log_msg_ptr);
}
long_vec.clear();
double_vec.clear();
Expand Down
96 changes: 96 additions & 0 deletions include/utilities/logging_utils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
#ifndef NGEN_LOGGING_UTILS_H
#define NGEN_LOGGING_UTILS_H

#include <iostream>
#include <cstring>
#include <string>
program-- marked this conversation as resolved.
Show resolved Hide resolved

#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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

std::cerr << "DEBUG: the message string exceeds the maximum length limit" << std::endl;
}

std::cerr<<"DEBUG: " << msg_str << std::endl;
#endif
}

/**
* Send info content to std::cerr
* @param msg The variable carries the humanly readable info text.
*/
inline void info(char* msg)
{
#ifndef NGEN_QUIET
std::string msg_str = std::string(msg);
if (msg_str.size() > MAX_STRING_SIZE) {
std::cerr << "INFO: the message string exceeds the maximum length limit" << std::endl;
}

std::cerr<<"INFO: " <<std::string(msg) << std::endl;
#endif
}

/**
* Send warning to std::cerr
* @param msg The variable carries the humanly readable warning message.
*/
inline void warning(char* msg)
{
#ifndef NGEN_QUIET
std::string msg_str = std::string(msg);
if (msg_str.size() > MAX_STRING_SIZE) {
std::cerr << "WARNING: the message string exceeds the maximum length limit" << std::endl;
}

std::cerr<<"WARNING: " <<std::string(msg) << std::endl;
#endif
}

/**
* Send error message to std::cerr
* @param msg The variable carries the humanly readable error message.
*/
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)
program-- marked this conversation as resolved.
Show resolved Hide resolved
{
std::string msg_str = std::string(msg);
if (msg_str.size() > MAX_STRING_SIZE) {
std::cerr << "CRITICAL: the message string exceeds the maximum length limit" << std::endl;
}

std::cerr<<"CRITICAL: " <<std::string(msg) << std::endl;
}
}

#ifdef __cplusplus
}
#endif

#endif //NGEN_LOGGING_UTILS_H
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree. this should be done.

Loading