Skip to content

Commit 2a86710

Browse files
onurkaracaliclaude
andcommitted
metrics-proxy: config-driven mapping as a map keyed by service (review)
Address gjoranv's review on metric-dimension-mapping.def: - Change `mapping[].service` + `mapping[].dimension[]` to a map keyed by service name: `service{}.dimension[]`. Models service -> dimensions directly, drops the redundant service field, and makes duplicate-service entries impossible (the array form silently allowed duplicates). - Use the generated Consumer<Builder> lambda overload at the three build sites (TestUtil.standardDimensionMapping, MetricDimensionMappingTest, MetricsProxyContainerCluster.getConfig) instead of building the inner struct explicitly. No behaviour change: MetricDimensionMapping.allowedFor/managedDimensions and all of ExternalMetrics/MetricsManager are unchanged; only how the config is expressed and parsed changed. metrics-proxy 155 green, config-model metricsproxy 23 green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent f9097a5 commit 2a86710

6 files changed

Lines changed: 11 additions & 17 deletions

File tree

config-model/src/main/java/com/yahoo/vespa/model/admin/metricsproxy/MetricsProxyContainerCluster.java

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -203,8 +203,7 @@ public void getConfig(MetricDimensionMappingConfig.Builder builder) {
203203
if (isHostedVespa()) {
204204
builder.defaultDimension(PublicDimensions.HOSTNAME);
205205
builder.defaultDimension(PublicDimensions.PARENT_HOSTNAME);
206-
builder.mapping(new MetricDimensionMappingConfig.Mapping.Builder()
207-
.service(HOST_LIFE_SERVICE)
206+
builder.service(HOST_LIFE_SERVICE, s -> s
208207
.dimension(PublicDimensions.HOSTNAME)
209208
.dimension(PublicDimensions.PARENT_HOSTNAME)
210209
.dimension(PublicDimensions.OS_VERSION));

config-model/src/test/java/com/yahoo/vespa/model/admin/metricsproxy/MetricsProxyContainerClusterTest.java

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -118,19 +118,17 @@ void hosted_application_propagates_metric_dimension_mapping() {
118118

119119
assertEquals(List.of(PublicDimensions.HOSTNAME, PublicDimensions.PARENT_HOSTNAME), config.defaultDimension());
120120

121-
assertEquals(1, config.mapping().size());
122-
MetricDimensionMappingConfig.Mapping mapping = config.mapping().get(0);
123-
assertEquals("host_life", mapping.service());
121+
assertEquals(Set.of("host_life"), config.service().keySet());
124122
assertEquals(List.of(PublicDimensions.HOSTNAME, PublicDimensions.PARENT_HOSTNAME, PublicDimensions.OS_VERSION),
125-
mapping.dimension());
123+
config.service("host_life").dimension());
126124
}
127125

128126
@Test
129127
void self_hosted_application_has_empty_metric_dimension_mapping() {
130128
VespaModel model = getModel(servicesWithAdminOnly(), self_hosted);
131129
MetricDimensionMappingConfig config = getMetricDimensionMappingConfig(model);
132130
assertTrue(config.defaultDimension().isEmpty());
133-
assertTrue(config.mapping().isEmpty());
131+
assertTrue(config.service().isEmpty());
134132
}
135133

136134
@Test

metrics-proxy/src/main/java/ai/vespa/metricsproxy/metric/dimensions/MetricDimensionMapping.java

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -30,9 +30,9 @@ public MetricDimensionMapping(MetricDimensionMappingConfig config) {
3030
defaultDimensions = config.defaultDimension().stream()
3131
.map(DimensionId::toDimensionId)
3232
.collect(toUnmodifiableSet());
33-
dimensionsByService = config.mapping().stream().collect(toUnmodifiableMap(
34-
mapping -> toServiceId(mapping.service()),
35-
mapping -> mapping.dimension().stream().map(DimensionId::toDimensionId).collect(toUnmodifiableSet())));
33+
dimensionsByService = config.service().entrySet().stream().collect(toUnmodifiableMap(
34+
entry -> toServiceId(entry.getKey()),
35+
entry -> entry.getValue().dimension().stream().map(DimensionId::toDimensionId).collect(toUnmodifiableSet())));
3636
Set<DimensionId> all = new HashSet<>(defaultDimensions);
3737
dimensionsByService.values().forEach(all::addAll);
3838
managedDimensions = Set.copyOf(all);

metrics-proxy/src/main/resources/configdefinitions/metric-dimension-mapping.def

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,5 @@ package=ai.vespa.metricsproxy.metric.dimensions
44
# Host dimensions allowed on metrics whose service is not explicitly listed below.
55
defaultDimension[] string
66

7-
# Per-service overrides: the host dimensions allowed on metrics from this service.
8-
mapping[].service string
9-
mapping[].dimension[] string
7+
# Per-service overrides, keyed by service name: the host dimensions allowed on metrics from that service.
8+
service{}.dimension[] string

metrics-proxy/src/test/java/ai/vespa/metricsproxy/TestUtil.java

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -39,8 +39,7 @@ public static MetricDimensionMapping standardDimensionMapping() {
3939
return new MetricDimensionMapping(new MetricDimensionMappingConfig.Builder()
4040
.defaultDimension("host")
4141
.defaultDimension("parentHostname")
42-
.mapping(new MetricDimensionMappingConfig.Mapping.Builder()
43-
.service("host_life")
42+
.service("host_life", s -> s
4443
.dimension("host")
4544
.dimension("parentHostname")
4645
.dimension("osVersion"))

metrics-proxy/src/test/java/ai/vespa/metricsproxy/metric/dimensions/MetricDimensionMappingTest.java

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,8 +20,7 @@ public void uses_default_for_unmapped_services_and_explicit_mapping_for_listed_s
2020
new MetricDimensionMappingConfig.Builder()
2121
.defaultDimension("host")
2222
.defaultDimension("parentHostname")
23-
.mapping(new MetricDimensionMappingConfig.Mapping.Builder()
24-
.service("host_life")
23+
.service("host_life", s -> s
2524
.dimension("host")
2625
.dimension("parentHostname")
2726
.dimension("osVersion"))

0 commit comments

Comments
 (0)