-
Notifications
You must be signed in to change notification settings - Fork 9
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.
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
Add minimal log support #22
Conversation
int associationType; // 0:not set,1:new association, 2:existing association | ||
// Log structure | ||
struct logEvent { | ||
// code identifing the kind of events |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we add the port number to logEvent?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As the idea is to have a minimal logging facilities, that's why I didn't add it for now.
But it is in the improvements list : #21 !
We could discuss about that to know if this should be in the v0.1.0.
|
||
# Define pythons log level | ||
logLevelNames = ["CRITICAL","ERROR","WARNING","INFO","DEBUG","TRACE"] | ||
logging.addLevelName(5, "TRACE") # add TRACE level |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
About the log levels, on AirVantage (and Loggly) we only use Error, Warning, Info and Debug. I would suggest doing the same. Debug and Trace are basically the same thing.
On MQTTFE, we used to differentiate Debug and Trace but we ended up merging them because it was too impractical.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DEBUG and TRACE is mainly for development time, not really for production.
I need 2 different log levels : one to track NAT, another for full tracing.
Here the explanation :
CRITICAL => should never happened,
ERROR => unexpected state,
WARNING => unwanted traffic (invalid packet),
INFO => not supported traffic (fragmented ip, ip option used),
DEBUG => all about NAT,
TRACE => all output we can.
} | ||
// Should not happened, mainly needed to make verfier happy | ||
if (rsIp == NULL) { | ||
log(CRITICAL, ctx, INGRESS_CANNOT_CREATE_ASSO2, &logEvent); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't use the Critical level.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason about that ?
This PR aims to implements #4.
This differs a lot from what it was specify initialy.
Finally there is 6 log levels :
CRITICAL => should never happened,
ERROR => unexpected state,
WARNING => unwanted traffic (invalid packet),
INFO => not supported traffic (fragmented ip, ip option used),
DEBUG => all about NAT,
TRACE => all output we can.
A new CLI option is added :
The output looks like this :
Of course more could/should? be done : #21