sankalpsthakur commented on code in PR #949:
URL:
https://github.com/apache/spark-kubernetes-operator/pull/949#discussion_r4268996744
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/metrics/source/OperatorJosdkMetrics.java:
##########
@@ -59,6 +58,7 @@ public class OperatorJosdkMetrics extends BaseOperatorSource
implements Source,
private static final String SUCCESS = "success";
private static final String FAILURE = "failure";
private static final String EXCEPTION = "exception";
+ private static final String NANOS = "nanos";
Review Comment:
switched to SummingTimer in 0de793f, no more nanos name matching so a
namespace like team-nanos is untouched (covered by a new test)
##########
docs/migration_guide.md:
##########
@@ -121,6 +121,15 @@ affect building the project, CI, tests or examples.
`0.5` and `0.999` quantiles of the `operator_sdk_*` histograms are the
median and the 99.9th
percentile instead of the mean and the 99th percentile. A histogram or timer
of a custom metrics
source has no `_sum` unless it is a `SummingHistogram` or a `SummingTimer`.
+- Since 1.1.0, the histograms of the `operator.sdk` metrics source which time
the controller
+ executions, i.e. the reconciliations and cleanups, record nanoseconds, and
their names end with
+ `nanos`, e.g.
`operator.sdk.sparkapplication.sparkappreconciler.reconcile.both.nanos` instead
of
+ `operator.sdk.sparkapplication.sparkappreconciler.reconcile.both`, so
`PrometheusPullModelHandler`
+ exports them in seconds with a `_seconds` suffix, e.g.
Review Comment:
with timers the export is _duration_seconds regardless of the sanitize flag,
updated the guide
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/metrics/source/OperatorJosdkMetrics.java:
##########
@@ -152,33 +164,35 @@ public <T> T
timeControllerExecution(ControllerExecution<T> execution) throws Ex
getResourceClass(metadata);
final String execName = execution.name();
- long startTime = clock.getTimeMillis();
+ long startTime = clock.nanoTime();
try {
T result = execution.execute();
+ final long durationNanos = clock.nanoTime() - startTime;
final String successType = execution.successTypeName(result);
if (resourceClass.isPresent()) {
String metricsPrefix = getMetricNamePrefix(resourceClass.get());
- getHistogram(metricsPrefix, name, execName,
successType).update(toSeconds(startTime));
+ getHistogram(metricsPrefix, name, execName, successType,
NANOS).update(durationNanos);
Review Comment:
done, getTimer(...).update(durationNanos, TimeUnit.NANOSECONDS) for success
and failure, cluster and namespaced
##########
spark-operator/src/test/java/org/apache/spark/k8s/operator/metrics/source/OperatorJosdkMetricsTest.java:
##########
@@ -81,12 +90,18 @@ void testTimeControllerExecution() throws Exception {
} catch (Exception e) {
Review Comment:
now uses assertThrows(NoSuchFieldException.class, ...) then asserts
##########
spark-operator/src/test/java/org/apache/spark/k8s/operator/metrics/PrometheusPullModelHandlerTest.java:
##########
@@ -96,6 +96,26 @@ void testFormatMetricsSnapshotIncludesHistogramWithNanos()
throws Exception {
assertTrue(output.contains("foo_seconds_histogram_sum 0.001572032"));
}
+ @Test
+ void
testFormatMetricsSnapshotIncludesControllerExecutionHistogramInSeconds() {
Review Comment:
moved to OperatorJosdkMetricsTest, records through new
OperatorJosdkMetrics(clock) and formats its registry, dropped the duplicate case
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
