Date: Fri, 23 Apr 2010 13:17:54 +1000 (EST)
From: nathans@aconex.com
Sender: nscott@aconex.com
To: kenj@internode.on.net
Cc: pcp <pcp@oss.sgi.com>
Message-ID: <1988458083.206861271992673963.JavaMail.root@mail-au.aconex.com>
In-Reply-To: <2077613103.206821271992571566.JavaMail.root@mail-au.aconex.com>
Subject: Re: Dynamic names issue
MIME-Version: 1.0
Content-Type: multipart/mixed; 
	boundary="----=_Part_15295_957658181.1271992673960"

------=_Part_15295_957658181.1271992673960
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit


----- "Ken McDonell" <kenj@internode.on.net> wrote:

> The first time a pmid appears in a pmFetch, pmlogger calls pmNameAll
> to get all of the names of the metric from the pmns to build the pmns
> fragment in the archive ... at this point the only name that can be
> found for pmid 2.4.1 is pmcd.agent.status, because the other "name"
> is not really in the pmns that is available to pmcd, and is not even
> "owned" by the PMDA with domain 2.
> 
> I don't see a simple way to prevent this happening ...

How's this patch look?  It has the added bonus that it removes two
round trip PDUs per metric logged (hmmmm ... wonder if this is why
some of my remote loggers take so long to start!).  The main open
question is whether the (existing) task cache is ever invalid at
the time that initial callback happens... there's some hairy code
in there, but fortunately the guy who wrote it is right here.  :)

$ cat tmp/config
log advisory on 1sec {
        sample.secret.foo.bar.max.redirect
}
$ pminfo -a tmp/archive
sample.secret.foo.bar.max.redirect
pmcd.pmlogger.archive
pmcd.pmlogger.port
pmcd.pmlogger.host
$ 

Thoughts?  Seems like its much improved this way, I'm wondering why
it wasn't done like this before!?  (IOW, what am I missing?)

cheers.

-- 
Nathan

------=_Part_15295_957658181.1271992673960
Content-Type: text/x-patch; name=pmlogger.patch
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment; filename=pmlogger.patch

diff --git a/src/pmlogger/callback.c b/src/pmlogger/callback.c
index f857d40..4eec34a 100644
--- a/src/pmlogger/callback.c
+++ b/src/pmlogger/callback.c
@@ -294,6 +294,16 @@ check_inst(pmValueSet *vsp, int hint, pmResult *lrp)
     return 0;
 }
 
+static int
+lookupTaskCache(task_t *tp, pmID pmid)
+{
+    int i;
+    for (i = 0; i < tp->t_numpmid; i++) {
+	if (tp->t_pmidlist[i] == pmid)
+	    return i;
+    }
+    return -1;
+}
 
 void
 log_callback(int afid, void *data)
@@ -466,27 +476,28 @@ log_callback(int afid, void *data)
 	for (i = 0; i < resp->numpmid; i++) {
 	    pmValueSet	*vsp = resp->vset[i];
 	    pmDesc	desc;
-	    char	**names = NULL;
+	    char	*names = NULL;
 	    int		numnames = 0;
+
 	    sts = __pmLogLookupDesc(&logctl, vsp->pmid, &desc);
 	    if (sts < 0) {
-		if (archive_version == PM_LOG_VERS02) {
-		    if ((numnames = pmNameAll(vsp->pmid, &names)) < 0) {
-		        fprintf(stderr, "pmNameAll: %s\n", pmErrStr(numnames));
-		        exit(1);
-		    }
-		}
-		if ((sts = pmLookupDesc(vsp->pmid, &desc)) < 0) {
-		    fprintf(stderr, "pmLookupDesc: %s\n", pmErrStr(sts));
+		/* lookup name and descriptor in task cache */
+		int taskindex = lookupTaskCache(tp, vsp->pmid);
+		if (taskindex == -1) {
+		    fprintf(stderr, "lookupTaskCache cannot find PMID %s\n",
+				pmIDStr(vsp->pmid));
 		    exit(1);
 		}
-		if ((sts = __pmLogPutDesc(&logctl, &desc, numnames, names)) < 0) {
+		if (archive_version == PM_LOG_VERS02) {
+		    numnames = 1;
+		    names = tp->t_namelist[taskindex];
+		}
+		desc = tp->t_desclist[taskindex];
+
+		if ((sts = __pmLogPutDesc(&logctl, &desc, numnames, &names)) < 0) {
 		    fprintf(stderr, "__pmLogPutDesc: %s\n", pmErrStr(sts));
 		    exit(1);
 		}
-		if (names != NULL) {
-		    free(names);
-                }
 	    }
 	    if (desc.indom != PM_INDOM_NULL && vsp->numval > 0) {
 		/*
@@ -605,7 +616,7 @@ log_callback(int afid, void *data)
     }
 
     if (rflag && tp->t_size == 0 && pdu_metrics > 0) {
-	char	**names = NULL;
+	char	*names = NULL;
 
 	tp->t_size = pdu_bytes;
 
@@ -614,27 +625,26 @@ log_callback(int afid, void *data)
 	else
 	    fprintf(stderr, "\nMetric ");
 	if (archive_version == PM_LOG_VERS02) {
-	    if (pmNameAll(pdu_first_pmid, &names) < 0)
-		names = NULL;
-	}
-	if (names != NULL) {
-	    fprintf(stderr, "%s", names[0]);
-	    free(names);
+	    int taskindex = lookupTaskCache(tp, pdu_first_pmid);
+	    if (tp->t_namelist[taskindex] != NULL)
+		names = tp->t_namelist[taskindex];
 	}
+	if (names != NULL)
+	    fprintf(stderr, "%s", names);
 	else
 	    fprintf(stderr, "%s", pmIDStr(pdu_first_pmid));
 	if (pdu_metrics > 1) {
 	    fprintf(stderr, "\n\t");
 	    if (pdu_metrics > 2)
 		fprintf(stderr, "...\n\t");
+	    names = NULL;
 	    if (archive_version == PM_LOG_VERS02) {
-		if (pmNameAll(pdu_last_pmid, &names) < 0)
-		    names = NULL;
-	    }
-	    if (names != NULL) {
-		fprintf(stderr, "%s", names[0]);
-		free(names);
+		int taskindex = lookupTaskCache(tp, pdu_last_pmid);
+		if (tp->t_namelist[taskindex] != NULL)
+		    names = tp->t_namelist[taskindex];
 	    }
+	    if (names != NULL)
+		fprintf(stderr, "%s", names);
 	    else
 		fprintf(stderr, "%s", pmIDStr(pdu_last_pmid));
 	    fprintf(stderr, "\n}");

------=_Part_15295_957658181.1271992673960--