-
Notifications
You must be signed in to change notification settings - Fork 284
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
Feature bootstrap trace/debug log output #895
Conversation
@nurupo I added the logger. It outputs something maybe interesting like, some log taged as error. I'm new in tox. Not sure is this feature useful. Any suggestion? |
I moved comment lines into commit to keep source code clean. |
95b8fca
to
8ba7cb6
Compare
other/DHT_bootstrap.c
Outdated
@@ -95,6 +95,44 @@ static void manage_keys(DHT *dht) | |||
fclose(keys_file); | |||
} | |||
|
|||
void print_log(void *context, LOGGER_LEVEL level, const char *file, int line, |
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.
This function is now replicated 3 times in our codebase. Can we make that only 1 and include it in all 3 places?
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.
I found only 2 print_log()
.
Where's the 3rd?
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.
Found the 3rd, in IRC chat. note it here.
c-toxcore/auto_tests/helpers.h
Line 48 in ae7899c
static void print_debug_log(Tox *m, TOX_LOG_LEVEL level, const char *file, uint32_t line, const char *func, |
Note that tox-bootstrapd has two logging options
The way you have added toxcore logging to tox-bootstrapd makes it always be sent to stderr. I think that debug/trace logging should use the same logging backend that the rest of tox-bootstrapd does, so that when user makes it log to syslog, debug/trace output is logged to syslog too. |
|
Added a commit that does proper logging of toxcore's logs in tox-bootstrapd. Also removed 2 of your "Merge branch 'master' into bootstrap-logger" commits and instead rebased your branch on top of the master (no conflicts were found), as that's what you should have done instead of the merges. |
Why do we want to log only if Also, isn't that check redundant? Doesn't logger check for |
Hm, I see. By default toxcore uses |
char *strlevel; | ||
LOG_LEVEL log_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.
Can we use the already exist argument level
here? So that we don't need to declare a new variable.
My bad. The level
is in type LOGGER_LEVEL
not LOG_LEVEL
. Sorry for bother.
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.
Right, LOGGER_LEVEL
is toxcore's log level from toxcore/logger.h
, but LOG_LEVEL
is tox-bootstrap's log level from other/bootstrap_daemon/src/log.h
.
Hi @nurupo,
Yes.
Thank you, nurupo :-) I made this "mistake" by click on "Update branch" button on GitHub web. Which I want to make a response to @iphydf that I've read his message. I need more time to find the suitable file to place the logger function. That's why I didn't commit yet. BTW, your c1bd6d0 is every cool :) |
c1bd6d0 is your commit though, I have made no changes to the code contents of it, only metadata has changed. It has changed the hash, added me as the commiter (and still has you as the author) and got my gpg signature because I have rebased it on top of the master branch. Feel free to squash commits before getting this PR merged. I kept your original commit in case there was some disagreement about the modification I have done, which is very easy to undo if it's in its own separate commit on top of your separate commit, and because it's easier to see what I have modified exactly that way. |
Is this done? @cotox did you deduplicate the functions? |
b4eb7af
to
6e0f94c
Compare
Was it rebased on master? Afaik there was a refactor in the toxcore's logger, so logger constant names and such would need to be changed. |
b4eb7af
to
9b56841
Compare
I think I have fixed the build now (it's still building...). The deduplication issue is still not addressed though. |
Huh, I'm confused. I thought I had made changes and pushed them, but apparently not? Will redo it again. |
9b56841
to
d9b4148
Compare
Should be good now. |
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.
Reviewable status: complete! 1 of 1 approvals obtained (waiting on @cotox and @nurupo)
other/DHT_bootstrap.c, line 97 at r4 (raw file):
} static void print_log(void *context, LOGGER_LEVEL level, const char *file, int line,
Logger_Level
other/bootstrap_daemon/src/tox-bootstrapd.c, line 183 at r4 (raw file):
// Logs toxcore logger message using our logger facility static void toxcore_logger_callback(void *context, LOGGER_LEVEL level, const char *file, int line,
Logger_Level
other/bootstrap_daemon/src/tox-bootstrapd.c, line 186 at r4 (raw file):
const char *func, const char *message, void *userdata) { char *strlevel;
I don't think we need strlevel, given that we translate it to a LOG_LEVEL already. Yes, we lose a bit of information on whether it's debug or info or trace, but I think that's acceptable.
968b7d6
to
ad3d1c9
Compare
Codecov Report
@@ Coverage Diff @@
## master #895 +/- ##
========================================
- Coverage 82.7% 82.5% -0.2%
========================================
Files 82 81 -1
Lines 14478 14437 -41
========================================
- Hits 11980 11924 -56
- Misses 2498 2513 +15
Continue to review full report at Codecov.
|
ad3d1c9
to
1954501
Compare
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.
Reviewed 1 of 2 files at r5, 1 of 1 files at r6.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @cotox and @nurupo)
other/bootstrap_daemon/src/tox-bootstrapd.c, line 186 at r4 (raw file):
Previously, iphydf wrote…
I don't think we need strlevel, given that we translate it to a LOG_LEVEL already. Yes, we lose a bit of information on whether it's debug or info or trace, but I think that's acceptable.
@nurupo what do you think?
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.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @cotox and @nurupo)
other/bootstrap_daemon/src/tox-bootstrapd.c, line 186 at r4 (raw file):
Previously, iphydf wrote…
@nurupo what do you think?
It depends on whether you want to know the [LOG|LOGGER]_LEVEL_
of log messages when users eventually submit bug reports with log messages to toxcore. By default LOG_LEVEL
information is not available when reading the log text, it gets stripped down at the log filtering stage.
rsyslog uses the log level information primary for filtering log messages when creating custom logging rules, but by default, at least on Debian, log messages are not prefixed with the log level or anything like that. By default, all of tox-bootstrapd's log messages will be written to /var/log/syslog
file with the log level information being lost. You could, of course, create a custom rsyslog config file that tells rsyslog to prefix log messages with the log level or split which log levels get written to which files, e.g. write tox-boostrapd errors to /var/log/tox-bootstrapd-errors.log
and such. However, this is something tox-bootstrapd user would need to configure themselves if they want to, this won't happen in a default setup, so expect the bug reports you see not to contain any log level information in them.
stdout backend is similar, it splits log level in stdout and stderr, so best you can distinguish between INFO (stdout) and WARNING/ERROR (stderr), but that's only if you redirected two of the two streams into different places, otherwise you get them all together with no way to tell what log level they are.
I think we are the ones who will get affected by the change the most. Users wouldn't be affected by either lack or presence of strlevel
, as they will be able to filter the logs with rsyslog/stdout backends in any case. So whether we want strlevel
or not depends on whether the log/logger level information is useful for us when a tox-bootstrapd user sends their logs in a bug report.
Btw, strlevel
has been removed right after you made that comment 6 days ago. I can re-add it if you want.
Feel free to merge. |
1ea6e43
to
6d83572
Compare
Don't know why codes with macro dosen't work. As it's only a few expensive, just code it without macro for now. \#if (MIN_LOGGER_LEVEL == LOG_TRACE) || (MIN_LOGGER_LEVEL == LOG_DEBUG) fprintf(stderr, "[%s] %s:%d(%s) %s\n", strlevel, file, line, func, message); \#endif
6d83572
to
2e4cae6
Compare
A meanful logger for bootstrap. It outputs trace or debug log only.
Build with default options, the bootstrap outputs nothing more.
Build with
DEBUG=ON
,tox-bootstrapd
outputs as,Build with
TRACE=ON
,tox-bootstrapd
outputs as,This change is