Merge branch 'tor-gitlab/mr/697'

changes/ticket40758

@@ -0,0 +1,3 @@
+  o Minor features (metrics):
+    - Add a `reason` label to the HS error metrics.
+      Closes ticket 40758.

src/core/or/circuituse.c

@@ -1753,7 +1753,8 @@ circuit_build_failed(origin_circuit_t *circ)
 
     /* If the path failed on an RP, retry it. */
     if (TO_CIRCUIT(circ)->purpose == CIRCUIT_PURPOSE_S_CONNECT_REND) {
-      hs_metrics_failed_rdv(&circ->hs_ident->identity_pk);
+      hs_metrics_failed_rdv(&circ->hs_ident->identity_pk,
+                            HS_METRICS_ERR_RDV_PATH);
       hs_circ_retry_service_rendezvous_point(circ);
     }
 
@@ -1866,7 +1867,8 @@ circuit_build_failed(origin_circuit_t *circ)
                escaped(build_state_get_exit_nickname(circ->build_state)),
                failed_at_last_hop?"last":"non-last");
 
-      hs_metrics_failed_rdv(&circ->hs_ident->identity_pk);
+      hs_metrics_failed_rdv(&circ->hs_ident->identity_pk,
+                            HS_METRICS_ERR_RDV_RP_CONN_FAILURE);
       hs_circ_retry_service_rendezvous_point(circ);
       break;
     /* default:

src/feature/hs/hs_circuit.c

@@ -494,7 +494,10 @@ retry_service_rendezvous_point(const origin_circuit_t *circ)
   if (new_circ == NULL) {
     log_warn(LD_REND, "Failed to launch rendezvous circuit to %s",
              safe_str_client(extend_info_describe(bstate->chosen_exit)));
-    hs_metrics_failed_rdv(&circ->hs_ident->identity_pk);
+
+    hs_metrics_failed_rdv(&circ->hs_ident->identity_pk,
+                          HS_METRICS_ERR_RDV_RETRY);
+
     goto done;
   }
 
@@ -832,7 +835,6 @@ hs_circ_service_rp_has_opened(const hs_service_t *service,
 {
   size_t payload_len;
   uint8_t payload[RELAY_PAYLOAD_SIZE] = {0};
-  int reason = 0;
 
   tor_assert(service);
   tor_assert(circ);
@@ -864,33 +866,34 @@ hs_circ_service_rp_has_opened(const hs_service_t *service,
     payload_len = HS_LEGACY_RENDEZVOUS_CELL_SIZE;
   }
 
-  if ((reason = relay_send_command_from_edge(CONTROL_CELL_ID, TO_CIRCUIT(circ),
+  if (relay_send_command_from_edge(CONTROL_CELL_ID, TO_CIRCUIT(circ),
                                    RELAY_COMMAND_RENDEZVOUS1,
                                    (const char *) payload,
                                    payload_len,
-                                             circ->cpath->prev)) < 0) {
+                                   circ->cpath->prev) < 0) {
     /* On error, circuit is closed. */
     log_warn(LD_REND, "Unable to send RENDEZVOUS1 cell on circuit %u "
                       "for service %s",
              TO_CIRCUIT(circ)->n_circ_id,
              safe_str_client(service->onion_address));
+
+    hs_metrics_failed_rdv(&service->keys.identity_pk,
+                          HS_METRICS_ERR_RDV_RENDEZVOUS1);
     goto done;
   }
 
   /* Setup end-to-end rendezvous circuit between the client and us. */
-  if ((reason = hs_circuit_setup_e2e_rend_circ(circ,
+  if (hs_circuit_setup_e2e_rend_circ(circ,
                        circ->hs_ident->rendezvous_ntor_key_seed,
                        sizeof(circ->hs_ident->rendezvous_ntor_key_seed),
-                       1)) < 0) {
+                       1) < 0) {
     log_warn(LD_GENERAL, "Failed to setup circ");
+
+    hs_metrics_failed_rdv(&service->keys.identity_pk, HS_METRICS_ERR_RDV_E2E);
     goto done;
   }
 
  done:
-  if (reason < 0) {
-    hs_metrics_failed_rdv(&service->keys.identity_pk);
-  }
-
   memwipe(payload, 0, sizeof(payload));
 }
 
