[Gluster-devel] Glusterfs and the new logging framework

Mon Apr 28 08:05:41 UTC 2014

Hi,

We should definitely have descriptive names but linking to msg strings can still cause us to use up a lot of ids. For example:

DHT_LOOKUP_FAILED_WITH_STRERR	        (BUMP_MSG_ID), "%s: failed to lookup file (%s)"
DHT_LOOKUP_FAILED_WITH_NODE_DETAIL	(BUMP_MSG_ID), "%s: failed to lookup file on %s"
DHT_LOOKUP_FAILED_WITH_NODE_AND_ERR_DETAIL	(BUMP_MSG_ID), "%s: failed to lookup file on %s (%s)"

etc can still be rather confusing and does not really provide additional information from the doc point of view.

It might be better to just stick to

#define DHT_LOOKUP_FAILED (MSG_ID) 

and allow the dev to define the required string in each case.

The DHT_LOOKUP_FAILED message id will be part of the log message and that alone can be documented . 
Eg:
DHT_LOOKUP_FAILED
Value : 10234
Description: The DHT lookup failed etc etc...
Cause: The lookup can fail for the following reasons:
...
Possible actions:
Check if the subvolume is up...
etc etc

The admin can search for the particular msg id (Eg: 10234)in the logs and get additional info for the same instead of trying to match a particular string.

The other question is whether log messages need to be localized - my understanding is that they are mainly intended for dev and sys admins who are assumed to know sufficient english to be able to use them. I am not aware of any projects that localize logs( but could be wrong so please point me to any you know of). Localizing log messages would make things difficult for a developer in case s/he is sent ,say,a French log file but does not know French- trying to figure out what went wrong would be very difficult. 

The point for localization is very valid and necessary for UI like management consoles etc but perhaps not for a log file.

Regards,
Nithya

----- Original Message -----
From: "Kaushal M" <kshlmster at gmail.com>
To: gluster-devel at gluster.org
Cc: "Nithya Balachandran" <nbalacha at redhat.com>
Sent: Monday, April 28, 2014 1:15:32 PM
Subject: Re: [Gluster-devel] Glusterfs and the new logging framework

Why not just have the macro names be meaningful? This would help a lot
for a developer who is reading the code.
For example, instead of a macro of the format
 #define something_msg_1 (BUMP_MSG_ID) ''Some message string''
have it as
 #define descriptive_name (BUMP_MSG_ID) "Some message string"

This should help the developers, while keeping the i18n concerns satisfied.

~kaushal

