dongjoon-hyun commented on code in PR #949:
URL:
https://github.com/apache/spark-kubernetes-operator/pull/949#discussion_r4268226771
##########
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:
This holds only when
`spark.kubernetes.operator.metrics.sanitizePrometheusMetricsNameEnabled` is
`true` (the default). `nanosMetricsNameToSeconds` replaces only `_nanos`, so
when sanitizing is disabled, the name stays
`operator.sdk.sparkapplication.sparkappreconciler.reconcile.both.nanos` while
its values are converted to seconds, e.g. `0.01`. Could you either handle that
case or mention it here?
##########
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:
This relies on `PrometheusPullModelHandler.formatHistogram`, which treats a
histogram as nanos with `baseName.contains("nanos")` and renames it with
`replaceAll("_nanos", "_seconds")`. Both match anywhere in the name, including
the namespace segment of the namespaced histograms.
For example, namespace `team-nanos` gives
`operator_sdk_sparkapplication_team_nanos_sparkappreconciler_reconcile_both_nanos`,
which is exported as
`operator_sdk_sparkapplication_team_seconds_sparkappreconciler_reconcile_both_seconds`.
Also, a namespace like `nanoservices` contains `nanos`, so any non-nanos
histogram there would be divided by 1e9.
Could you match only the trailing `_nanos` suffix, e.g. with `endsWith`?
##########
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:
Instead of a histogram whose unit comes from a `nanos` name suffix, how
about the existing `BaseOperatorSource.getTimer` (`SummingTimer`)?
```java
getTimer(metricsPrefix, name, execName, successType).update(durationNanos,
TimeUnit.NANOSECONDS);
```
`formatTimer` already exports a timer in seconds as `..._duration_seconds`,
regardless of the name, so it doesn't depend on the name heuristic (see the
other comments). It also still works with `ManualClock`, since the duration is
measured outside the timer.
##########
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:
This registers a hardcoded `...reconcile.both.nanos` histogram in a fresh
registry, so it mostly repeats
`testFormatMetricsSnapshotIncludesHistogramWithNanos` and still passes if
`OperatorJosdkMetrics` stops appending `NANOS`. Since the `Clock` constructor
is package-private in `metrics.source`, could you move this check to
`OperatorJosdkMetricsTest`, i.e. record an execution through `new
OperatorJosdkMetrics(clock)` and format its registry with
`PrometheusPullModelHandler`, to cover the real wiring?
##########
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:
The new `failure.nanos` assertions are inside this `catch` with no `fail()`
after `timeControllerExecution`, so they are skipped silently if no exception
propagates, e.g. after a regression that swallows the exception. Could you use
`assertThrows(NoSuchFieldException.class, () ->
operatorMetrics.timeControllerExecution(failedExecution))` and then assert?
--
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]
