[GitHub] [kafka] ableegoldman commented on a change in pull request #8882: KAFKA-10165: Remove Percentiles from e2e metrics

[GitBox <gi...@apache.org>]

ableegoldman commented on a change in pull request #8882:
URL: https://github.com/apache/kafka/pull/8882#discussion_r441161523



##########
File path: streams/src/main/java/org/apache/kafka/streams/processor/internals/metrics/StreamsMetricsImpl.java
##########
@@ -675,27 +670,6 @@ public static void addMinAndMaxAndP99AndP90ToSensor(final Sensor sensor,
                 tags),
             new Max()
         );
-
-        sensor.add(

Review comment:
       Github won't let me comment on these lines, but we should remove the two percentiles-necessitated constants above (`PERCENTILES_SIZE_IN_BYTES` and `MAXIMUM_E2E_LATENCY`)

##########
File path: streams/src/main/java/org/apache/kafka/streams/processor/internals/StandbyTask.java
##########
@@ -160,18 +162,21 @@ public void postCommit() {
 
     @Override
     public void closeClean() {
+        streamsMetrics.removeAllTaskLevelSensors(Thread.currentThread().getName(), id.toString());

Review comment:
       Sounds good. But why do it both here and in `closeDirty` vs doing so in `close(clean)`?

##########
File path: streams/src/main/java/org/apache/kafka/streams/processor/internals/ProcessorContextImpl.java
##########
@@ -235,7 +235,7 @@ public StateStore getStateStore(final String name) {
         setCurrentNode(child);
         child.process(key, value);
         if (child.isTerminalNode()) {
-            streamTask.maybeRecordE2ELatency(timestamp(), child.name());
+            streamTask.maybeRecordE2ELatency(timestamp(), currentSystemTimeMs(), child.name());

Review comment:
       Oh, hm, I thought we decided to push the stateful-node-level metrics to TRACE so we could get the actual time at each node without a (potential) performance hit. But with the INFO-level metrics it would be ok since we're only updating it twice per process.
   But maybe I'm misremembering...I suppose ideally we could run some benchmarks for both cases and see if it really makes a difference...

##########
File path: streams/src/main/java/org/apache/kafka/streams/processor/internals/StreamTask.java
##########
@@ -456,12 +457,14 @@ public void postCommit() {
 
     @Override
     public void closeClean() {
+        streamsMetrics.removeAllTaskLevelSensors(Thread.currentThread().getName(), id.toString());

Review comment:
       Agreed, we should clean up anything we created in the same class




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org