Normalize metrics from measurements #45

nineinchnick · 2022-09-14T07:56:31Z

This PR extracts name and unit from measurements into another metrics table. Such metrics also have attributes, which allow the removal of prefixes and suffixes from metric names and make it easier to filter some metrics when building reports.

The denormalized measurement.name and measurement.unit columns are not removed, but the name format is changed from cluster-cpu_max to cpu {scope=cluster,aggregate=max}.

This change matches how other metric monitoring system stores data, using labels (Prometheus) or attributes (OpenTelemetry).

benchto-driver/src/main/java/io/trino/benchto/driver/graphite/GraphiteMetricsLoader.java

benchto-driver/src/main/java/io/trino/benchto/driver/presto/PrestoClient.java

benchto-driver/src/test/java/io/trino/benchto/driver/DriverAppIntegrationTest.java

benchto-service/src/main/java/io/trino/benchto/service/model/Measurement.java

benchto-service/src/main/resources/db/migration/V006__metrics.sql

nineinchnick · 2022-09-19T08:08:08Z

@przemekak @sopel39 @radek-starburst PTAL. There are breaking changes here but I tried to preserve some backward compatibility.

radek-kondziolka

I like that change. I've added some comments and I think that before merge and release it we need to firstly make changes in our integrations.

radek-kondziolka · 2022-09-19T09:02:56Z

benchto-driver/src/test/java/io/trino/benchto/driver/DriverAppIntegrationTest.java

@@ -89,11 +101,11 @@ public void testBenchmark()
    @Test
    public void testConcurrentBenchmark()
    {
-        ImmutableList<String> concurrentQueryMeasurementName = ImmutableList.of("duration");
-        ImmutableList<String> concurrentBenchmarkMeasurementNames = ImmutableList.<String>builder()
+        List<Measurement> concurrentQueryMeasurement = ImmutableList.of(measurement("duration", "MILLISECONDS", 0.0, ImmutableMap.of("scope", "driver")));


It looks like a factory method. Maybe something like Measurement.of?

I agree, but I'd rather do this in a separate PR.

benchto-service/src/main/java/io/trino/benchto/service/model/Metric.java

benchto-service/src/main/java/io/trino/benchto/service/BenchmarkService.java

radek-kondziolka · 2022-09-19T09:31:45Z

benchto-service/src/main/java/io/trino/benchto/service/model/Measurement.java

@@ -109,22 +145,21 @@ public boolean equals(Object o)
            return false;
        }
        Measurement that = (Measurement) o;
-        return Objects.equals(name, that.name);
+        return metric.equals(that.metric);


Why unit does not matter here? Hibernate strongly depends on correct equals / hashcode and it looks like two different object could be treated as same, od I miss something?

This is why I switched to comparing metrics, to also account for the unit (and attributes). The name and unit properties that remain here are denormalized from the metric.

sopel39 · 2022-09-22T08:31:20Z

benchto-driver/src/test/java/io/trino/benchto/driver/PrestoClientIntegrationTest.java

-                Measurement.measurement("prestoQuery-outputDataSize", "BYTES", 6900.0),
-                Measurement.measurement("prestoQuery-peakTotalMemoryReservation", "BYTES", 6800.0),
-                Measurement.measurement("prestoQuery-physicalWrittenDataSize", "BYTES", 462265065.0));
+                Measurement.measurement("analysisTime", "MILLISECONDS", 21.07, attributes),


this will break scripts (jupyther). I think it's fine, but needs to be announced

Not only scripts but our workflows as well.

You mean alerts?

sopel39 · 2022-09-22T08:32:57Z

benchto-driver/src/main/java/io/trino/benchto/driver/graphite/GraphiteMetricsLoader.java

