-
Notifications
You must be signed in to change notification settings - Fork 29
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Normalize metrics from measurements #45
base: master
Are you sure you want to change the base?
Conversation
08b0527
to
12c9ec0
Compare
benchto-driver/src/main/java/io/trino/benchto/driver/graphite/GraphiteMetricsLoader.java
Outdated
Show resolved
Hide resolved
benchto-driver/src/main/java/io/trino/benchto/driver/graphite/GraphiteMetricsLoader.java
Show resolved
Hide resolved
benchto-driver/src/main/java/io/trino/benchto/driver/presto/PrestoClient.java
Outdated
Show resolved
Hide resolved
benchto-driver/src/test/java/io/trino/benchto/driver/DriverAppIntegrationTest.java
Outdated
Show resolved
Hide resolved
benchto-service/src/main/java/io/trino/benchto/service/model/Measurement.java
Show resolved
Hide resolved
benchto-service/src/main/resources/db/migration/V006__metrics.sql
Outdated
Show resolved
Hide resolved
benchto-service/src/main/resources/db/migration/V006__metrics.sql
Outdated
Show resolved
Hide resolved
3a96397
to
337b6d8
Compare
@przemekak @sopel39 @radek-starburst PTAL. There are breaking changes here but I tried to preserve some backward compatibility. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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"))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like a factory method. Maybe something like Measurement.of
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, but I'd rather do this in a separate PR.
benchto-service/src/main/java/io/trino/benchto/service/model/Metric.java
Outdated
Show resolved
Hide resolved
benchto-service/src/main/java/io/trino/benchto/service/BenchmarkService.java
Show resolved
Hide resolved
@@ -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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
990dd8e
to
008cd61
Compare
008cd61
to
0f2f138
Compare
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), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this will break scripts (jupyther). I think it's fine, but needs to be announced
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not only scripts but our workflows as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean alerts?
@@ -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"))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
METRIC_SCOPE
instead of cluster
?
@@ -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())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -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")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prestoQuery
-> query
@@ -33,7 +33,6 @@ | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add description in commit message
"duration", | ||
"MILLISECONDS", | ||
measurable.getQueryDuration().toMillis(), | ||
Collections.singletonMap("scope", "driver"))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use guava immutable collections (do we have guava as dependency?)
@MapKeyColumn(name = "name") | ||
@Column(name = "value") | ||
@CollectionTable(name = "metric_attributes", joinColumns = @JoinColumn(name = "metric_id")) | ||
private Map<String, String> attributes = newHashMap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why would there be same metrics with same name and attributes created?
@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. |
This PR extracts
name
andunit
from measurements into anothermetrics
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
andmeasurement.unit
columns are not removed, but the name format is changed fromcluster-cpu_max
tocpu {scope=cluster,aggregate=max}
.This change matches how other metric monitoring system stores data, using labels (Prometheus) or attributes (OpenTelemetry).