@@ -1004,12 +1007,15 @@ hs_circ_handle_introduce2(const hs_service_t *service,
   data.replay_cache = ip->replay_cache;
   data.cc_enabled = 0;
 
-  if (get_subcredential_for_handling_intro2_cell(service,
+  if (get_subcredential_for_handling_intro2_cell(service, &data,
-                                                 &data, subcredential)) {
+                                                 subcredential)) {
+    hs_metrics_reject_intro_req(service,
+                                HS_METRICS_ERR_INTRO_REQ_SUBCREDENTIAL);
     goto done;
   }
 
   if (hs_cell_parse_introduce2(&data, circ, service) < 0) {
+    hs_metrics_reject_intro_req(service, HS_METRICS_ERR_INTRO_REQ_INTRODUCE2);
     goto done;
   }
 
@@ -1027,6 +1033,8 @@ hs_circ_handle_introduce2(const hs_service_t *service,
     log_info(LD_REND, "We received an INTRODUCE2 cell with same REND_COOKIE "
                       "field %ld seconds ago. Dropping cell.",
              (long int) elapsed);
+    hs_metrics_reject_intro_req(service,
+                                HS_METRICS_ERR_INTRO_REQ_INTRODUCE2_REPLAY);
     goto done;
   }
 

src/feature/hs/hs_metrics.c

@@ -13,6 +13,7 @@
 #include "lib/malloc/malloc.h"
 #include "lib/container/smartlist.h"
 #include "lib/metrics/metrics_store.h"
+#include "lib/log/util_bug.h"
 
 #include "feature/hs/hs_metrics.h"
 #include "feature/hs/hs_metrics_entry.h"
@@ -29,6 +30,87 @@ port_to_str(const uint16_t port)
   return buf;
 }
 
+/** Add a new metric to the metrics store of the service.
+ *
+ * <b>metric</b> is the index of the metric in the <b>base_metrics</b> array.
+ */
+static void
+add_metric_with_labels(hs_service_t *service, hs_metrics_key_t metric,
+                       bool port_as_label, uint16_t port)
+{
+  metrics_store_t *store;
+  const char **error_reasons = NULL;
+  size_t num_error_reasons = 0;
+
+  tor_assert(service);
+
+  if (BUG(metric >= base_metrics_size))
+    return;
+
+  store = service->metrics.store;
+
+  /* Check whether the current metric is an error metric, because error metrics
+   * require an additional `reason` label. */
+  switch (metric) {
+    case HS_METRICS_NUM_REJECTED_INTRO_REQ:
+      error_reasons = hs_metrics_intro_req_error_reasons;
+      num_error_reasons = hs_metrics_intro_req_error_reasons_size;
+      break;
+    case HS_METRICS_NUM_FAILED_RDV:
+      error_reasons = hs_metrics_rend_error_reasons;
+      num_error_reasons = hs_metrics_rend_error_reasons_size;
+      break;
+    /* Fall through for all other metrics, as they don't need a
+     * reason label. */
+    case HS_METRICS_NUM_INTRODUCTIONS: FALLTHROUGH;
+    case HS_METRICS_APP_WRITE_BYTES: FALLTHROUGH;
+    case HS_METRICS_APP_READ_BYTES: FALLTHROUGH;
+    case HS_METRICS_NUM_ESTABLISHED_RDV: FALLTHROUGH;
+    case HS_METRICS_NUM_RDV: FALLTHROUGH;
+    case HS_METRICS_NUM_ESTABLISHED_INTRO: FALLTHROUGH;
+    default:
+      break;
+  }
+
+  /* We don't need a reason label for this metric */
+  if (!num_error_reasons) {
+      metrics_store_entry_t *entry = metrics_store_add(
+          store, base_metrics[metric].type, base_metrics[metric].name,
+          base_metrics[metric].help);
+
+      metrics_store_entry_add_label(entry,
+              metrics_format_label("onion", service->onion_address));
+
+      if (port_as_label) {
+        metrics_store_entry_add_label(entry,
+                metrics_format_label("port", port_to_str(port)));
+      }
+
+      return;
+  }
+
+  tor_assert(error_reasons);
+
+  /* Add entries with reason as label. We need one metric line per
+   * reason. */
+  for (size_t i = 0; i < num_error_reasons; ++i) {
+    metrics_store_entry_t *entry =
+      metrics_store_add(store, base_metrics[metric].type,
+                        base_metrics[metric].name,
+                        base_metrics[metric].help);
+    /* Add labels to the entry. */
+    metrics_store_entry_add_label(entry,
+            metrics_format_label("onion", service->onion_address));
+    metrics_store_entry_add_label(entry,
+            metrics_format_label("reason", error_reasons[i]));
+
+    if (port_as_label) {
+      metrics_store_entry_add_label(entry,
+              metrics_format_label("port", port_to_str(port)));
+    }
+  }
+}
+
 /** Initialize a metrics store for the given service.
  *
  * Essentially, this goes over the base_metrics array and adds them all to the
@@ -36,44 +118,30 @@ port_to_str(const uint16_t port)
 static void
 init_store(hs_service_t *service)
 {
-  metrics_store_t *store;
-
   tor_assert(service);
 
-  store = service->metrics.store;
-
   for (size_t i = 0; i < base_metrics_size; ++i) {
     /* Add entries with port as label. We need one metric line per port. */
     if (base_metrics[i].port_as_label && service->config.ports) {
       SMARTLIST_FOREACH_BEGIN(service->config.ports,
                               const hs_port_config_t *, p) {
-        metrics_store_entry_t *entry =
+        add_metric_with_labels(service, base_metrics[i].key, true,
-          metrics_store_add(store, base_metrics[i].type, base_metrics[i].name,
+                               p->virtual_port);
-                            base_metrics[i].help);
-
-        /* Add labels to the entry. */
-        metrics_store_entry_add_label(entry,
-                metrics_format_label("onion", service->onion_address));
-        metrics_store_entry_add_label(entry,
-                metrics_format_label("port", port_to_str(p->virtual_port)));
       } SMARTLIST_FOREACH_END(p);
     } else {
-      metrics_store_entry_t *entry =
+      add_metric_with_labels(service, base_metrics[i].key, false, 0);
-        metrics_store_add(store, base_metrics[i].type, base_metrics[i].name,
-                          base_metrics[i].help);
-      metrics_store_entry_add_label(entry,
-              metrics_format_label("onion", service->onion_address));
     }
   }
 }
 
 /** Update the metrics key entry in the store in the given service. The port,
- * if non 0, is used to find the correct metrics entry. The value n is the
+ * if non 0, and the reason label, if non NULL, are used to find the correct
- * value used to update the entry. */
+ * metrics entry. The value n is the value used to update the entry. */
 void
 hs_metrics_update_by_service(const hs_metrics_key_t key,
                              const hs_service_t *service,
-                             uint16_t port, int64_t n)
+                             uint16_t port, const char *reason,
+                             int64_t n)
 {
   tor_assert(service);
 
@@ -89,9 +157,11 @@ hs_metrics_update_by_service(const hs_metrics_key_t key,
    * XXX: This is not the most optimal due to the string format. Maybe at some
    * point turn this into a kvline and a map in a metric entry? */
   SMARTLIST_FOREACH_BEGIN(entries, metrics_store_entry_t *, entry) {
-    if (port == 0 ||
+    if ((port == 0 ||
-        metrics_store_entry_has_label(entry,
+         metrics_store_entry_has_label(
-                      metrics_format_label("port", port_to_str(port)))) {
+             entry, metrics_format_label("port", port_to_str(port)))) &&
+        ((!reason || metrics_store_entry_has_label(
+                         entry, metrics_format_label("reason", reason))))) {
         metrics_store_entry_update(entry, n);
         break;
     }
@@ -99,15 +169,17 @@ hs_metrics_update_by_service(const hs_metrics_key_t key,
 }
 
 /** Update the metrics key entry in the store of a service identified by the
- * given identity public key. The port, if non 0, is used to find the correct
+ * given identity public key. The port, if non 0, and the reason label, if non
- * metrics entry. The value n is the value used to update the entry.
+ * NULL, are used to find the correct metrics entry. The value n is the value
+ * used to update the entry.
  *
  * This is used by callsite that have access to the key but not the service
  * object so an extra lookup is done to find the service. */
 void
 hs_metrics_update_by_ident(const hs_metrics_key_t key,
                            const ed25519_public_key_t *ident_pk,
-                           const uint16_t port, int64_t n)
+                           const uint16_t port, const char *reason,
+                           int64_t n)
 {
   hs_service_t *service;
 
@@ -121,7 +193,7 @@ hs_metrics_update_by_ident(const hs_metrics_key_t key,
      * service and thus the only way to know is to lookup the service. */
     return;
   }
-  hs_metrics_update_by_service(key, service, port, n);
+  hs_metrics_update_by_service(key, service, port, reason, n);
 }
 
 /** Return a list of all the onion service metrics stores. This is the

src/feature/hs/hs_metrics.h

@@ -26,53 +26,59 @@ const smartlist_t *hs_metrics_get_stores(void);
 /* Metrics Update. */
 void hs_metrics_update_by_ident(const hs_metrics_key_t key,
                                 const ed25519_public_key_t *ident_pk,
-                                const uint16_t port, int64_t n);
+                                const uint16_t port, const char *reason,
+                                int64_t n);
 void hs_metrics_update_by_service(const hs_metrics_key_t key,
                                   const hs_service_t *service,
-                                  uint16_t port, int64_t n);
+                                  uint16_t port, const char *reason,
+                                  int64_t n);
 
 /** New introducion request received. */
 #define hs_metrics_new_introduction(s) \
-  hs_metrics_update_by_service(HS_METRICS_NUM_INTRODUCTIONS, (s), 0, 1)
+  hs_metrics_update_by_service(HS_METRICS_NUM_INTRODUCTIONS, (s), 0, NULL, 1)
 
 /** Introducion request rejected. */
-#define hs_metrics_reject_intro_req(s) \
+#define hs_metrics_reject_intro_req(s, reason)                            \
-  hs_metrics_update_by_service(HS_METRICS_NUM_REJECTED_INTRO_REQ, (s), 0, 1)
+  hs_metrics_update_by_service(HS_METRICS_NUM_REJECTED_INTRO_REQ, (s), 0, \
+                               (reason), 1)
 
 /** Number of bytes written to the application from the service. */
 #define hs_metrics_app_write_bytes(i, port, n)                              \
-  hs_metrics_update_by_ident(HS_METRICS_APP_WRITE_BYTES, (i), (port), (n))
+  hs_metrics_update_by_ident(HS_METRICS_APP_WRITE_BYTES, (i), (port), NULL, \
+                             (n))
 
 /** Number of bytes read from the application to the service. */
 #define hs_metrics_app_read_bytes(i, port, n) \
-  hs_metrics_update_by_ident(HS_METRICS_APP_READ_BYTES, (i), (port), (n))
+  hs_metrics_update_by_ident(HS_METRICS_APP_READ_BYTES, (i), (port), NULL, (n))
 
 /** Newly established rendezvous. This is called as soon as the circuit purpose
  * is REND_JOINED which is when the RENDEZVOUS2 cell is sent. */
 #define hs_metrics_new_established_rdv(s) \
-  hs_metrics_update_by_service(HS_METRICS_NUM_ESTABLISHED_RDV, (s), 0, 1)
+  hs_metrics_update_by_service(HS_METRICS_NUM_ESTABLISHED_RDV, (s), 0, NULL, 1)
 
 /** New rendezvous circuit failure. */
-#define hs_metrics_failed_rdv(i) \
+#define hs_metrics_failed_rdv(i, reason) \
-  hs_metrics_update_by_ident(HS_METRICS_NUM_FAILED_RDV, (i), 0, 1)
+  hs_metrics_update_by_ident(HS_METRICS_NUM_FAILED_RDV, (i), 0, (reason), 1)
 
 /** Established rendezvous closed. This is called when the circuit in
  * REND_JOINED state is marked for close. */
 #define hs_metrics_close_established_rdv(i) \
-  hs_metrics_update_by_ident(HS_METRICS_NUM_ESTABLISHED_RDV, (i), 0, -1)
+  hs_metrics_update_by_ident(HS_METRICS_NUM_ESTABLISHED_RDV, (i), 0, NULL, -1)
 
 /** New rendezvous circuit being launched. */
 #define hs_metrics_new_rdv(i) \
-  hs_metrics_update_by_ident(HS_METRICS_NUM_RDV, (i), 0, 1)
+  hs_metrics_update_by_ident(HS_METRICS_NUM_RDV, (i), 0, NULL, 1)
 
 /** New introduction circuit has been established. This is called when the
  * INTRO_ESTABLISHED has been received by the service. */
 #define hs_metrics_new_established_intro(s)                              \
-  hs_metrics_update_by_service(HS_METRICS_NUM_ESTABLISHED_INTRO, (s), 0, 1)
+  hs_metrics_update_by_service(HS_METRICS_NUM_ESTABLISHED_INTRO, (s), 0, \
+                               NULL, 1)
 
 /** Established introduction circuit closes. This is called when
  * INTRO_ESTABLISHED circuit is marked for close. */
 #define hs_metrics_close_established_intro(i)                                \
-  hs_metrics_update_by_ident(HS_METRICS_NUM_ESTABLISHED_INTRO, (i), 0, -1)
+  hs_metrics_update_by_ident(HS_METRICS_NUM_ESTABLISHED_INTRO, (i), 0, NULL, \
+                             -1)
 
 #endif /* !defined(TOR_FEATURE_HS_HS_METRICS_H) */

src/feature/hs/hs_metrics_entry.c

@@ -8,9 +8,13 @@
 
 #define HS_METRICS_ENTRY_PRIVATE
 
+#include <stddef.h>
+
 #include "orconfig.h"
 
 #include "lib/cc/compat_compiler.h"
+#include "lib/log/log.h"
+#include "lib/log/util_bug.h"
 
 #include "feature/hs/hs_metrics_entry.h"
 
@@ -75,3 +79,31 @@ const hs_metrics_entry_t base_metrics[] =
 
 /** Size of base_metrics array that is number of entries. */
 const size_t base_metrics_size = ARRAY_LENGTH(base_metrics);
+
+/** Possible values for the reason label of the
+ * hs_intro_rejected_intro_req_count metric. */
+const char *hs_metrics_intro_req_error_reasons[] =
+{
+  HS_METRICS_ERR_INTRO_REQ_BAD_AUTH_KEY,
+  HS_METRICS_ERR_INTRO_REQ_INTRODUCE2,
+  HS_METRICS_ERR_INTRO_REQ_SUBCREDENTIAL,
+  HS_METRICS_ERR_INTRO_REQ_INTRODUCE2_REPLAY,
+};
+
+/** The number of entries in the hs_metrics_intro_req_error_reasons array. */
+const size_t hs_metrics_intro_req_error_reasons_size =
+    ARRAY_LENGTH(hs_metrics_intro_req_error_reasons);
+
+/** Possible values for the reason label of the hs_rdv_error_count metric. */
+const char *hs_metrics_rend_error_reasons[] =
+{
+  HS_METRICS_ERR_RDV_RP_CONN_FAILURE,
+  HS_METRICS_ERR_RDV_PATH,
+  HS_METRICS_ERR_RDV_RENDEZVOUS1,
+  HS_METRICS_ERR_RDV_E2E,
+  HS_METRICS_ERR_RDV_RETRY,
+};
+
+/** The number of entries in the hs_metrics_rend_error_reasons array. */
+const size_t hs_metrics_rend_error_reasons_size =
+    ARRAY_LENGTH(hs_metrics_rend_error_reasons);

src/feature/hs/hs_metrics_entry.h

@@ -13,6 +13,33 @@
 
 #include "lib/metrics/metrics_common.h"
 
+/* Possible values for the reason label of the
+ * hs_intro_rejected_intro_req_count metric. */
+/** The hidden service received an unknown introduction auth key. */
+#define HS_METRICS_ERR_INTRO_REQ_BAD_AUTH_KEY      "bad_auth_key"
+/** The hidden service received a malformed INTRODUCE2 cell. */
+#define HS_METRICS_ERR_INTRO_REQ_INTRODUCE2        "invalid_introduce2"
+/** The hidden service does not have the necessary subcredential. */
+#define HS_METRICS_ERR_INTRO_REQ_SUBCREDENTIAL     "subcredential"
+/** The hidden service received an INTRODUCE2 replay. */
+#define HS_METRICS_ERR_INTRO_REQ_INTRODUCE2_REPLAY "replay"
+
+/* Possible values for the reason label of the hs_rdv_error_count metric. */
+/** The hidden service failed to connect to the rendezvous point. */
+#define HS_METRICS_ERR_RDV_RP_CONN_FAILURE         "rp_conn_failure"
+/** The hidden service failed to build a circuit to the rendezvous point due
+ * to an invalid selected path. */
+#define  HS_METRICS_ERR_RDV_PATH                   "invalid_path"
+/** The hidden service failed to send the RENDEZVOUS1 cell on rendezvous
+ * circuit. */
+#define HS_METRICS_ERR_RDV_RENDEZVOUS1             "rendezvous1"
+/** The hidden service failed to set up an end-to-end rendezvous circuit to
+ * the client. */
+#define HS_METRICS_ERR_RDV_E2E                     "e2e_circ"
+/** The hidden service reattempted to connect to the rendezvous point by
+ * launching a new circuit to it, but failed */
+#define HS_METRICS_ERR_RDV_RETRY                   "retry"
+
 /** Metrics key which are used as an index in the main base metrics array. */
 typedef enum {
   /** Number of introduction requests. */
@@ -50,6 +77,11 @@ typedef struct hs_metrics_entry_t {
 extern const hs_metrics_entry_t base_metrics[];
 extern const size_t base_metrics_size;
 
-#endif /* defined(HS_METRICS_ENTRY_PRIVATE) */
+extern const char *hs_metrics_intro_req_error_reasons[];
+extern const size_t hs_metrics_intro_req_error_reasons_size;
 
+extern const char *hs_metrics_rend_error_reasons[];
+extern const size_t hs_metrics_rend_error_reasons_size;
+
+#endif /* defined(HS_METRICS_ENTRY_PRIVATE) */
 #endif /* !defined(TOR_FEATURE_HS_METRICS_ENTRY_H) */

src/feature/hs/hs_service.c

@@ -42,6 +42,7 @@
 #include "feature/hs/hs_ident.h"
 #include "feature/hs/hs_intropoint.h"
 #include "feature/hs/hs_metrics.h"
+#include "feature/hs/hs_metrics_entry.h"
 #include "feature/hs/hs_service.h"
 #include "feature/hs/hs_stats.h"
 #include "feature/hs/hs_ob.h"
@@ -3508,6 +3509,9 @@ service_handle_introduce2(origin_circuit_t *circ, const uint8_t *payload,
                      "an INTRODUCE2 cell on circuit %u for service %s",
              TO_CIRCUIT(circ)->n_circ_id,
              safe_str_client(service->onion_address));
+
+    hs_metrics_reject_intro_req(service,
+                                HS_METRICS_ERR_INTRO_REQ_BAD_AUTH_KEY);
     goto err;
   }
   /* If we have an IP object, we MUST have a descriptor object. */
@@ -3524,9 +3528,6 @@ service_handle_introduce2(origin_circuit_t *circ, const uint8_t *payload,
 
   return 0;
  err:
-  if (service) {
-    hs_metrics_reject_intro_req(service);
-  }
 
   return -1;
 }

src/lib/metrics/metrics_store_entry.c

@@ -127,3 +127,23 @@ metrics_store_entry_has_label(const metrics_store_entry_t *entry,
 
   return smartlist_contains_string(entry->labels, label);
 }
+
+/** Return the first entry that has the given label, or NULL if none
+ * of the entries have the label. */
+metrics_store_entry_t *
+metrics_store_find_entry_with_label(const smartlist_t *entries,
+                                    const char *label)
+{
+  tor_assert(entries);
+  tor_assert(label);
+
+  SMARTLIST_FOREACH_BEGIN(entries, metrics_store_entry_t *, entry) {
+    tor_assert(entry);
+
+    if (smartlist_contains_string(entry->labels, label)) {
+      return entry;
+    }
+  } SMARTLIST_FOREACH_END(entry);
+
+  return NULL;
+}

src/lib/metrics/metrics_store_entry.h

@@ -12,6 +12,7 @@
 #include "lib/cc/torint.h"
 
 #include "lib/metrics/metrics_common.h"
+#include "lib/container/smartlist.h"
 
 #ifdef METRICS_STORE_ENTRY_PRIVATE
 
@@ -57,6 +58,8 @@ void metrics_store_entry_free_(metrics_store_entry_t *entry);
 int64_t metrics_store_entry_get_value(const metrics_store_entry_t *entry);
 bool metrics_store_entry_has_label(const metrics_store_entry_t *entry,
                                    const char *label);
+metrics_store_entry_t *metrics_store_find_entry_with_label(
+        const smartlist_t *entries, const char *label);
 
 /* Modifiers. */
 void metrics_store_entry_add_label(metrics_store_entry_t *entry,

src/test/test_hs_metrics.c

@@ -40,7 +40,7 @@ test_metrics(void *arg)
 
   /* Update entry by identifier. */
   hs_metrics_update_by_ident(HS_METRICS_NUM_INTRODUCTIONS,
-                             &service->keys.identity_pk, 0, 42);
+                             &service->keys.identity_pk, 0, NULL, 42);
 
   /* Confirm the entry value. */
   const smartlist_t *entries = metrics_store_get_all(service->metrics.store,
@@ -53,24 +53,29 @@ test_metrics(void *arg)
 
   /* Update entry by service now. */
   hs_metrics_update_by_service(HS_METRICS_NUM_INTRODUCTIONS,
-                               service, 0, 42);
+                               service, 0, NULL, 42);
   tt_int_op(metrics_store_entry_get_value(entry), OP_EQ, 84);
 
+  const char *reason = HS_METRICS_ERR_INTRO_REQ_BAD_AUTH_KEY;
+
   /* Update tor_hs_intro_rejected_intro_req_count */
   hs_metrics_update_by_ident(HS_METRICS_NUM_REJECTED_INTRO_REQ,
-                             &service->keys.identity_pk, 0, 112);
+                             &service->keys.identity_pk, 0, reason, 112);
 
   entries = metrics_store_get_all(service->metrics.store,
                                   "tor_hs_intro_rejected_intro_req_count");
   tt_assert(entries);
-  tt_int_op(smartlist_len(entries), OP_EQ, 1);
+  tt_int_op(smartlist_len(entries), OP_EQ,
-  entry = smartlist_get(entries, 0);
+            hs_metrics_intro_req_error_reasons_size);
+
+  entry = metrics_store_find_entry_with_label(
+      entries, "reason=\"bad_auth_key\"");
   tt_assert(entry);
   tt_int_op(metrics_store_entry_get_value(entry), OP_EQ, 112);
 
   /* Update tor_hs_intro_rejected_intro_req_count entry by service now. */
-  hs_metrics_update_by_service(HS_METRICS_NUM_REJECTED_INTRO_REQ,
+  hs_metrics_update_by_service(HS_METRICS_NUM_REJECTED_INTRO_REQ, service, 0,
-                               service, 0, 10);
+                               reason, 10);
   tt_int_op(metrics_store_entry_get_value(entry), OP_EQ, 122);
 
  done:

src/test/test_hs_service.c

@@ -1229,14 +1229,34 @@ test_bad_introduce2(void *arg)
                             "an INTRODUCE2 cell on circuit");
   teardown_capture_of_logs();
 
