Issue when using @Timed annotation with prometheus-metrics-instrumentation-dropwizard

[kpur-sbab]

@timed annotation, which can be annotated on the methods generates dropwizard metrics with .total as a suffix.

This is not compatible with prometheus-metrics-instrumentation-dropwizard to export dropwizard metrics.

Since the io.prometheus.metrics.instrumentation.dropwizard.DropwizardExports class has a method fromSnapshotAndCount which sanitizes the metric name withPrometheusNaming.sanitizeMetricName(dropwizardName).

As a result it causes duplicate metric name exception when prometheus endpoint is invoked.

Original reference here:
dropwizard/dropwizard#10030 #

[zeitlinger]

This will be fixed in prometheus/OpenMetrics#286 - which basically says that metric names are not changed or checked in client sdks

[kpur-sbab]

Great thanks. We are doing this as a temporary fix for DW 4.x

prometheusRegistry.register(new DropwizardExports(environment.metrics(),
                (name, metric) -> !(metric instanceof Timer) || !name.matches(".*[._]total.*")));
        environment.admin()
                .addServlet("prometheusMetrics", new PrometheusMetricsServlet(prometheusRegistry))
                .addMapping("/prometheusMetrics");

[YellowFlora]

Hello @zeitlinger,

In the current client_java version the .total suffix is still dropped by AbstractDropwizardExports because it calls sanitizeMetricName, which removes the .total suffix.

My understanding was that this behavior was supposed to change so that Dropwizard metrics ending in .total are handled in a way that avoids name collisions. Is that correct, or is the .total suffix still intentionally dropped?

The issue is that when a Dropwizard metric like <servicepath>.uploadDocumentFile.total and a metric without the suffix both get sanitized to the same name (<servicepath>_uploadDocumentFile), the merge of snapshots later fails due to duplicate names with identical (empty) label sets.

This causes a failure in TextFormatUtil.mergeDuplicates, because it tries to combine metrics with the same name and identical (empty) labels, resulting in a DuplicateLabelsException.

Example test illustrating the failure:

@Test
void testMergeDuplicates_sameName_mergesLabels() {
  SummarySnapshot counter1 =
    SummarySnapshot.builder()
      .name("<servicepath>_uploadDocumentFile")
      .dataPoint(
        SummarySnapshot.SummaryDataPointSnapshot.builder()
          .count(100)
          .build())
      .build();

  SummarySnapshot counter2 =
    SummarySnapshot.builder()
      .name("<servicepath>_uploadDocumentFile")
      .dataPoint(
        SummarySnapshot.SummaryDataPointSnapshot.builder()
          .count(10)
          .build())
      .build();

  MetricSnapshots snapshots = new MetricSnapshots(counter1, counter2);

  assertThrows(DuplicateLabelsException.class, () -> TextFormatUtil.mergeDuplicates(snapshots));
}

So my question is: should .total still be stripped at this stage, or should the exporter avoid dropping it to prevent duplicate metric name collisions?

[zeitlinger]

@YellowFlora You're right that .total is currently stripped, and that's working as designed per the OpenMetrics 1.0 spec. This will change as part of #1912 (OpenMetrics 2.0 support), which will stop treating _total/.total as reserved suffixes that get stripped during sanitization.

[josephlbarnett]

rather than filtering them out (we use async requests so the .total is more useful than the raw one), we map ".total" -> ".async" like so:

.customLabelMapper(
                    object : CustomLabelMapper(
                        listOf(MapperConfig(
                            // This config won't actually match what we want because the * glob stops at '.'s
                            // but we do need a non-empty list of MapperConfigs to use a CustomLabelMapper
                            "*.total", $$"${0}.total", mapOf())),
                    ) {
                        override fun getName(dropwizardName: String?): String {
                            // this one actually remaps **.total -> **.async
                            return super.getName(dropwizardName?.replace("\\.total$".toRegex(), ".async"))
                        }
                    },
                )

not sure if there's a less awkward way to set it up with the current code?