@@ -150,8 +153,8 @@ private void addMeanMaxMeasurements(Map<String, double[]> loadedMetrics, List<Me
    {
        Optional<StatisticalSummary> statistics = getStats(loadedMetrics, metricName);
        if (statistics.isPresent()) {
-            measurements.add(Measurement.measurement("cluster-" + metricName + "_max", unit, statistics.get().getMax()));
-            measurements.add(Measurement.measurement("cluster-" + metricName + "_mean", unit, statistics.get().getMean()));
+            measurements.add(Measurement.measurement(metricName, unit, statistics.get().getMax(), ImmutableMap.of("scope", "cluster", "aggregate", "max")));


METRIC_SCOPE instead of cluster?

sopel39 · 2022-09-22T08:34:13Z

benchto-driver/src/main/java/io/trino/benchto/driver/graphite/GraphiteMetricsLoader.java

@@ -150,8 +153,8 @@ private void addMeanMaxMeasurements(Map<String, double[]> loadedMetrics, List<Me
    {
        Optional<StatisticalSummary> statistics = getStats(loadedMetrics, metricName);
        if (statistics.isPresent()) {
-            measurements.add(Measurement.measurement("cluster-" + metricName + "_max", unit, statistics.get().getMax()));


won't this make scripts harder, e.g: previously I could just select name in SQL query. Now I will have to join on attributes and do subquery. Seems complicated

sopel39 · 2022-09-22T08:34:28Z

benchto-driver/src/main/java/io/trino/benchto/driver/presto/PrestoClient.java

@@ -123,7 +124,7 @@ private URI buildQueryInfoURI(String queryId)
    private Measurement parseQueryStatistic(String name, Object statistic, Unit requiredUnit)
    {
        double value = UnitConverter.parseValueAsUnit(statistic.toString(), requiredUnit);
-        return measurement("prestoQuery-" + name, UnitConverter.format(requiredUnit), value);
+        return measurement(name, UnitConverter.format(requiredUnit), value, Collections.singletonMap("scope", "prestoQuery"));


prestoQuery -> query

sopel39 · 2022-09-22T08:40:18Z

benchto-driver/src/main/java/io/trino/benchto/driver/graphite/GraphiteMetricsLoader.java

@@ -33,7 +33,6 @@



please add description in commit message

sopel39 · 2022-09-22T08:40:48Z

...rc/main/java/io/trino/benchto/driver/listeners/measurements/DurationMeasurementProvider.java

+                    "duration",
+                    "MILLISECONDS",
+                    measurable.getQueryDuration().toMillis(),
+                    Collections.singletonMap("scope", "driver")));


use guava immutable collections (do we have guava as dependency?)

sopel39 · 2022-09-22T08:48:38Z

benchto-service/src/main/java/io/trino/benchto/service/model/Metric.java

+    @MapKeyColumn(name = "name")
+    @Column(name = "value")
+    @CollectionTable(name = "metric_attributes", joinColumns = @JoinColumn(name = "metric_id"))
+    private Map<String, String> attributes = newHashMap();


This will probably make it harder to extract specific attributes for a measurement (min, max). This might be a problem when someone is writing a script.

Could we embed attributes somehow for a measurment?
It seems that measurement for a given type will have a fixed list of attributes, so maybe we don't need flexibility of extra metric_attributes table.

sopel39 · 2022-09-22T08:50:02Z

benchto-service/src/main/java/io/trino/benchto/service/BenchmarkService.java

@@ -173,6 +173,8 @@ public void finishExecution(String uniqueName, String benchmarkSequenceId, Strin
    private List<Measurement> normalizeMeasurements(List<FlatMeasurement> input)
    {
        // use a lookup map to avoid building a complex SQL query that compares attributes list
+        // acquire a lock to avoid adding multiple metrics with same name and same attributes


why would there be same metrics with same name and attributes created?

sopel39 · 2022-10-04T10:22:55Z

@nineinchnick are you working on this? Is this required for benchmarking infra

nineinchnick · 2022-10-04T10:27:15Z

@nineinchnick are you working on this? Is this required for benchmarking infra

I'm not, since it's not required. It would only make report queries a bit simpler, as it would have to parse metric names to split out prefixes.

Because this is a breaking change, I want to prepare responses to all the comments I got, but it's a low priority.

cla-bot bot added the cla-signed label Sep 14, 2022

nineinchnick force-pushed the metric-attributes branch 3 times, most recently from 08b0527 to 12c9ec0 Compare September 14, 2022 08:25

MiguelWeezardo reviewed Sep 14, 2022

View reviewed changes

nineinchnick force-pushed the metric-attributes branch 2 times, most recently from 3a96397 to 337b6d8 Compare September 16, 2022 12:37

radek-kondziolka reviewed Sep 19, 2022

View reviewed changes

nineinchnick force-pushed the metric-attributes branch 3 times, most recently from 990dd8e to 008cd61 Compare September 21, 2022 11:27

nineinchnick mentioned this pull request Sep 21, 2022

Use PostgreSQL in all integration tests, instead of H2 #46

Merged

nineinchnick added 4 commits September 21, 2022 16:54

Add metric attributes to measurements in the driver

23cd790

Save metrics normalized from measurements

a75646c

Document metrics in the data model

87c507a

Use advisory locks to avoid duplicate metrics

0f2f138

nineinchnick force-pushed the metric-attributes branch from 008cd61 to 0f2f138 Compare September 21, 2022 14:54

sopel39 reviewed Sep 22, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Normalize metrics from measurements #45

Normalize metrics from measurements #45

nineinchnick commented Sep 14, 2022

nineinchnick commented Sep 19, 2022

radek-kondziolka left a comment

radek-kondziolka Sep 19, 2022

nineinchnick Sep 19, 2022

radek-kondziolka Sep 19, 2022

nineinchnick Sep 19, 2022

sopel39 Sep 22, 2022

radek-kondziolka Sep 22, 2022

sopel39 Sep 22, 2022

sopel39 Sep 22, 2022

sopel39 Sep 22, 2022

sopel39 Sep 22, 2022

sopel39 Sep 22, 2022

sopel39 Sep 22, 2022

sopel39 Sep 22, 2022

sopel39 Sep 22, 2022

sopel39 commented Oct 4, 2022

nineinchnick commented Oct 4, 2022

Normalize metrics from measurements #45

Are you sure you want to change the base?

Normalize metrics from measurements #45

Conversation

nineinchnick commented Sep 14, 2022

nineinchnick commented Sep 19, 2022

radek-kondziolka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sopel39 commented Oct 4, 2022

nineinchnick commented Oct 4, 2022