-  /* Make sure the tor_hs_intro_rejected_intro_req_count metric was
-   * incremented */
   entries = metrics_store_get_all(service->metrics.store,
                                   "tor_hs_intro_rejected_intro_req_count");
 
   tt_assert(entries);
-  tt_int_op(smartlist_len(entries), OP_EQ, 1);
+  /* There are `hs_metrics_intro_req_size` entries (one for each
-  entry = smartlist_get(entries, 0);
+   * possible `reason` label value). */
+  tt_int_op(smartlist_len(entries), OP_EQ,
+            hs_metrics_intro_req_error_reasons_size);
+
+  /* Make sure the tor_hs_intro_rejected_intro_req_count metric was
+   * only incremented for reason HS_METRICS_ERR_INTRO_REQ_BAD_AUTH_KEY. */
+  for (size_t i = 0; i < hs_metrics_intro_req_error_reasons_size; ++i) {
+    const char *reason = hs_metrics_intro_req_error_reasons[i];
+
+    if (!strcmp(reason, HS_METRICS_ERR_INTRO_REQ_BAD_AUTH_KEY)) {
+      continue;
+    }
+
+    entry = metrics_store_find_entry_with_label(
+        entries,
+        metrics_format_label("reason", reason));
+    tt_assert(entry);
+    tt_int_op(metrics_store_entry_get_value(entry), OP_EQ, 0);
+  }
+
+  entry = metrics_store_find_entry_with_label(
+      entries,
+      metrics_format_label("reason", HS_METRICS_ERR_INTRO_REQ_BAD_AUTH_KEY));
   tt_assert(entry);
   tt_int_op(metrics_store_entry_get_value(entry), OP_EQ, 1);
 
