[Gluster-devel] Glusterfs and the new logging framework

Mon Apr 28 09:53:00 UTC 2014

Hi,

I would recommend that we do not require the same string to be reused ((though dev is free to if they want to ) but that the correct id be used and the dev use a string that contains the  appropriate details for his scenario.The documentation would have a detailed explanation of the msgid and the scenarios in which it is likely to occur. Having the msgid coupled with the string reduces code readability.

I'm wondering if it is required for the document to actually contain the actual message string - that should be fairly easy for the admin to understand anyway. What would be useful is more info as to probable causes and solutions - something not captured in out logs. That is something that can be well explained in documentation without requiring the exact string to be captured.

Regards,
Nithya

----- Original Message -----
From: "Krutika Dhananjay" <kdhananj at redhat.com>
To: "Nithya Balachandran" <nbalacha at redhat.com>
Cc: gluster-devel at gluster.org
Sent: Monday, April 28, 2014 3:12:56 PM
Subject: Re: [Gluster-devel] Glusterfs and the new logging framework

----- Original Message -----

> From: "Raghavendra Gowdappa" <rgowdapp at redhat.com>
> To: "Nithya Balachandran" <nbalacha at redhat.com>
> Cc: gluster-devel at gluster.org
> Sent: Monday, April 28, 2014 11:39:50 AM
> Subject: Re: [Gluster-devel] Glusterfs and the new logging framework

> (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);
> >
I am guessing the original author of the new logging framework decided to retain the msg string (like "Failed to get cached subvol for %s") 
in the header file due to the ease with which the one-to-one mapping between the msg id and the message itself can be captured using scripts/tools 
which may be needed for documentation - something that becomes difficult with the proposed approach. 

Even from development perspective, it is much easier for a developer adding new code (which means many more calls to gf_msg()) 
to merely look up one single place (which would be the messages header file for that component) to determine whether the message he/she is about to log 
is already in use (for example to know whether DHT_MSG_1 can be reused in their changes), rather than having to look up all files in that component. 

-Krutika 

> >
> > 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