[PATCH] debuginfod: PR28242 - extend server http-response metrics

[PATCH] debuginfod: PR28242 - extend server http-response metrics

[Di Chen]

From a574dfaf5d5636cbb7159a0118eb30e2c4aaa301 Mon Sep 17 00:00:00 2001
From: Di Chen <dichen@redhat.com>
Date: Fri, 1 Oct 2021 22:03:41 +0800
Subject: [PATCH] debuginfod: PR28242 - extend server http-response metrics

This patch aims to extend http_responses_* metrics with another label
"type" by getting the extra artifact-type content added as a new key=value
tag.

https://sourceware.org/bugzilla/show_bug.cgi?id=28242

Signed-off-by: Di Chen <dichen@redhat.com>
---
 debuginfod/debuginfod.cxx              | 55 ++++++++++++++++++++------
 tests/run-debuginfod-000-permission.sh | 12 +++---
 2 files changed, 49 insertions(+), 18 deletions(-)

diff --git a/debuginfod/debuginfod.cxx b/debuginfod/debuginfod.cxx
index 2b9a1c41..6a469b21 100644
--- a/debuginfod/debuginfod.cxx
+++ b/debuginfod/debuginfod.cxx
@@ -2080,6 +2080,33 @@ add_metric(const string& metric,

 // and more for higher arity labels if needed

+static void
+inc_metric(const string& metric,
+           const string& lname, const string& lvalue,
+           const string& rname, const string& rvalue)
+{
+  string key = (metric + "{" + metric_label(lname, lvalue) + ",");
+  if (rvalue != "debuginfo" && rvalue != "executable" && rvalue !=
"source")
+    key = (key + metric_label(rname, "") + "}");
+  else
+    key = (key + metric_label(rname, rvalue) + "}");
+  unique_lock<mutex> lock(metrics_lock);
+  metrics[key] ++;
+}
+static void
+add_metric(const string& metric,
+           const string& lname, const string& lvalue,
+           const string& rname, const string& rvalue,
+           double value)
+{
+  string key = (metric + "{" + metric_label(lname, lvalue) + ",");
+  if (rvalue != "debuginfo" && rvalue != "executable" && rvalue !=
"source")
+    key = (key + metric_label(rname, "") + "}");
+  else
+    key = (key + metric_label(rname, rvalue) + "}");
+  unique_lock<mutex> lock(metrics_lock);
+  metrics[key] += value;
+}

 static struct MHD_Response*
 handle_metrics (off_t* size)
@@ -2165,6 +2192,7 @@ handler_cb (void * /*cls*/,
   struct timespec ts_start, ts_end;
   clock_gettime (CLOCK_MONOTONIC, &ts_start);
   double afteryou = 0.0;
+  string artifacttype, suffix;

   try
     {
@@ -2203,7 +2231,7 @@ handler_cb (void * /*cls*/,
           string buildid = url_copy.substr(slash1+1, slash2-slash1-1);

           size_t slash3 = url_copy.find('/', slash2+1);
-          string artifacttype, suffix;
+
           if (slash3 == string::npos)
             {
               artifacttype = url_copy.substr(slash2+1);
@@ -2275,17 +2303,20 @@ handler_cb (void * /*cls*/,
   // related prometheus metrics
   string http_code_str = to_string(http_code);
   if (http_size >= 0)
-    add_metric("http_responses_transfer_bytes_sum","code",http_code_str,
-               http_size);
-  inc_metric("http_responses_transfer_bytes_count","code",http_code_str);
-
-
 add_metric("http_responses_duration_milliseconds_sum","code",http_code_str,
-             deltas*1000); // prometheus prefers _seconds and floating
point
-
 inc_metric("http_responses_duration_milliseconds_count","code",http_code_str);
-
-
 add_metric("http_responses_after_you_milliseconds_sum","code",http_code_str,
-             afteryou*1000);
-
 inc_metric("http_responses_after_you_milliseconds_count","code",http_code_str);
+    add_metric("http_responses_transfer_bytes_sum",
+               "code", http_code_str, "type", artifacttype, http_size);
+  inc_metric("http_responses_transfer_bytes_count",
+             "code", http_code_str, "type", artifacttype);
+
+  add_metric("http_responses_duration_milliseconds_sum",
+             "code", http_code_str, "type", artifacttype, deltas*1000); //
prometheus prefers _seconds and floating point
+  inc_metric("http_responses_duration_milliseconds_count",
+             "code", http_code_str, "type", artifacttype);
+
+  add_metric("http_responses_after_you_milliseconds_sum",
+             "code", http_code_str, "type", artifacttype, afteryou*1000);
+  inc_metric("http_responses_after_you_milliseconds_count",
+             "code", http_code_str, "type", artifacttype);

   return rc;
 }
diff --git a/tests/run-debuginfod-000-permission.sh
b/tests/run-debuginfod-000-permission.sh
index 1e92bdb8..afa7e931 100755
--- a/tests/run-debuginfod-000-permission.sh
+++ b/tests/run-debuginfod-000-permission.sh
@@ -61,22 +61,22 @@ if [ -r $DEBUGINFOD_CACHE_PATH/01234567/debuginfo ];
then
   err
 fi

-bytecount_before=`curl -s http://127.0.0.1:$PORT1/metrics | grep
'http_responses_transfer_bytes_count{code="404"}'`
+bytecount_before=`curl -s http://127.0.0.1:$PORT1/metrics | grep
'http_responses_transfer_bytes_count{code="404",type="debuginfo"}'`
 testrun ${abs_top_builddir}/debuginfod/debuginfod-find debuginfo 01234567
|| true
-bytecount_after=`curl -s http://127.0.0.1:$PORT1/metrics | grep
'http_responses_transfer_bytes_count{code="404"}'`
+bytecount_after=`curl -s http://127.0.0.1:$PORT1/metrics | grep
'http_responses_transfer_bytes_count{code="404",type="debuginfo"}'`
 if [ "$bytecount_before" != "$bytecount_after" ]; then
-  echo "http_responses_transfer_bytes_count{code="404"} has changed."
+  echo "http_responses_transfer_bytes_count{code="404",type="debuginfo"}
has changed."
   err
 fi

 # set cache_miss_s to 0 and sleep 1 to make the mtime expire.
 echo 0 > $DEBUGINFOD_CACHE_PATH/cache_miss_s
 sleep 1
-bytecount_before=`curl -s http://127.0.0.1:$PORT1/metrics | grep
'http_responses_transfer_bytes_count{code="404"}'`
+bytecount_before=`curl -s http://127.0.0.1:$PORT1/metrics | grep
'http_responses_transfer_bytes_count{code="404",type="debuginfo"}'`
 testrun ${abs_top_builddir}/debuginfod/debuginfod-find debuginfo 01234567
|| true
-bytecount_after=`curl -s http://127.0.0.1:$PORT1/metrics | grep
'http_responses_transfer_bytes_count{code="404"}'`
+bytecount_after=`curl -s http://127.0.0.1:$PORT1/metrics | grep
'http_responses_transfer_bytes_count{code="404",type="debuginfo"}'`
 if [ "$bytecount_before" == "$bytecount_after" ]; then
-  echo "http_responses_transfer_bytes_count{code="404"} should be
incremented."
+  echo "http_responses_transfer_bytes_count{code="404",type="debuginfo"}
should be incremented."
   err
 fi

-- 
2.31.1

Re: [PATCH] debuginfod: PR28242 - extend server http-response metrics

[Mark Wielaard]

Hi,

On Fri, 2021-10-01 at 22:05 +0800, Di Chen via Elfutils-devel wrote:
> From a574dfaf5d5636cbb7159a0118eb30e2c4aaa301 Mon Sep 17 00:00:00
> 2001
> From: Di Chen <dichen@redhat.com>
> Date: Fri, 1 Oct 2021 22:03:41 +0800
> Subject: [PATCH] debuginfod: PR28242 - extend server http-response
> metrics

BTW. If possible use git send-email HEAD^ or attach the result of git
format-patch HEAD^ which will make sure some whitespace/linebreaks pass
through the mailinglist better.

> This patch aims to extend http_responses_* metrics with another label
> "type" by getting the extra artifact-type content added as a new
> key=value
> tag.
> 
> https://sourceware.org/bugzilla/show_bug.cgi?id=28242

I believe the patch does what it says, the locking seems good and the
testcases have been adapted. Nice work. It is missing a ChangeLog entry
which would have helped a little with review.

But I would like someone else (Frank?) who knows more about the metrics
to take a quick peek before pushing.

Thanks,

Mark

Re: [PATCH] debuginfod: PR28242 - extend server http-response metrics

[Frank Ch. Eigler]

Hi -

> But I would like someone else (Frank?) who knows more about the metrics
> to take a quick peek before pushing.

Thanks, pushed with a couple of small tweaks.

- FChE