diff options
5 files changed, 142 insertions, 47 deletions
diff --git a/container-core/src/main/java/com/yahoo/container/jdisc/LoggingRequestHandler.java b/container-core/src/main/java/com/yahoo/container/jdisc/LoggingRequestHandler.java index 705092ef12e..62dc81aa103 100644 --- a/container-core/src/main/java/com/yahoo/container/jdisc/LoggingRequestHandler.java +++ b/container-core/src/main/java/com/yahoo/container/jdisc/LoggingRequestHandler.java @@ -240,37 +240,43 @@ public abstract class LoggingRequestHandler extends ThreadedHttpRequestHandler { renderStartTime, commitStartTime, endTime, - jdiscRequest.getUri().toString(), + getUri(jdiscRequest), extendedResponse.getParsedQuery(), extendedResponse.getTiming()); Optional<AccessLogEntry> jdiscRequestAccessLogEntry = AccessLoggingRequestHandler.getAccessLogEntry(jdiscRequest); - + AccessLogEntry entry; if (jdiscRequestAccessLogEntry.isPresent()) { - // This means we are running with Jetty, not Netty. + // The request is created by JDisc http layer (Jetty) // Actual logging will be done by the Jetty integration; here, we just need to populate. - httpResponse.populateAccessLogEntry(jdiscRequestAccessLogEntry.get()); - return; + entry = jdiscRequestAccessLogEntry.get(); + } else { + // Not running on JDisc http layer (Jetty), e.g unit tests + AccessLogEntry accessLogEntry = new AccessLogEntry(); + populateAccessLogEntryNotCreatedByHttpServer( + accessLogEntry, + jdiscRequest, + extendedResponse.getTiming(), + httpRequest.getUri().toString(), + commitStartTime, + startTime, + rendererWiring.written(), + httpResponse.getStatus()); + accessLog.log(accessLogEntry); + entry = accessLogEntry; } + httpResponse.populateAccessLogEntry(entry); + } - // We are running without Jetty. No access logging will be done at container level, so we do it here. - // TODO: Remove when netty support is removed. - - AccessLogEntry accessLogEntry = new AccessLogEntry(); - - populateAccessLogEntryNotCreatedByHttpServer( - accessLogEntry, - jdiscRequest, - extendedResponse.getTiming(), - httpRequest.getUri().toString(), - commitStartTime, - startTime, - rendererWiring.written(), - httpResponse.getStatus()); - httpResponse.populateAccessLogEntry(accessLogEntry); - - accessLog.log(accessLogEntry); + private String getUri(com.yahoo.jdisc.http.HttpRequest jdiscRequest) { + URI uri = jdiscRequest.getUri(); + StringBuilder builder = new StringBuilder(uri.getPath()); + String query = uri.getQuery(); + if (query != null && !query.isBlank()) { + builder.append('?').append(query); + } + return builder.toString(); } } diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/resource/ResourceAllocation.java b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/resource/ResourceAllocation.java index bea34567752..205007978e9 100644 --- a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/resource/ResourceAllocation.java +++ b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/resource/ResourceAllocation.java @@ -1,6 +1,8 @@ // Copyright 2019 Oath Inc. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.hosted.controller.api.integration.resource; +import java.util.Objects; + /** * An allocation of node resources. * @@ -46,5 +48,22 @@ public class ResourceAllocation { return new ResourceAllocation(cpuCores * multiplicand, memoryGb * multiplicand, diskGb * multiplicand); } + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (!(o instanceof ResourceAllocation)) return false; + + ResourceAllocation other = (ResourceAllocation) o; + return Double.compare(this.cpuCores, other.cpuCores) == 0 && + Double.compare(this.memoryGb, other.memoryGb) == 0 && + Double.compare(this.diskGb, other.diskGb) == 0; + + } + + @Override + public int hashCode() { + return Objects.hash(cpuCores, memoryGb, diskGb); + } + } diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/resource/ResourceSnapshot.java b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/resource/ResourceSnapshot.java index cfe0f18260a..70aaf5de112 100644 --- a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/resource/ResourceSnapshot.java +++ b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/resource/ResourceSnapshot.java @@ -8,6 +8,7 @@ import com.yahoo.vespa.hosted.controller.api.integration.configserver.Node; import java.time.Instant; import java.util.List; +import java.util.Objects; import java.util.Set; import java.util.stream.Collectors; @@ -70,4 +71,20 @@ public class ResourceSnapshot { return zoneId; } + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (!(o instanceof ResourceSnapshot)) return false; + + ResourceSnapshot other = (ResourceSnapshot) o; + return this.applicationId.equals(other.applicationId) && + this.resourceAllocation.equals(other.resourceAllocation) && + this.timestamp.equals(other.timestamp) && + this.zoneId.equals(other.zoneId); + } + + @Override + public int hashCode(){ + return Objects.hash(applicationId, resourceAllocation, timestamp, zoneId); + } } diff --git a/metrics-proxy/src/main/java/ai/vespa/metricsproxy/http/application/NodeMetricsClient.java b/metrics-proxy/src/main/java/ai/vespa/metricsproxy/http/application/NodeMetricsClient.java index 8987ca25be1..8b397bc65f8 100644 --- a/metrics-proxy/src/main/java/ai/vespa/metricsproxy/http/application/NodeMetricsClient.java +++ b/metrics-proxy/src/main/java/ai/vespa/metricsproxy/http/application/NodeMetricsClient.java @@ -16,17 +16,21 @@ import java.io.IOException; import java.time.Clock; import java.time.Duration; import java.time.Instant; +import java.util.HashMap; import java.util.List; +import java.util.Map; import java.util.logging.Logger; -import static ai.vespa.metricsproxy.http.ValuesFetcher.DEFAULT_PUBLIC_CONSUMER_ID; import static com.yahoo.log.LogLevel.DEBUG; import static java.util.Collections.emptyList; /** - * This class is used to retrieve metrics from a single Vespa node over http. - * Keeps and serves a snapshot of the node's metrics, with a fixed TTL, to - * avoid unnecessary load on metrics proxies. + * Retrieves metrics from a single Vespa node over http. To avoid unnecessary load on metrics + * proxies, a cached snapshot per consumer is retained and served for a fixed TTL period. + * Upon failure to retrieve metrics, an empty snapshot is cached. + * + * This class assumes that the consumer id is a valid and existing one, which is already + * ensured by the {@link ApplicationMetricsHandler}. * * @author gjoranv */ @@ -39,41 +43,39 @@ public class NodeMetricsClient { private final HttpClient httpClient; private final Clock clock; - private List<MetricsPacket.Builder> metrics = emptyList(); - private Instant metricsTimestamp = Instant.EPOCH; + private final Map<ConsumerId, Snapshot> snapshots = new HashMap<>(); private long snapshotsRetrieved = 0; - public NodeMetricsClient(HttpClient httpClient, Node node, Clock clock) { + NodeMetricsClient(HttpClient httpClient, Node node, Clock clock) { this.httpClient = httpClient; this.node = node; this.clock = clock; } - public List<MetricsPacket.Builder> getMetrics() { - return getMetrics(DEFAULT_PUBLIC_CONSUMER_ID); - } - public List<MetricsPacket.Builder> getMetrics(ConsumerId consumer) { - if (Instant.now(clock).isAfter(metricsTimestamp.plus(METRICS_TTL))) { - retrieveMetrics(consumer); + var currentSnapshot = snapshots.get(consumer); + if (currentSnapshot == null || currentSnapshot.isStale(clock) || currentSnapshot.metrics.isEmpty()) { + Snapshot snapshot = retrieveMetrics(consumer); + snapshots.put(consumer, snapshot); + return snapshot.metrics; + } else { + return snapshots.get(consumer).metrics; } - return metrics; } - private void retrieveMetrics(ConsumerId consumer) { + private Snapshot retrieveMetrics(ConsumerId consumer) { String metricsUri = node.metricsUri(consumer).toString(); log.log(DEBUG, () -> "Retrieving metrics from host " + metricsUri); try { String metricsJson = httpClient.execute(new HttpGet(metricsUri), new BasicResponseHandler()); - metrics = GenericJsonUtil.toMetricsPackets(metricsJson); - metricsTimestamp = Instant.now(clock); + var newMetrics = GenericJsonUtil.toMetricsPackets(metricsJson); snapshotsRetrieved ++; - log.log(DEBUG, () -> "Successfully retrieved " + metrics.size() + " metrics packets from " + metricsUri); - + log.log(DEBUG, () -> "Successfully retrieved " + newMetrics.size() + " metrics packets from " + metricsUri); + return new Snapshot(Instant.now(clock), newMetrics); } catch (IOException e) { log.warning("Unable to retrieve metrics from " + metricsUri + ": " + Exceptions.toMessageString(e)); - metrics = emptyList(); + return new Snapshot(Instant.now(clock), emptyList()); } } @@ -81,4 +83,23 @@ public class NodeMetricsClient { return snapshotsRetrieved; } + + /** + * Convenience class for storing a metrics snapshot with its timestamp. + */ + static class Snapshot { + + final Instant timestamp; + final List<MetricsPacket.Builder> metrics; + + Snapshot(Instant timestamp, List<MetricsPacket.Builder> metrics) { + this.timestamp = timestamp; + this.metrics = metrics; + } + + boolean isStale(Clock clock) { + return Instant.now(clock).isAfter(timestamp.plus(METRICS_TTL)); + } + } + } diff --git a/metrics-proxy/src/test/java/ai/vespa/metricsproxy/http/application/NodeMetricsClientTest.java b/metrics-proxy/src/test/java/ai/vespa/metricsproxy/http/application/NodeMetricsClientTest.java index d028db93d43..0c0b0749011 100644 --- a/metrics-proxy/src/test/java/ai/vespa/metricsproxy/http/application/NodeMetricsClientTest.java +++ b/metrics-proxy/src/test/java/ai/vespa/metricsproxy/http/application/NodeMetricsClientTest.java @@ -20,11 +20,15 @@ import java.util.List; import static ai.vespa.metricsproxy.TestUtil.getFileContents; import static ai.vespa.metricsproxy.http.ValuesFetcher.DEFAULT_PUBLIC_CONSUMER_ID; +import static ai.vespa.metricsproxy.metric.model.ConsumerId.toConsumerId; +import static ai.vespa.metricsproxy.metric.model.MetricId.toMetricId; import static com.github.tomakehurst.wiremock.client.WireMock.aResponse; +import static com.github.tomakehurst.wiremock.client.WireMock.equalTo; import static com.github.tomakehurst.wiremock.client.WireMock.get; import static com.github.tomakehurst.wiremock.client.WireMock.urlPathEqualTo; import static com.github.tomakehurst.wiremock.core.WireMockConfiguration.options; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; /** * Two optimizations worth noting: @@ -40,6 +44,10 @@ public class NodeMetricsClientTest { private static final String RESPONSE = getFileContents(TEST_FILE); private static final CloseableHttpClient httpClient = HttpClients.createDefault(); + private static final String CPU_METRIC = "cpu.util"; + private static final String REPLACED_CPU_METRIC = "replaced_cpu_util"; + private static final String CUSTOM_CONSUMER = "custom-consumer"; + private static Node node; private ManualClock clock; @@ -54,6 +62,17 @@ public class NodeMetricsClientTest { URI metricsUri = node.metricsUri(DEFAULT_PUBLIC_CONSUMER_ID); wireMockRule.stubFor(get(urlPathEqualTo(metricsUri.getPath())) .willReturn(aResponse().withBody(RESPONSE))); + + wireMockRule.stubFor(get(urlPathEqualTo(metricsUri.getPath())) + .withQueryParam("consumer", equalTo(DEFAULT_PUBLIC_CONSUMER_ID.id)) + .willReturn(aResponse().withBody(RESPONSE))); + + // Add a slightly different response for a custom consumer. + String myConsumerResponse = RESPONSE.replaceAll(CPU_METRIC, REPLACED_CPU_METRIC); + wireMockRule.stubFor(get(urlPathEqualTo(metricsUri.getPath())) + .withQueryParam("consumer", equalTo(CUSTOM_CONSUMER)) + .willReturn(aResponse().withBody(myConsumerResponse))); + } @Before @@ -69,29 +88,42 @@ public class NodeMetricsClientTest { @Test public void metrics_are_retrieved_upon_first_request() { - List<MetricsPacket.Builder> metrics = nodeMetricsClient.getMetrics(); + List<MetricsPacket.Builder> metrics = nodeMetricsClient.getMetrics(DEFAULT_PUBLIC_CONSUMER_ID); assertEquals(1, nodeMetricsClient.snapshotsRetrieved()); assertEquals(4, metrics.size()); } @Test public void cached_metrics_are_used_when_ttl_has_not_expired() { - nodeMetricsClient.getMetrics(); + nodeMetricsClient.getMetrics(DEFAULT_PUBLIC_CONSUMER_ID); assertEquals(1, nodeMetricsClient.snapshotsRetrieved()); clock.advance(NodeMetricsClient.METRICS_TTL.minusMillis(1)); - nodeMetricsClient.getMetrics(); + nodeMetricsClient.getMetrics(DEFAULT_PUBLIC_CONSUMER_ID); assertEquals(1, nodeMetricsClient.snapshotsRetrieved()); } @Test public void metrics_are_refreshed_when_ttl_has_expired() { - nodeMetricsClient.getMetrics(); + nodeMetricsClient.getMetrics(DEFAULT_PUBLIC_CONSUMER_ID); assertEquals(1, nodeMetricsClient.snapshotsRetrieved()); clock.advance(NodeMetricsClient.METRICS_TTL.plusMillis(1)); - nodeMetricsClient.getMetrics(); + nodeMetricsClient.getMetrics(DEFAULT_PUBLIC_CONSUMER_ID); assertEquals(2, nodeMetricsClient.snapshotsRetrieved()); } + @Test + public void metrics_for_different_consumers_are_cached_separately() { + List<MetricsPacket.Builder> defaultMetrics = nodeMetricsClient.getMetrics(DEFAULT_PUBLIC_CONSUMER_ID); + assertEquals(1, nodeMetricsClient.snapshotsRetrieved()); + assertEquals(4, defaultMetrics.size()); + + List<MetricsPacket.Builder> customMetrics = nodeMetricsClient.getMetrics(toConsumerId(CUSTOM_CONSUMER)); + assertEquals(2, nodeMetricsClient.snapshotsRetrieved()); + assertEquals(4, customMetrics.size()); + + MetricsPacket replacedCpuMetric = customMetrics.get(0).build(); + assertTrue(replacedCpuMetric.metrics().containsKey(toMetricId(REPLACED_CPU_METRIC))); + } } |