@@ -1257,8 +1277,31 @@ test_bad_introduce2(void *arg)
   tt_u64_op(ip->introduce2_count, OP_EQ, 0);
 
   /* Make sure the tor_hs_intro_rejected_intro_req_count metric was incremented
-   * a second time */
+   * a second time, this time, with reason="invalid_introduce2_cell". */
-  tt_int_op(metrics_store_entry_get_value(entry), OP_EQ, 2);
+  entry = metrics_store_find_entry_with_label(
+      entries,
+      metrics_format_label("reason", HS_METRICS_ERR_INTRO_REQ_INTRODUCE2));
+  tt_assert(entry);
+  tt_int_op(metrics_store_entry_get_value(entry), OP_EQ, 1);
+
+  /* The metric entries with other reason labels are unaffected */
+  entry = metrics_store_find_entry_with_label(
+      entries,
+      metrics_format_label("reason", HS_METRICS_ERR_INTRO_REQ_SUBCREDENTIAL));
+  tt_assert(entry);
+  tt_int_op(metrics_store_entry_get_value(entry), OP_EQ, 0);
+
+  entry = metrics_store_find_entry_with_label(
+      entries, metrics_format_label(
+                   "reason", HS_METRICS_ERR_INTRO_REQ_INTRODUCE2_REPLAY));
+  tt_assert(entry);
+  tt_int_op(metrics_store_entry_get_value(entry), OP_EQ, 0);
+
+  entry = metrics_store_find_entry_with_label(
+      entries,
+      metrics_format_label("reason", HS_METRICS_ERR_INTRO_REQ_BAD_AUTH_KEY));
+  tt_assert(entry);
+  tt_int_op(metrics_store_entry_get_value(entry), OP_EQ, 1);
 
  done:
   or_state_free(dummy_state);
