You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "magibney (via GitHub)" <gi...@apache.org> on 2024/07/24 16:22:32 UTC
[PR] SOLR-17378: Add a RequestHandler metric for "number of outstanding concurrent requests" [solr]
magibney opened a new pull request, #2591:
URL: https://github.com/apache/solr/pull/2591
https://issues.apache.org/jira/browse/SOLR-17378
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17378: Add a RequestHandler metric for "number of outstanding concurrent requests" [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #2591:
URL: https://github.com/apache/solr/pull/2591#discussion_r1690411012
##########
solr/core/src/java/org/apache/solr/handler/RequestHandlerBase.java:
##########
@@ -246,20 +250,24 @@ public void handleRequest(SolrQueryRequest req, SolrQueryResponse rsp) {
processErrorMetricsOnException(normalized, metrics);
rsp.setException(normalized);
} finally {
- long elapsed = timer.stop();
- metrics.totalTime.inc(elapsed);
-
- if (publishCpuTime && threadCpuTimer != null) {
- Optional<Long> cpuTime = threadCpuTimer.getElapsedCpuMs();
- if (cpuTime.isPresent()) {
- // add CPU_TIME if not already added by SearchHandler
- NamedList<Object> header = rsp.getResponseHeader();
- if (header != null) {
- if (header.get(ThreadCpuTimer.CPU_TIME) == null) {
- header.add(ThreadCpuTimer.CPU_TIME, cpuTime.get());
+ try {
+ metrics.concurrency.update(-1);
Review Comment:
using a try-finally on calling this just feels weird for something that looks like it ought to never fail. I'd rather make it bullet-proof and ditch try-finally paranoia (adds control-flow complexity, cyclomatic complexity)
##########
solr/core/src/java/org/apache/solr/metrics/MaxHistogram.java:
##########
@@ -0,0 +1,191 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.metrics;
+
+import com.codahale.metrics.Clock;
+import com.codahale.metrics.Histogram;
+import com.codahale.metrics.Reservoir;
+import com.codahale.metrics.Snapshot;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicLong;
+import java.util.function.Function;
+
+/**
+ * A special Histogram that conceptually updates at regular intervals, according to the max value of
+ * a counter over the preceding interval. This is appropriate for lazily tracking ambient values
+ * (like "number of outstanding active requests"), where we are interested in:
+ *
+ * <ul>
+ * <li><i>max</i> values over a unit-granularity time-range (as opposed to arbitrary values that
+ * could be retrieved by sampling)
+ * <li>aggregate values not skewed by activity (i.e., the number of events triggering value
+ * recording should not matter -- thus we want to record values at a constant rate)
+ * <li>not dedicating a thread to actually recording values at regular wall-clock intervals
+ * </ul>
+ *
+ * <p>The {@link #update(long)} methods should be invoked with positive and negative args, analogous
+ * to {@link com.codahale.metrics.Counter#inc(long)} and {@link
+ * com.codahale.metrics.Counter#dec(long)}, respectively. The effect is the same as a histogram
+ * regularly sampling a counter's max value over a given interval; but in implementation, samples
+ * are lazily backfilled upon write (value change {@link #update(long)}) or read ({@link
+ * #getSnapshot()}) operations.
+ */
+public class MaxHistogram extends Histogram {
+
+ private final long intervalNanos;
+
+ private final AtomicLong val = new AtomicLong();
Review Comment:
I was going to say "LongAdder is a better fit to metrics use-case as it's update-often, read-rarely". But on update() below it's checked on every call; so much for that.
##########
solr/core/src/java/org/apache/solr/jersey/RequestMetricHandling.java:
##########
Review Comment:
I like how this class layers on nicely (it's composeable). It's the opposite of RequestHandlerBase with embedded indentation and try-finally.
##########
solr/core/src/java/org/apache/solr/metrics/MaxHistogram.java:
##########
@@ -0,0 +1,191 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.metrics;
+
+import com.codahale.metrics.Clock;
+import com.codahale.metrics.Histogram;
+import com.codahale.metrics.Reservoir;
+import com.codahale.metrics.Snapshot;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicLong;
+import java.util.function.Function;
+
+/**
+ * A special Histogram that conceptually updates at regular intervals, according to the max value of
+ * a counter over the preceding interval. This is appropriate for lazily tracking ambient values
+ * (like "number of outstanding active requests"), where we are interested in:
+ *
+ * <ul>
+ * <li><i>max</i> values over a unit-granularity time-range (as opposed to arbitrary values that
+ * could be retrieved by sampling)
+ * <li>aggregate values not skewed by activity (i.e., the number of events triggering value
+ * recording should not matter -- thus we want to record values at a constant rate)
+ * <li>not dedicating a thread to actually recording values at regular wall-clock intervals
+ * </ul>
+ *
+ * <p>The {@link #update(long)} methods should be invoked with positive and negative args, analogous
+ * to {@link com.codahale.metrics.Counter#inc(long)} and {@link
+ * com.codahale.metrics.Counter#dec(long)}, respectively. The effect is the same as a histogram
+ * regularly sampling a counter's max value over a given interval; but in implementation, samples
+ * are lazily backfilled upon write (value change {@link #update(long)}) or read ({@link
+ * #getSnapshot()}) operations.
+ */
+public class MaxHistogram extends Histogram {
+
+ private final long intervalNanos;
+
+ private final AtomicLong val = new AtomicLong();
+
+ private final AtomicLong lastUpdate;
+
+ private volatile long lastValue = 0;
+
+ private final AtomicLong maxSinceLastUpdate = new AtomicLong(0);
+
+ private final long maxBackdateNanos;
+
+ private final Clock clock;
+
+ private final RelayClock relayClock;
+
+ private static class RelayClock extends Clock {
Review Comment:
What's a "RelayClock"?
##########
solr/core/src/java/org/apache/solr/metrics/MaxHistogram.java:
##########
@@ -0,0 +1,191 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.metrics;
+
+import com.codahale.metrics.Clock;
+import com.codahale.metrics.Histogram;
+import com.codahale.metrics.Reservoir;
+import com.codahale.metrics.Snapshot;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicLong;
+import java.util.function.Function;
+
+/**
+ * A special Histogram that conceptually updates at regular intervals, according to the max value of
+ * a counter over the preceding interval. This is appropriate for lazily tracking ambient values
+ * (like "number of outstanding active requests"), where we are interested in:
+ *
+ * <ul>
+ * <li><i>max</i> values over a unit-granularity time-range (as opposed to arbitrary values that
+ * could be retrieved by sampling)
+ * <li>aggregate values not skewed by activity (i.e., the number of events triggering value
+ * recording should not matter -- thus we want to record values at a constant rate)
+ * <li>not dedicating a thread to actually recording values at regular wall-clock intervals
+ * </ul>
+ *
+ * <p>The {@link #update(long)} methods should be invoked with positive and negative args, analogous
+ * to {@link com.codahale.metrics.Counter#inc(long)} and {@link
+ * com.codahale.metrics.Counter#dec(long)}, respectively. The effect is the same as a histogram
+ * regularly sampling a counter's max value over a given interval; but in implementation, samples
+ * are lazily backfilled upon write (value change {@link #update(long)}) or read ({@link
+ * #getSnapshot()}) operations.
+ */
+public class MaxHistogram extends Histogram {
+
+ private final long intervalNanos;
+
+ private final AtomicLong val = new AtomicLong();
+
+ private final AtomicLong lastUpdate;
+
+ private volatile long lastValue = 0;
+
+ private final AtomicLong maxSinceLastUpdate = new AtomicLong(0);
+
+ private final long maxBackdateNanos;
+
+ private final Clock clock;
+
+ private final RelayClock relayClock;
+
+ private static class RelayClock extends Clock {
+
+ private long nextTick;
+
+ private RelayClock(long initialTick) {
+ nextTick = initialTick;
+ }
+
+ @Override
+ public long getTick() {
+ return nextTick;
+ }
+
+ public void setTick(long value) {
+ nextTick = value;
+ }
+ }
+
+ /**
+ * Constructs a new {@link MaxHistogram} instance.
+ *
+ * @param intervalMillis this is the unit interval at which values will be recorded into the
+ * underlying histogram (the max value for the immediately preceding interval). This
+ * granularity is somewhat arbitrary, and the default of 1000ms (1s) should be sufficient for
+ * most applications.
+ * @param maxBackdateSeconds limits how far back value updates will be lazily backfilled upon
+ * value change or read access. Without this, the first value change or read access after a
+ * long period of inactivity could result in backfilling an arbitrarily large number of
+ * intervals. This should ordinarily be set to align with the time window of the reservoir (or
+ * 300s/5m for the default configuration of {@link
+ * com.codahale.metrics.ExponentiallyDecayingReservoir}).
+ * @param clock the backing clock to use for the reservoir
+ * @param reservoirFunction because {@link MaxHistogram} internally shims the clock actually used
+ * by the underlying reservoir, the reservoir cannot be constructed directly. This function
+ * supports constructing a reservoir based on the shimmed clock
+ * @return a new {@link MaxHistogram} instance
+ */
+ public static MaxHistogram newInstance(
+ int intervalMillis,
+ int maxBackdateSeconds,
+ Clock clock,
+ Function<Clock, Reservoir> reservoirFunction) {
+ RelayClock relayClock = new RelayClock(clock.getTick());
+ return new MaxHistogram(
+ intervalMillis, maxBackdateSeconds, reservoirFunction.apply(relayClock), relayClock, clock);
+ }
+
+ private MaxHistogram(
+ int intervalMillis,
+ int maxBackdateSeconds,
+ Reservoir reservoir,
+ Clock relayClock,
+ Clock clock) {
+ super(reservoir);
+ this.intervalNanos = TimeUnit.MILLISECONDS.toNanos(intervalMillis);
+ if (maxBackdateSeconds == -1) {
+ this.maxBackdateNanos = Long.MAX_VALUE; // effectively no limit
+ } else if (TimeUnit.SECONDS.toMillis(maxBackdateSeconds) >= intervalMillis) {
+ // align `maxBackdateNanos` to a multiple of `intervalNanos`
+ long requestedMaxBackdate = TimeUnit.SECONDS.toNanos(maxBackdateSeconds);
+ this.maxBackdateNanos = requestedMaxBackdate - (requestedMaxBackdate % intervalNanos);
+ } else {
+ throw new IllegalArgumentException(
+ "maxBackdateSeconds must be -1 (unlimited) or >= "
+ + TimeUnit.MILLISECONDS.toSeconds(intervalMillis)
+ + "; found "
+ + maxBackdateSeconds);
+ }
+ this.relayClock = (RelayClock) relayClock;
Review Comment:
Why is the param not RelayClock then?
##########
solr/solr-ref-guide/modules/deployment-guide/pages/metrics-reporting.adoc:
##########
@@ -231,6 +231,40 @@ This is only valid for the `ExponentiallyDecayingReservoir`.
+
The window size, in seconds, and only valid for the `SlidingTimeWindowReservoir`.
+`<maxHistogram>`::
Review Comment:
this is some hard-core docs... not sure 1% of readers will understand it. Not necessarily a problem; you put thought into this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17378: Add a RequestHandler metric for "number of outstanding concurrent requests" [solr]
Posted by "magibney (via GitHub)" <gi...@apache.org>.
magibney commented on code in PR #2591:
URL: https://github.com/apache/solr/pull/2591#discussion_r1691465927
##########
solr/core/src/java/org/apache/solr/metrics/MaxHistogram.java:
##########
@@ -0,0 +1,191 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.metrics;
+
+import com.codahale.metrics.Clock;
+import com.codahale.metrics.Histogram;
+import com.codahale.metrics.Reservoir;
+import com.codahale.metrics.Snapshot;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicLong;
+import java.util.function.Function;
+
+/**
+ * A special Histogram that conceptually updates at regular intervals, according to the max value of
+ * a counter over the preceding interval. This is appropriate for lazily tracking ambient values
+ * (like "number of outstanding active requests"), where we are interested in:
+ *
+ * <ul>
+ * <li><i>max</i> values over a unit-granularity time-range (as opposed to arbitrary values that
+ * could be retrieved by sampling)
+ * <li>aggregate values not skewed by activity (i.e., the number of events triggering value
+ * recording should not matter -- thus we want to record values at a constant rate)
+ * <li>not dedicating a thread to actually recording values at regular wall-clock intervals
+ * </ul>
+ *
+ * <p>The {@link #update(long)} methods should be invoked with positive and negative args, analogous
+ * to {@link com.codahale.metrics.Counter#inc(long)} and {@link
+ * com.codahale.metrics.Counter#dec(long)}, respectively. The effect is the same as a histogram
+ * regularly sampling a counter's max value over a given interval; but in implementation, samples
+ * are lazily backfilled upon write (value change {@link #update(long)}) or read ({@link
+ * #getSnapshot()}) operations.
+ */
+public class MaxHistogram extends Histogram {
+
+ private final long intervalNanos;
+
+ private final AtomicLong val = new AtomicLong();
+
+ private final AtomicLong lastUpdate;
+
+ private volatile long lastValue = 0;
+
+ private final AtomicLong maxSinceLastUpdate = new AtomicLong(0);
+
+ private final long maxBackdateNanos;
+
+ private final Clock clock;
+
+ private final RelayClock relayClock;
+
+ private static class RelayClock extends Clock {
+
+ private long nextTick;
+
+ private RelayClock(long initialTick) {
+ nextTick = initialTick;
+ }
+
+ @Override
+ public long getTick() {
+ return nextTick;
+ }
+
+ public void setTick(long value) {
+ nextTick = value;
+ }
+ }
+
+ /**
+ * Constructs a new {@link MaxHistogram} instance.
+ *
+ * @param intervalMillis this is the unit interval at which values will be recorded into the
+ * underlying histogram (the max value for the immediately preceding interval). This
+ * granularity is somewhat arbitrary, and the default of 1000ms (1s) should be sufficient for
+ * most applications.
+ * @param maxBackdateSeconds limits how far back value updates will be lazily backfilled upon
+ * value change or read access. Without this, the first value change or read access after a
+ * long period of inactivity could result in backfilling an arbitrarily large number of
+ * intervals. This should ordinarily be set to align with the time window of the reservoir (or
+ * 300s/5m for the default configuration of {@link
+ * com.codahale.metrics.ExponentiallyDecayingReservoir}).
+ * @param clock the backing clock to use for the reservoir
+ * @param reservoirFunction because {@link MaxHistogram} internally shims the clock actually used
+ * by the underlying reservoir, the reservoir cannot be constructed directly. This function
+ * supports constructing a reservoir based on the shimmed clock
+ * @return a new {@link MaxHistogram} instance
+ */
+ public static MaxHistogram newInstance(
+ int intervalMillis,
+ int maxBackdateSeconds,
+ Clock clock,
+ Function<Clock, Reservoir> reservoirFunction) {
+ RelayClock relayClock = new RelayClock(clock.getTick());
+ return new MaxHistogram(
+ intervalMillis, maxBackdateSeconds, reservoirFunction.apply(relayClock), relayClock, clock);
+ }
+
+ private MaxHistogram(
+ int intervalMillis,
+ int maxBackdateSeconds,
+ Reservoir reservoir,
+ Clock relayClock,
+ Clock clock) {
+ super(reservoir);
+ this.intervalNanos = TimeUnit.MILLISECONDS.toNanos(intervalMillis);
+ if (maxBackdateSeconds == -1) {
+ this.maxBackdateNanos = Long.MAX_VALUE; // effectively no limit
+ } else if (TimeUnit.SECONDS.toMillis(maxBackdateSeconds) >= intervalMillis) {
+ // align `maxBackdateNanos` to a multiple of `intervalNanos`
+ long requestedMaxBackdate = TimeUnit.SECONDS.toNanos(maxBackdateSeconds);
+ this.maxBackdateNanos = requestedMaxBackdate - (requestedMaxBackdate % intervalNanos);
+ } else {
+ throw new IllegalArgumentException(
+ "maxBackdateSeconds must be -1 (unlimited) or >= "
+ + TimeUnit.MILLISECONDS.toSeconds(intervalMillis)
+ + "; found "
+ + maxBackdateSeconds);
+ }
+ this.relayClock = (RelayClock) relayClock;
Review Comment:
💯 -- this was leftover from some refactoring and had to do with class scope/visibility ... no longer an issue, will fix.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17378: Add a RequestHandler metric for "number of outstanding concurrent requests" [solr]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #2591: SOLR-17378: Add a RequestHandler metric for "number of outstanding concurrent requests"
URL: https://github.com/apache/solr/pull/2591
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17378: Add a RequestHandler metric for "number of outstanding concurrent requests" [solr]
Posted by "magibney (via GitHub)" <gi...@apache.org>.
magibney commented on code in PR #2591:
URL: https://github.com/apache/solr/pull/2591#discussion_r1691506265
##########
solr/core/src/java/org/apache/solr/metrics/MaxHistogram.java:
##########
@@ -0,0 +1,191 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.metrics;
+
+import com.codahale.metrics.Clock;
+import com.codahale.metrics.Histogram;
+import com.codahale.metrics.Reservoir;
+import com.codahale.metrics.Snapshot;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicLong;
+import java.util.function.Function;
+
+/**
+ * A special Histogram that conceptually updates at regular intervals, according to the max value of
+ * a counter over the preceding interval. This is appropriate for lazily tracking ambient values
+ * (like "number of outstanding active requests"), where we are interested in:
+ *
+ * <ul>
+ * <li><i>max</i> values over a unit-granularity time-range (as opposed to arbitrary values that
+ * could be retrieved by sampling)
+ * <li>aggregate values not skewed by activity (i.e., the number of events triggering value
+ * recording should not matter -- thus we want to record values at a constant rate)
+ * <li>not dedicating a thread to actually recording values at regular wall-clock intervals
+ * </ul>
+ *
+ * <p>The {@link #update(long)} methods should be invoked with positive and negative args, analogous
+ * to {@link com.codahale.metrics.Counter#inc(long)} and {@link
+ * com.codahale.metrics.Counter#dec(long)}, respectively. The effect is the same as a histogram
+ * regularly sampling a counter's max value over a given interval; but in implementation, samples
+ * are lazily backfilled upon write (value change {@link #update(long)}) or read ({@link
+ * #getSnapshot()}) operations.
+ */
+public class MaxHistogram extends Histogram {
+
+ private final long intervalNanos;
+
+ private final AtomicLong val = new AtomicLong();
+
+ private final AtomicLong lastUpdate;
+
+ private volatile long lastValue = 0;
+
+ private final AtomicLong maxSinceLastUpdate = new AtomicLong(0);
+
+ private final long maxBackdateNanos;
+
+ private final Clock clock;
+
+ private final RelayClock relayClock;
+
+ private static class RelayClock extends Clock {
Review Comment:
It's a shim that allows backdating/backfilling updates by passing in explicitly set `tick` values. I've updated javadocs in 05ecf30b4f4cd88df80f0f5230da1f38939d786d.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17378: Add a RequestHandler metric for "number of outstanding concurrent requests" [solr]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #2591:
URL: https://github.com/apache/solr/pull/2591#issuecomment-2613624553
This PR is now closed due to 60 days of inactivity after being marked as stale. Re-opening this PR is still possible, in which case it will be marked as active again.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17378: Add a RequestHandler metric for "number of outstanding concurrent requests" [solr]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #2591:
URL: https://github.com/apache/solr/pull/2591#issuecomment-2496424718
This PR has had no activity for 60 days and is now labeled as stale. Any new activity will remove the stale label. To attract more reviewers, please tag people who might be familiar with the code area and/or notify the dev@solr.apache.org mailing list. To exempt this PR from being marked as stale, make it a draft PR or add the label "exempt-stale". If left unattended, this PR will be closed after another 60 days of inactivity. Thank you for your contribution!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17378: Add a RequestHandler metric for "number of outstanding concurrent requests" [solr]
Posted by "magibney (via GitHub)" <gi...@apache.org>.
magibney commented on code in PR #2591:
URL: https://github.com/apache/solr/pull/2591#discussion_r1691465927
##########
solr/core/src/java/org/apache/solr/metrics/MaxHistogram.java:
##########
@@ -0,0 +1,191 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.metrics;
+
+import com.codahale.metrics.Clock;
+import com.codahale.metrics.Histogram;
+import com.codahale.metrics.Reservoir;
+import com.codahale.metrics.Snapshot;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicLong;
+import java.util.function.Function;
+
+/**
+ * A special Histogram that conceptually updates at regular intervals, according to the max value of
+ * a counter over the preceding interval. This is appropriate for lazily tracking ambient values
+ * (like "number of outstanding active requests"), where we are interested in:
+ *
+ * <ul>
+ * <li><i>max</i> values over a unit-granularity time-range (as opposed to arbitrary values that
+ * could be retrieved by sampling)
+ * <li>aggregate values not skewed by activity (i.e., the number of events triggering value
+ * recording should not matter -- thus we want to record values at a constant rate)
+ * <li>not dedicating a thread to actually recording values at regular wall-clock intervals
+ * </ul>
+ *
+ * <p>The {@link #update(long)} methods should be invoked with positive and negative args, analogous
+ * to {@link com.codahale.metrics.Counter#inc(long)} and {@link
+ * com.codahale.metrics.Counter#dec(long)}, respectively. The effect is the same as a histogram
+ * regularly sampling a counter's max value over a given interval; but in implementation, samples
+ * are lazily backfilled upon write (value change {@link #update(long)}) or read ({@link
+ * #getSnapshot()}) operations.
+ */
+public class MaxHistogram extends Histogram {
+
+ private final long intervalNanos;
+
+ private final AtomicLong val = new AtomicLong();
+
+ private final AtomicLong lastUpdate;
+
+ private volatile long lastValue = 0;
+
+ private final AtomicLong maxSinceLastUpdate = new AtomicLong(0);
+
+ private final long maxBackdateNanos;
+
+ private final Clock clock;
+
+ private final RelayClock relayClock;
+
+ private static class RelayClock extends Clock {
+
+ private long nextTick;
+
+ private RelayClock(long initialTick) {
+ nextTick = initialTick;
+ }
+
+ @Override
+ public long getTick() {
+ return nextTick;
+ }
+
+ public void setTick(long value) {
+ nextTick = value;
+ }
+ }
+
+ /**
+ * Constructs a new {@link MaxHistogram} instance.
+ *
+ * @param intervalMillis this is the unit interval at which values will be recorded into the
+ * underlying histogram (the max value for the immediately preceding interval). This
+ * granularity is somewhat arbitrary, and the default of 1000ms (1s) should be sufficient for
+ * most applications.
+ * @param maxBackdateSeconds limits how far back value updates will be lazily backfilled upon
+ * value change or read access. Without this, the first value change or read access after a
+ * long period of inactivity could result in backfilling an arbitrarily large number of
+ * intervals. This should ordinarily be set to align with the time window of the reservoir (or
+ * 300s/5m for the default configuration of {@link
+ * com.codahale.metrics.ExponentiallyDecayingReservoir}).
+ * @param clock the backing clock to use for the reservoir
+ * @param reservoirFunction because {@link MaxHistogram} internally shims the clock actually used
+ * by the underlying reservoir, the reservoir cannot be constructed directly. This function
+ * supports constructing a reservoir based on the shimmed clock
+ * @return a new {@link MaxHistogram} instance
+ */
+ public static MaxHistogram newInstance(
+ int intervalMillis,
+ int maxBackdateSeconds,
+ Clock clock,
+ Function<Clock, Reservoir> reservoirFunction) {
+ RelayClock relayClock = new RelayClock(clock.getTick());
+ return new MaxHistogram(
+ intervalMillis, maxBackdateSeconds, reservoirFunction.apply(relayClock), relayClock, clock);
+ }
+
+ private MaxHistogram(
+ int intervalMillis,
+ int maxBackdateSeconds,
+ Reservoir reservoir,
+ Clock relayClock,
+ Clock clock) {
+ super(reservoir);
+ this.intervalNanos = TimeUnit.MILLISECONDS.toNanos(intervalMillis);
+ if (maxBackdateSeconds == -1) {
+ this.maxBackdateNanos = Long.MAX_VALUE; // effectively no limit
+ } else if (TimeUnit.SECONDS.toMillis(maxBackdateSeconds) >= intervalMillis) {
+ // align `maxBackdateNanos` to a multiple of `intervalNanos`
+ long requestedMaxBackdate = TimeUnit.SECONDS.toNanos(maxBackdateSeconds);
+ this.maxBackdateNanos = requestedMaxBackdate - (requestedMaxBackdate % intervalNanos);
+ } else {
+ throw new IllegalArgumentException(
+ "maxBackdateSeconds must be -1 (unlimited) or >= "
+ + TimeUnit.MILLISECONDS.toSeconds(intervalMillis)
+ + "; found "
+ + maxBackdateSeconds);
+ }
+ this.relayClock = (RelayClock) relayClock;
Review Comment:
💯 -- this was leftover from some refactoring and had to do with class scope/visibility ... no longer an issue, fixed in 05ecf30b4f4cd88df80f0f5230da1f38939d786d.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17378: Add a RequestHandler metric for "number of outstanding concurrent requests" [solr]
Posted by "magibney (via GitHub)" <gi...@apache.org>.
magibney commented on code in PR #2591:
URL: https://github.com/apache/solr/pull/2591#discussion_r1691502929
##########
solr/core/src/java/org/apache/solr/metrics/MetricSuppliers.java:
##########
@@ -154,17 +155,25 @@ public static Clock getClock(PluginInfo info, String param) {
private static final String EDR_CLAZZ = ExponentiallyDecayingReservoir.class.getName();
private static final String UNI_CLAZZ = UniformReservoir.class.getName();
private static final String STW_CLAZZ = SlidingTimeWindowReservoir.class.getName();
+ private static final String STWA_CLAZZ = SlidingTimeWindowArrayReservoir.class.getName();
Review Comment:
I noticed there's no support for the new `SlidingTimeWindowArrayReservoir` class, so I added it here. See [dropwizard docs](https://metrics.dropwizard.io/4.2.0/manual/core.html#sliding-time-window-reservoirs):
> Hint: Try to use our new optimised version of `SlidingTimeWindowReservoir` called `SlidingTimeWindowArrayReservoir`. It brings much lower memory overhead. Also it’s allocation/free patterns are different, so GC overhead is 60x-80x lower then `SlidingTimeWindowReservoir`. Now `SlidingTimeWindowArrayReservoir` is comparable with `ExponentiallyDecayingReservoir` in terms GC overhead and performance. As for required memory, `SlidingTimeWindowArrayReservoir` takes ~128 bits per stored measurement and you can simply calculate required amount of heap.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17378: Add a RequestHandler metric for "number of outstanding concurrent requests" [solr]
Posted by "magibney (via GitHub)" <gi...@apache.org>.
magibney commented on code in PR #2591:
URL: https://github.com/apache/solr/pull/2591#discussion_r1691461924
##########
solr/core/src/java/org/apache/solr/handler/RequestHandlerBase.java:
##########
@@ -246,20 +250,24 @@ public void handleRequest(SolrQueryRequest req, SolrQueryResponse rsp) {
processErrorMetricsOnException(normalized, metrics);
rsp.setException(normalized);
} finally {
- long elapsed = timer.stop();
- metrics.totalTime.inc(elapsed);
-
- if (publishCpuTime && threadCpuTimer != null) {
- Optional<Long> cpuTime = threadCpuTimer.getElapsedCpuMs();
- if (cpuTime.isPresent()) {
- // add CPU_TIME if not already added by SearchHandler
- NamedList<Object> header = rsp.getResponseHeader();
- if (header != null) {
- if (header.get(ThreadCpuTimer.CPU_TIME) == null) {
- header.add(ThreadCpuTimer.CPU_TIME, cpuTime.get());
+ try {
+ metrics.concurrency.update(-1);
Review Comment:
> try-finally paranoia
heh yeah, I guess that's all it is really. Thanks for calling it out, fixed in 8d4b0779fc366f06ed96a0f26420458db9126154
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17378: Add a RequestHandler metric for "number of outstanding concurrent requests" [solr]
Posted by "magibney (via GitHub)" <gi...@apache.org>.
magibney commented on PR #2591:
URL: https://github.com/apache/solr/pull/2591#issuecomment-2248437989
The core code should be ready for review, but the test coverage is not at all adequate yet.
There is a test stubbed out, but it's sloppy/too long, and basically exists only to exercise the code. Still considering what cases we need to test and how best to test, etc.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17378: Add a RequestHandler metric for "number of outstanding concurrent requests" [solr]
Posted by "magibney (via GitHub)" <gi...@apache.org>.
magibney commented on code in PR #2591:
URL: https://github.com/apache/solr/pull/2591#discussion_r1691464517
##########
solr/core/src/java/org/apache/solr/metrics/MaxHistogram.java:
##########
@@ -0,0 +1,191 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.metrics;
+
+import com.codahale.metrics.Clock;
+import com.codahale.metrics.Histogram;
+import com.codahale.metrics.Reservoir;
+import com.codahale.metrics.Snapshot;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicLong;
+import java.util.function.Function;
+
+/**
+ * A special Histogram that conceptually updates at regular intervals, according to the max value of
+ * a counter over the preceding interval. This is appropriate for lazily tracking ambient values
+ * (like "number of outstanding active requests"), where we are interested in:
+ *
+ * <ul>
+ * <li><i>max</i> values over a unit-granularity time-range (as opposed to arbitrary values that
+ * could be retrieved by sampling)
+ * <li>aggregate values not skewed by activity (i.e., the number of events triggering value
+ * recording should not matter -- thus we want to record values at a constant rate)
+ * <li>not dedicating a thread to actually recording values at regular wall-clock intervals
+ * </ul>
+ *
+ * <p>The {@link #update(long)} methods should be invoked with positive and negative args, analogous
+ * to {@link com.codahale.metrics.Counter#inc(long)} and {@link
+ * com.codahale.metrics.Counter#dec(long)}, respectively. The effect is the same as a histogram
+ * regularly sampling a counter's max value over a given interval; but in implementation, samples
+ * are lazily backfilled upon write (value change {@link #update(long)}) or read ({@link
+ * #getSnapshot()}) operations.
+ */
+public class MaxHistogram extends Histogram {
+
+ private final long intervalNanos;
+
+ private final AtomicLong val = new AtomicLong();
Review Comment:
Yeah since we want to track the max value over the granularity interval, to get transient peaks, AtomicInteger is necessary.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17378: Add a RequestHandler metric for "number of outstanding concurrent requests" [solr]
Posted by "magibney (via GitHub)" <gi...@apache.org>.
magibney commented on code in PR #2591:
URL: https://github.com/apache/solr/pull/2591#discussion_r1691461924
##########
solr/core/src/java/org/apache/solr/handler/RequestHandlerBase.java:
##########
@@ -246,20 +250,24 @@ public void handleRequest(SolrQueryRequest req, SolrQueryResponse rsp) {
processErrorMetricsOnException(normalized, metrics);
rsp.setException(normalized);
} finally {
- long elapsed = timer.stop();
- metrics.totalTime.inc(elapsed);
-
- if (publishCpuTime && threadCpuTimer != null) {
- Optional<Long> cpuTime = threadCpuTimer.getElapsedCpuMs();
- if (cpuTime.isPresent()) {
- // add CPU_TIME if not already added by SearchHandler
- NamedList<Object> header = rsp.getResponseHeader();
- if (header != null) {
- if (header.get(ThreadCpuTimer.CPU_TIME) == null) {
- header.add(ThreadCpuTimer.CPU_TIME, cpuTime.get());
+ try {
+ metrics.concurrency.update(-1);
Review Comment:
> try-finally paranoia
heh yeah, I guess that's all it is really. Thanks for calling it out.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17378: Add a RequestHandler metric for "number of outstanding concurrent requests" [solr]
Posted by "magibney (via GitHub)" <gi...@apache.org>.
magibney commented on PR #2591:
URL: https://github.com/apache/solr/pull/2591#issuecomment-2371725359
> Have you deployed it / ran it for a while to see if it behaves sensibly?
Quick status update -- have _not_ yet deployed it locally. This is still definitely on the radar though, so I hope to report back and move this forward soon.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17378: Add a RequestHandler metric for "number of outstanding concurrent requests" [solr]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #2591:
URL: https://github.com/apache/solr/pull/2591#issuecomment-2369805690
This PR has had no activity for 60 days and is now labeled as stale. Any new activity or converting it to draft will remove the stale label. To attract more reviewers, please tag people who might be familiar with the code area and/or notify the dev@solr.apache.org mailing list. Thank you for your contribution!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org