The principle of this is fine, but the proposed implementation is not satisfactory. It is too complex for a C macro. More importantly, it is very inefficient as the code expansion would be repeated thousands of times.

Although making extensive changes may be laborious, it's not clear that there is a better solution. There are also questions about whether logging ought to be less implementation specific, so as to allow possible alternative mechanisms.

Please feel free to implement with better code. I proposed macro - so we do not have to do extensive code change across the board.

Right now with our logging, we cannot quickly pin point exact location of an error - So which ever way you implement - please implement it, as long as source code file name and line number are included in log messages.

It will take time. And the macro wouldn't work, for a variety of reasons. One that worked would be rather more complex.

There really are strong reasons for not doing this without extensive consideration of a range of possible solutions and their implications. It also seems unwise to embark on this without looking at wider questions about the logging system. The macro proposal has serious drawbacks, apart from not working. It is not something that should be done without proper discussion by the full development team.

It is not going to be done using macro. Markus will present his design to team and implement in time for 1.2.1.

markus makela Please update the estimate of this task.

The current plan for this is to use function name instead of file and line. This will keep the output more compact and will allow inspection of the code without requiring the same source code versions.

As long as function name is unique across all the source code files in MaxScale it is ok. All though within function - there should not be same message logged more than once , or there should be some way of uniquely differentiating them in the logged message.

In any case - you still need to go to the same release-branch from which the binary was built in order to see where the message logged from - as function could have changed from release to release.

Having file, line and/or function information would be valuable, so why not use a simple macro approach like?

{{
#define SKYGW_LOG_WRITE(id, format, ...) \
do {\
if (LOG_IS_ENABLED(id))

\
while (0)
}}

with one additional function that formats the file/line/function information as deemed appropriate. And the same for the flush version.

And then all skygw_log_write calls would have to be changed into SKYGW_LOG_WRITE calls. Or perhaps better to rename to MS_LOG_WRITE and/or possibly provide MS_ERROR (and friends) that log the relevant log.

This is now fixed using macros so that there is a trailing [functionname] on each logged line. Together with the commit id that is logged along with crashes and that MaxScale is capable of telling when invoked, that is sufficient to be able to pinpoint the exact location where a message is logged (unless exactly the same message is logged in several different locations inside a function). The function name, while still meaningful only for the developer, is less intrusive and confusing for an end-user (well, dba).

johan.wikman Can this be moved to "In Review" status ?

Yes.

It's actually in Markus' and Martin's review queue at the moment.

Johan

johan.wikman You can click on "Request Review" action button at the top - to show the status change to "In Review" here in Jira