Add configurable logger support #390
Conversation
| @@ -661,15 +682,14 @@ void Aggregator::process_packet(const uint8_t *buf, size_t size, uint8_t wlan_id | |||
|
|
|||
| init_fec(new_session_data->k, new_session_data->n); | |||
|
|
|||
| fprintf(stdout, "%" PRIu64 "\tSESSION\t%" PRIu64 ":%u:%d:%d\n", get_time_ms(), epoch, WFB_FEC_VDM_RS, fec_k, fec_n); | |||
There was a problem hiding this comment.
@vertexodessa Why removing these two lines?
There was a problem hiding this comment.
they are replaced with
WFB_LOG(LogLevel::ALWAYS
which unconditionally logs without prefix.
Is it fine to log these lines to stderr?
There was a problem hiding this comment.
No. This is machine-readable telemetry counters and that is why it passed to stdout (also there is flush on the next line).
Machine readable logs are passed to stdout
Humane readable logs to stderr
There was a problem hiding this comment.
@vertexodessa Please rollback removing of this fprintf and fflush
src/rx.cpp
Outdated
| @@ -81,10 +85,12 @@ Receiver::Receiver(const char *wlan, int wlan_idx, uint32_t channel_id, BaseAggr | |||
| program = string_format("ether[0x0a:2]==0x5742 && ether[0x0c:4] == 0x%08x", channel_id); | |||
|
|
|||
| if (pcap_compile(ppcap, &bpfprogram, program.c_str(), 1, 0) == -1) { | |||
| WFB_LOG(LogLevel::ERROR, "Unable to compile %s: %s", program.c_str(), pcap_geterr(ppcap)); | |||
There was a problem hiding this comment.
@vertexodessa Why need to log before raising exception? It will print error twice: in a logger and in an exception handler
There was a problem hiding this comment.
good point, we don't need logging before exceptions, they can be ccaught on the calling side and logged.
src/rx.cpp
Outdated
|
|
||
| WFB_LOG(LogLevel::ALWAYS, "%" PRIu64 "\tSESSION\t%" PRIu64 ":%u:%d:%d", get_time_ms(), epoch, WFB_FEC_VDM_RS, fec_k, fec_n); |
There was a problem hiding this comment.
@vertexodessa Need to change level to INFO to be consistent with other machine-readable telemetry logs
There was a problem hiding this comment.
INFO will add prefix to the log, ALWAYS does not add the prefix
considering this, should I change the level?
There was a problem hiding this comment.
This is telemetry counters for python part of wfb-ng. If will be processes by fprintf(stdout, ... a few lines above. If you need then on the android side then set log level which will skip it on normal wfb-ng build
vertexodessa
force-pushed
the
master
branch
2 times, most recently
from
92df961 to
e3f1ad8
Compare
| @@ -661,15 +682,14 @@ void Aggregator::process_packet(const uint8_t *buf, size_t size, uint8_t wlan_id | |||
|
|
|||
| init_fec(new_session_data->k, new_session_data->n); | |||
|
|
|||
| fprintf(stdout, "%" PRIu64 "\tSESSION\t%" PRIu64 ":%u:%d:%d\n", get_time_ms(), epoch, WFB_FEC_VDM_RS, fec_k, fec_n); | |||
There was a problem hiding this comment.
@vertexodessa Please rollback removing of this fprintf and fflush
src/rx.cpp
Outdated
| @@ -820,14 +827,15 @@ void Aggregator::send_packet(int ring_idx, int fragment_idx) | |||
|
|
|||
| if (packet_seq > seq + 1 && seq > 0) | |||
| { | |||
| WFB_LOG(LogLevel::WARNING, "Lost packet %u\n", seq + 1); | |||
There was a problem hiding this comment.
@vertexodessa Message should be: "Lost %u packets", (packet_seq - seq - 1)
but this already tracked in dump_stats
if(count_p_override)
{
fprintf(stderr, "%u block overrides\n", count_p_override);
}
if(count_p_lost)
{
fprintf(stderr, "%u packets lost\n", count_p_lost);
}
There was a problem hiding this comment.
dump_stats is called within interval while this will be called immediately whe a packet was lost?
then I guess it makes sense to log this, though I'll change the log level to INFO
There was a problem hiding this comment.
ok. But in case of bad signal reception you will be flooded by these messages
There was a problem hiding this comment.
hm after another thought, a lost packet may be considered warning, so I'd prefer to leave it as is.
Should I change the default log level of the StdErrLogger to ERROR then?
If I won't, this will be an additional log compared to the current implementation.
There was a problem hiding this comment.
Please convert it to INFO or DEBUG. Because it will flood stderr during normal wfb-ng operation
There was a problem hiding this comment.
sure, converted. I was considering this log moreimportant than stats, but actually it should be OK for them to have the same importance.
The new logger interface provides a single function, `logln`, which can be implemented by the users of the API. The interface has a default implementation, `StdErrLogger`, which logs the data to `stderr` if the level is higher than `LogLevel::INFO`. `LogLevel::ALWAYS` is always logged without a prefix.
|
@vertexodessa Please rollback removing of: |
The new logger interface provides a single function,
logln, which can be implemented by the users of the API.The interface has a default implementation,
StdErrLogger, which logs the data tostderrif the level is higher thanLogLevel::INFO.LogLevel::ALWAYSis always logged without a prefix to match the existing behavior.