sankalpsthakur commented on code in PR #949:
URL:
https://github.com/apache/spark-kubernetes-operator/pull/949#discussion_r4439351309
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/metrics/source/OperatorJosdkMetrics.java:
##########
@@ -316,10 +341,6 @@ public void reconciliationSucceeded(HasMetadata resource,
Map<String, Object> me
.dec();
}
- private long toSeconds(long startTimeInMilliseconds) {
Review Comment:
the histograms map has a caller again through `getNanosHistogram`, which
shares the creation code with `getHistogram`. i kept `getHistogram` since it's
protected API, with a direct test in the new `BaseOperatorSourceTest` that it
creates a `SummingHistogram`
##########
spark-operator/src/test/java/org/apache/spark/k8s/operator/metrics/source/OperatorJosdkMetricsTest.java:
##########
@@ -55,11 +61,13 @@ class OperatorJosdkMetricsTest {
"test-controller-name");
private static final String controllerName = "test-controller";
+ private ManualClock clock;
private OperatorJosdkMetrics operatorMetrics;
@BeforeEach
void setup() {
- operatorMetrics = new OperatorJosdkMetrics();
+ clock = new ManualClock();
Review Comment:
the clock starts at 1000 ms now
##########
spark-operator/src/test/java/org/apache/spark/k8s/operator/metrics/source/OperatorJosdkMetricsTest.java:
##########
@@ -68,29 +76,57 @@ void testTimeControllerExecution() throws Exception {
operatorMetrics.timeControllerExecution(successExecution);
Map<String, Metric> metrics =
operatorMetrics.metricRegistry().getMetrics();
assertEquals(4, metrics.size());
- assertInstanceOf(
- SummingHistogram.class,
metrics.get("sparkapplication.test-controller.reconcile.both"));
-
assertTrue(metrics.containsKey("sparkapplication.testns.test-controller.reconcile.both"));
+ // The execution took 10 ms, which is recorded with sub-second precision
in nanoseconds.
+ long durationNanos = Duration.ofMillis(10).toNanos();
+ assertEquals(
+ durationNanos, recordedNanos(metrics,
"sparkapplication.test-controller.reconcile.both"));
+ assertEquals(
+ durationNanos,
+ recordedNanos(metrics,
"sparkapplication.testns.test-controller.reconcile.both"));
assertTrue(metrics.containsKey("sparkapplication.test-controller.reconcile.success.both"));
assertTrue(
metrics.containsKey("sparkapplication.testns.test-controller.reconcile.success.both"));
FooTestingExecutionBase<SparkApplication> failedExecution = new
FooTestingExecutionBase<>();
- try {
- operatorMetrics.timeControllerExecution(failedExecution);
- } catch (Exception e) {
- assertEquals("Foo exception", e.getMessage());
- assertEquals(8, metrics.size());
-
assertTrue(metrics.containsKey("sparkapplication.test-controller.reconcile.failure"));
- assertTrue(
- metrics.containsKey(
- "sparkapplication.test-controller.reconcile.failure.exception"
- + ".nosuchfieldexception"));
-
assertTrue(metrics.containsKey("sparkapplication.testns.test-controller.reconcile.failure"));
- assertTrue(
- metrics.containsKey(
- "sparkapplication.testns.test-controller.reconcile.failure."
- + "exception.nosuchfieldexception"));
+ Exception e =
+ assertThrows(
+ NoSuchFieldException.class,
+ () -> operatorMetrics.timeControllerExecution(failedExecution));
+ assertEquals("Foo exception", e.getMessage());
+ assertEquals(8, metrics.size());
+ long failureNanos = Duration.ofMillis(5).toNanos();
+ assertEquals(
+ failureNanos,
+ recordedNanos(metrics,
"sparkapplication.test-controller.reconcile.failure"));
+ assertTrue(
+ metrics.containsKey(
+ "sparkapplication.test-controller.reconcile.failure.exception"
+ + ".nosuchfieldexception"));
+ assertEquals(
+ failureNanos,
+ recordedNanos(metrics,
"sparkapplication.testns.test-controller.reconcile.failure"));
+ assertTrue(
+ metrics.containsKey(
+ "sparkapplication.testns.test-controller.reconcile.failure."
+ + "exception.nosuchfieldexception"));
+ }
+
+ @Test
+ void testTimeControllerExecutionIsExportedInSeconds() throws Exception {
+ operatorMetrics.timeControllerExecution(new TestingExecutionBase<>());
+ TestingPrometheusPullModelHandler handler =
+ new
TestingPrometheusPullModelHandler(operatorMetrics.metricRegistry());
Review Comment:
the export test now registers the source in a parent registry first, as
`MetricsSystem.registerSource` does, records afterwards, so the histograms
reach the parent through the registry listener, and asserts the `operator_sdk_`
names
--
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]