@@ -2293,6 +2336,8 @@ test_intro2_handling(void *arg)
   /* Disable onionbalance */
   x_service.config.ob_master_pubkeys = NULL;
   x_service.state.replay_cache_rend_cookie = replaycache_new(0,0);
+  /* Initialize the metrics store */
+  hs_metrics_service_init(&x_service);
 
   /* Create subcredential for x: */
   ed25519_keypair_t x_identity_keypair;
@@ -2456,6 +2501,20 @@ test_intro2_handling(void *arg)
                                    (uint8_t*)relay_payload, relay_payload_len);
   tt_int_op(retval, OP_EQ, 0);
 
+  /* We haven't encountered any errors yet, so all the introduction request
+   * error metrics should be 0 */
+  const smartlist_t *entries = metrics_store_get_all(
+      x_service.metrics.store, "tor_hs_intro_rejected_intro_req_count");
+  const metrics_store_entry_t *entry = NULL;
+
+  for (size_t i = 0; i < hs_metrics_intro_req_error_reasons_size; ++i) {
+    entry = metrics_store_find_entry_with_label(
+        entries,
+        metrics_format_label("reason", hs_metrics_intro_req_error_reasons[i]));
+    tt_assert(entry);
+    tt_int_op(metrics_store_entry_get_value(entry), OP_EQ, 0);
+  }
+
   /* ************************************************************ */
 
   /* Act IV:
@@ -2473,6 +2532,11 @@ test_intro2_handling(void *arg)
   tt_int_op(retval, OP_EQ, -1);
   expect_log_msg_containing("with the same ENCRYPTED section");
   teardown_capture_of_logs();
+  entry = metrics_store_find_entry_with_label(
+      entries,
+      metrics_format_label("reason", HS_METRICS_ERR_INTRO_REQ_INTRODUCE2));
+  tt_assert(entry);
+  tt_int_op(metrics_store_entry_get_value(entry), OP_EQ, 1);
 
   /* Now cleanup the intro point replay cache but not the service replay cache
      and see that this one triggers this time. */
