-
Notifications
You must be signed in to change notification settings - Fork 721
metrics-proxy: add osVersion to the alive metric via a config-driven host-dimension mapping #37254
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
Changes from all commits
96181d7
f9097a5
2a86710
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,7 @@ | |
|
|
||
| import ai.vespa.metricsproxy.core.ConfiguredMetric; | ||
| import ai.vespa.metricsproxy.core.MetricsConsumers; | ||
| import ai.vespa.metricsproxy.metric.dimensions.MetricDimensionMapping; | ||
| import ai.vespa.metricsproxy.metric.model.DimensionId; | ||
| import ai.vespa.metricsproxy.metric.model.MetricId; | ||
| import ai.vespa.metricsproxy.metric.model.MetricsPacket; | ||
|
|
@@ -39,9 +40,16 @@ public class ExternalMetrics { | |
|
|
||
| private volatile List<MetricsPacket.Builder> metrics = new ArrayList<>(); | ||
| private final MetricsConsumers consumers; | ||
| private final MetricDimensionMapping dimensionMapping; | ||
|
|
||
| public ExternalMetrics(MetricsConsumers consumers) { | ||
| public ExternalMetrics(MetricsConsumers consumers, MetricDimensionMapping dimensionMapping) { | ||
| this.consumers = consumers; | ||
| this.dimensionMapping = dimensionMapping; | ||
| } | ||
|
|
||
| /** The host dimensions allowed on metrics from the given service (explicit mapping, else default). */ | ||
| public Set<DimensionId> allowedHostDimensions(ServiceId serviceId) { | ||
| return dimensionMapping.allowedFor(serviceId); | ||
| } | ||
|
|
||
| public List<MetricsPacket.Builder> getMetrics() { | ||
|
|
@@ -56,6 +64,7 @@ public void setExtraMetrics(List<MetricsPacket.Builder> externalPackets) { | |
| externalPackets.forEach(packet -> packet.addConsumers(consumers.getAllConsumers()) | ||
| .retainMetrics(metricsToRetain()) | ||
| .applyOutputNames(outputNamesById())); | ||
| externalPackets.forEach(this::stripDisallowedHostDimensions); | ||
| metrics = List.copyOf(externalPackets); | ||
| } | ||
|
|
||
|
|
@@ -92,4 +101,33 @@ public static Map<DimensionId, String> extractConfigserverDimensions(Collection< | |
| return dimensions; | ||
| } | ||
|
|
||
| /** | ||
| * Extracts the host-level dimensions (host, parentHostname, osVersion) from the given packets. | ||
| * These are harvested separately from {@link #extractConfigserverDimensions} (role, state) so they | ||
| * can be applied to specific metrics via the metric-to-dimension mapping, rather than globally. | ||
| * If the same dimension exists in multiple packets, this implementation gives no guarantees | ||
| * about which value is returned. | ||
| */ | ||
| public Map<DimensionId, String> extractHostDimensions(Collection<MetricsPacket.Builder> packets) { | ||
| Map<DimensionId, String> dimensions = new HashMap<>(); | ||
| for (MetricsPacket.Builder packet : packets) { | ||
| dimensions.putAll(packet.build().dimensions()); | ||
| } | ||
| dimensions.keySet().retainAll(dimensionMapping.managedDimensions()); | ||
| return dimensions; | ||
| } | ||
|
|
||
| /** | ||
| * Removes host dimensions not allowed for the packet's service, e.g. strips 'osVersion' from | ||
| * carrier packets (vespa.node etc.) so it only remains where the mapping allows it (host_life). | ||
| * Non-host dimensions (role, state, ...) are left untouched. | ||
| */ | ||
| private void stripDisallowedHostDimensions(MetricsPacket.Builder packet) { | ||
| Set<DimensionId> allowed = allowedHostDimensions(packet.getServiceId()); | ||
| Set<DimensionId> managed = dimensionMapping.managedDimensions(); | ||
| Set<DimensionId> retained = packet.getDimensionIds(); | ||
| retained.removeIf(id -> managed.contains(id) && ! allowed.contains(id)); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This mutates
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair to flag, but I think this is within the method's documented contract rather than an assumption.
It hands back a fresh LinkedHashSet (not a view over the builder's internal map), intended for exactly this get → mutate → retainDimensions() pattern. So retained.removeIf(...) only mutates that copy, never the builder. Existing block in That said, if you'd rather this code not depend on that contract, I can to wrap it in a local new HashSet<>(...)..
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No that's fine. Reviewing such code is harder since the code isn't obviously correct. So usually code does the wrapping client-side. It would also have been fine if the method name had been clearer (e.g. |
||
| packet.retainDimensions(retained); | ||
| } | ||
|
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,51 @@ | ||
| // Copyright Vespa.ai. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. | ||
| package ai.vespa.metricsproxy.metric.dimensions; | ||
|
|
||
| import ai.vespa.metricsproxy.metric.model.DimensionId; | ||
| import ai.vespa.metricsproxy.metric.model.ServiceId; | ||
|
|
||
| import java.util.HashSet; | ||
| import java.util.Map; | ||
| import java.util.Set; | ||
|
|
||
| import static ai.vespa.metricsproxy.metric.model.DimensionId.toDimensionId; | ||
| import static ai.vespa.metricsproxy.metric.model.ServiceId.toServiceId; | ||
| import static java.util.stream.Collectors.toUnmodifiableMap; | ||
| import static java.util.stream.Collectors.toUnmodifiableSet; | ||
|
|
||
| /** | ||
| * Maps a metric packet's service to the host dimensions allowed on it. Services not explicitly | ||
| * listed fall back to the default set. Used to harvest host dimensions from external (host-admin) | ||
| * packets and to keep each such dimension only on the metrics the mapping allows. | ||
| * | ||
| * @author onur | ||
| */ | ||
| public class MetricDimensionMapping { | ||
|
|
||
| private final Set<DimensionId> defaultDimensions; | ||
| private final Map<ServiceId, Set<DimensionId>> dimensionsByService; | ||
| private final Set<DimensionId> managedDimensions; | ||
|
|
||
| public MetricDimensionMapping(MetricDimensionMappingConfig config) { | ||
| defaultDimensions = config.defaultDimension().stream() | ||
| .map(DimensionId::toDimensionId) | ||
| .collect(toUnmodifiableSet()); | ||
| dimensionsByService = config.service().entrySet().stream().collect(toUnmodifiableMap( | ||
| entry -> toServiceId(entry.getKey()), | ||
| entry -> entry.getValue().dimension().stream().map(DimensionId::toDimensionId).collect(toUnmodifiableSet()))); | ||
| Set<DimensionId> all = new HashSet<>(defaultDimensions); | ||
| dimensionsByService.values().forEach(all::addAll); | ||
| managedDimensions = Set.copyOf(all); | ||
| } | ||
|
|
||
| /** Host dimensions allowed on metrics from the given service (explicit mapping, else default). */ | ||
| public Set<DimensionId> allowedFor(ServiceId serviceId) { | ||
| return dimensionsByService.getOrDefault(serviceId, defaultDimensions); | ||
| } | ||
|
|
||
| /** The union of all dimensions the mapping manages; only these are harvested and stripped. */ | ||
| public Set<DimensionId> managedDimensions() { | ||
| return managedDimensions; | ||
| } | ||
|
|
||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| # Copyright Vespa.ai. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. | ||
| package=ai.vespa.metricsproxy.metric.dimensions | ||
|
|
||
| # Host dimensions allowed on metrics whose service is not explicitly listed below. | ||
| defaultDimension[] string | ||
|
|
||
| # Per-service overrides, keyed by service name: the host dimensions allowed on metrics from that service. | ||
| service{}.dimension[] string |
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.
We want to add the OS version dimension to the host_life metric for everyone?
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.
Yes, we want it to be a generic feature for everyone. Also it will be useful for us on our dashboards.