On Mon, Apr 28, 2014 at 11:39 AM, Raghavendra Gowdappa
<rgowdapp at redhat.com> wrote:
> (Moving the discussion to gluster-devel).
>
> ----- Original Message -----
>> From: "Nagaprasad Sathyanarayana" <nsathyan at redhat.com>
>> To: "Nithya Balachandran" <nbalacha at redhat.com>
>> Cc: "Raghavendra Gowdappa" <rgowdapp at redhat.com>, "Susant Palai" <spalai at redhat.com>, "Atin Mukherjee"
>> <amukherj at redhat.com>, "Varun Shastry" <vshastry at redhat.com>, "Krishnan Parthasarathi" <kparthas at redhat.com>,
>> "Ravishankar Narayanankutty" <ranaraya at redhat.com>, "Pranith Kumar Karampuri" <pkarampu at redhat.com>, "Venkatesh
>> Somyajulu" <vsomyaju at redhat.com>
>> Sent: Monday, April 28, 2014 10:53:55 AM
>> Subject: Re: Glusterfs and the new logging framework
>>
>> It is a very valuable suggestion Nithya.  Using descriptive message IDs
>> (DHT_GET_CACHED_SUBVOL_FAILED in place of dht_msg_1) is definitely helpful
>> to developers and improves the code readability.  However, IMO, keeping the
>> actual formatted message string in a header file is a good practice.  Some
>> of the reasons for it are;
>>
>> A> It helps in internationalization and localization at later point.
>> B> Any changes to the message & the formatting is easy to handle when it is
>> not embedded in the log function call.
>>
>> Please discuss this with wider audience, including Vijay & Alok, before
>> finalizing the approach.
>>
>> Thanks
>> Naga
>>
>> ----- Original Message -----
>> From: "Nithya Balachandran" <nbalacha at redhat.com>
>> To: "Raghavendra Gowdappa" <rgowdapp at redhat.com>, "Susant Palai"
>> <spalai at redhat.com>, "Atin Mukherjee" <amukherj at redhat.com>, "Varun Shastry"
>> <vshastry at redhat.com>, "Krishnan Parthasarathi" <kparthas at redhat.com>,
>> "Ravishankar Narayanankutty" <ranaraya at redhat.com>, "Pranith Kumar
>> Karampuri" <pkarampu at redhat.com>, "Venkatesh Somyajulu"
>> <vsomyaju at redhat.com>
>> Cc: "Nagaprasad Sathyanarayana" <nsathyan at redhat.com>
>> Sent: Monday, April 28, 2014 9:16:37 AM
>> Subject: Glusterfs and the new logging framework
>>
>> Hi,
>>
>> I am currently working on porting the DHT messages to the new logging
>> framework and I have some observations/concerns about the approach on the
>> same:
>>
>> 1. With the current approach, a call to gf_msg would look like:
>>
>> gf_msg ("dht", GF_LOG_ERROR, 0, dht_msg_1, param1);
>>
>> This provides little information to the developer when going through the
>> code. It becomes tedious to lookup the value of dht_msg_4 each time. This
>> also reduces code readability as the format string is not available
>> instantly and can potentially cause issues with incorrect parameters being
>> passed to the call.
>>
>>
>> 2. Adding new messages can potentially cause us to use up the chunk of
>> message ids quickly. A better approach would be to decouple the is
>> definition from the actual message string.There are be several potential
>> messages for a single operation state.Message formats might change in the
>> future as we add enhancements - for eg, we shouldn't have to add new
>> messages with new message ids to accommodate an additional parameter if we
>> change a struct definition, say. Defining some sort of "state ids" however
>> will take up fewer ids.
>>
>> Eg: The code has the following messages :
>>
>> "Failed to get cached subvol for %s"
>> "Failed to get cached subvol for %s on %s"
>>
>>
>> Both the above will require separate msg-ids with the current approach. The
>> difference in the strings is minimal and does not really provide any
>> additional information from the Doxygen doc point of view.
>>
>> Another approach would be to define a DHT_GET_CACHED_SUBVOL_FAILED id which
>> can be passed to the msglog, while the actual message string can change
>> depending on what the developer thinks will be helpful. This will also
>> improve code readability.
>>
>> So I propose that we do the following:
>>
>>
>> Current approach:
>>
>> Messages file:
>>
>> #define dht_msg_1 (GLFS_COM_BASE + 1) , "Failed to get cached subvol for %s"
>> #define dht_msg_2 (GLFS_COM_BASE + 2) , "Failed to get cached subvol for %s
>> on %s"
>> #define dht_msg_3 (GLFS_COM_BASE + 3) , "Failed to get hashed subvol " ...etc
>> etc
>>
>> Code :
>> gf_msg ("dht", GF_LOG_ERROR, 0, dht_msg_1, param1);
>>
>> gf_msg ("dht", GF_LOG_ERROR, 0, dht_msg_2, param1, param2);
>>
>> The above is confusing for a developer.
>>
>>
>> Proposed approach:
>>
>> #define DHT_GET_CACHED_SUBVOL_FAILED (GLFS_COM_BASE + 1)
>> #define DHT_GET_HASHED_SUBVOL_FAILED (GLFS_COM_BASE + 2)
>>
>> gf_msg ("dht", GF_LOG_ERROR, 0, DHT_GET_CACHED_SUBVOL_FAILED, "Failed to get
>> cached subvol for %s on %s" , param1, param2);
>>
>> gf_msg ("dht", GF_LOG_ERROR, 0, DHT_GET_CACHED_SUBVOL_FAILED, "Failed to get
>> cached subvol for %s " , param1);
>>
>>
>> Advantages:
>> Much clearer code - developer can use generated id or define new one easily.
>> Related ids can be grouped together in the header file.
>> Fewer ids required
>> No changes to the logging framework
>> Messages can change later
>> The approach of using and id and generating a doxygen doc for the same still
>> holds good - we are simply decoupling the actual string from the definition.
>> So the doc would still have the message id and a description of the
>> condition it specifies without tying it to a string that might change later.
>>
>>
>> Thoughts?
>>
>> Regards,
>> Nithya
>>
>>
>>
>>
>>
> _______________________________________________
> Gluster-devel mailing list
> Gluster-devel at gluster.org
> http://supercolony.gluster.org/mailman/listinfo/gluster-devel