sankalpsthakur commented on code in PR #949:
URL:
https://github.com/apache/spark-kubernetes-operator/pull/949#discussion_r4439350070
##########
docs/migration_guide.md:
##########
@@ -125,9 +125,23 @@ affect building the project, CI, tests or examples.
`kubernetes_client_http_response_latency_seconds_sum`, is the sum of all
recorded values. 1.0
exported the mean of roughly the last 5 minutes multiplied by the count
instead, which can
decrease between scrapes, so `rate()` of it and the average derived from it
were wrong. Also, the
- `0.5` and `0.999` quantiles of the `operator_sdk_*` histograms are the
median and the 99.9th
+ `0.5` and `0.999` quantiles of the operator histograms and timers 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 `operator.sdk` metrics source times the controller
executions, i.e. the
+ reconciliations and cleanups, with timers instead of histograms, so the
source has no histograms
+ anymore. The timers record nanoseconds. 1.0 recorded whole seconds, so an
execution under one
+ second, which most are, was recorded as `0`, and the quantiles and `_sum` of
these histograms
+ were mostly `0`. With the default text-based Prometheus format, the timers
are exported in
+ seconds with a `_duration_seconds` suffix regardless of
+ `spark.kubernetes.operator.metrics.sanitizePrometheusMetricsNameEnabled`,
e.g.
+
`operator_sdk_sparkapplication_sparkappreconciler_reconcile_both_duration_seconds`
instead of
+ `operator_sdk_sparkapplication_sparkappreconciler_reconcile_both`, plus
`_m1_rate`, `_m5_rate`
+ and `_m15_rate` gauges per timer. With
+ `spark.kubernetes.operator.metrics.prometheusTextBasedFormatEnabled=false`,
Spark's
+ `PrometheusServlet` exports them as `{type="timers"}` in raw nanoseconds,
and other Spark sinks,
+ e.g. JMX, Console or CSV, apply their own duration unit. Update queries and
dashboards
Review Comment:
since these stay histograms, JMX keeps `type=histograms` and the other sinks
keep their fields, only the values are nanoseconds instead of whole seconds.
the item says that now for `PrometheusServlet` and the other sinks
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/metrics/source/OperatorJosdkMetrics.java:
##########
@@ -134,7 +144,9 @@ public void eventReceived(Event event, Map<String, Object>
metadata) {
}
/**
- * Times the execution of a controller operation and records success or
failure metrics.
+ * Times the execution of a controller operation and records success or
failure metrics. The
+ * duration is measured with the monotonic clock in nanoseconds and recorded
in timers, which
+ * {@link org.apache.spark.k8s.operator.metrics.PrometheusPullModelHandler}
exports in seconds.
Review Comment:
added "with the default text-based format", also to the javadoc of the new
`NanosHistogram`
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/metrics/source/OperatorJosdkMetrics.java:
##########
@@ -192,6 +191,32 @@ public <T> T
timeControllerExecution(ControllerExecution<T> execution) throws Ex
}
throw e;
}
+ final long durationNanos = clock.nanoTime() - startTime;
+ final String successType = execution.successTypeName(result);
+ if (resourceClass.isPresent()) {
+ String metricsPrefix = getMetricNamePrefix(resourceClass.get());
+ recordDuration(metricsPrefix, namespaceOptional, durationNanos, name,
execName, successType);
+ getCounter(metricsPrefix, name, execName, SUCCESS, successType).inc();
+ if (namespaceOptional.isPresent()) {
+ getCounter(metricsPrefix, namespaceOptional.get(), name, execName,
SUCCESS, successType)
+ .inc();
+ }
+ }
+ return result;
+ }
+
+ private void recordDuration(
+ String metricsPrefix,
+ Optional<String> namespaceOptional,
+ long durationNanos,
+ String name,
+ String execName,
+ String type) {
+ getTimer(metricsPrefix, name, execName, type).update(durationNanos,
TimeUnit.NANOSECONDS);
Review Comment:
agreed, switched back to histograms in 51082d1. `NanosHistogram` is a
`SummingHistogram` which `formatHistogram` exports in seconds by its type,
without renaming it, so it doesn't depend on a `nanos` name and a namespace
like `team-nanos` stays as it is (the export test covers it again). it has no
`Meter`, so each metric keeps the 8 series of 1.0, or 11 with
`prometheusTextBasedFormatEnabled=false`
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/metrics/source/OperatorJosdkMetrics.java:
##########
@@ -192,6 +191,32 @@ public <T> T
timeControllerExecution(ControllerExecution<T> execution) throws Ex
}
throw e;
}
+ final long durationNanos = clock.nanoTime() - startTime;
+ final String successType = execution.successTypeName(result);
+ if (resourceClass.isPresent()) {
+ String metricsPrefix = getMetricNamePrefix(resourceClass.get());
+ recordDuration(metricsPrefix, namespaceOptional, durationNanos, name,
execName, successType);
+ getCounter(metricsPrefix, name, execName, SUCCESS, successType).inc();
+ if (namespaceOptional.isPresent()) {
+ getCounter(metricsPrefix, namespaceOptional.get(), name, execName,
SUCCESS, successType)
+ .inc();
+ }
+ }
+ return result;
+ }
+
+ private void recordDuration(
+ String metricsPrefix,
+ Optional<String> namespaceOptional,
+ long durationNanos,
+ String name,
+ String execName,
+ String type) {
+ getTimer(metricsPrefix, name, execName, type).update(durationNanos,
TimeUnit.NANOSECONDS);
+ if (namespaceOptional.isPresent()) {
+ getTimer(metricsPrefix, namespaceOptional.get(), name, execName, type)
+ .update(durationNanos, TimeUnit.NANOSECONDS);
Review Comment:
since these histograms go through `formatHistogram` now, it takes the
snapshot once in 51082d1. i left `formatTimer` as is to keep this to the
ticket, happy to change it too if you like
--
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]