@@ -2487,6 +2551,12 @@ test_intro2_handling(void *arg)
   expect_log_msg_containing("with same REND_COOKIE");
   teardown_capture_of_logs();
 
+  entry = metrics_store_find_entry_with_label(
+      entries, metrics_format_label(
+                   "reason", HS_METRICS_ERR_INTRO_REQ_INTRODUCE2_REPLAY));
+  tt_assert(entry);
+  tt_int_op(metrics_store_entry_get_value(entry), OP_EQ, 1);
+
   /* Now just to make sure cleanup both replay caches and make sure that the
      cell gets through */
   replaycache_free(x_ip->replay_cache);
@@ -2499,6 +2569,9 @@ test_intro2_handling(void *arg)
                                    (uint8_t*)relay_payload, relay_payload_len);
   tt_int_op(retval, OP_EQ, 0);
 
+  /* This time, the error metric was *not* incremented */
+  tt_int_op(metrics_store_entry_get_value(entry), OP_EQ, 1);
+
   /* As a final thing, create an INTRODUCE1 cell from Alice to X using Y's
    * subcred (should fail since Y is just another instance and not the frontend
    * service!) */
@@ -2515,7 +2588,13 @@ test_intro2_handling(void *arg)
                                    intro_circ, x_ip,
                                    &y_subcred,
                                    (uint8_t*)relay_payload, relay_payload_len);
+
   tt_int_op(retval, OP_EQ, -1);
+  entry = metrics_store_find_entry_with_label(
+      entries,
+      metrics_format_label("reason", HS_METRICS_ERR_INTRO_REQ_INTRODUCE2));
+  tt_assert(entry);
+  tt_int_op(metrics_store_entry_get_value(entry), OP_EQ, 2);
 
  done:
   /* Start cleaning up X */
@@ -2524,6 +2603,7 @@ test_intro2_handling(void *arg)
   tor_free(x_service.state.ob_subcreds);
   service_descriptor_free(x_service.desc_current);
   service_descriptor_free(x_service.desc_next);
+  hs_metrics_service_free(&x_service);
   service_intro_point_free(x_ip);
 
   /* Clean up Alice */