[GitHub] [pinot] walterddr commented on a diff in pull request #10179: [multistage][observability] Add an option to log operator stats to log.info

["walterddr (via GitHub)" <gi...@apache.org>]

walterddr commented on code in PR #10179:
URL: https://github.com/apache/pinot/pull/10179#discussion_r1088433667


##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/MailboxSendOperator.java:
##########
@@ -120,7 +120,7 @@ public List<MultiStageOperator> getChildOperators() {
   @Override
   public String toExplainString() {
     _dataTableBlockBaseOperator.toExplainString();
-    LOGGER.debug(_operatorStats.toString());
+    LOGGER.info(_operatorStats.toString());

Review Comment:
   can we move these to `MultiStageOperator` looks like they are all identical on every concrete impl



##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/executor/OpChainSchedulerService.java:
##########
@@ -114,18 +114,23 @@ public void runJob() {
                   register(operatorChain, false);
                 } else {
                   if (result.isErrorBlock()) {
-                    operatorChain.getRoot().toExplainString();
                     LOGGER.error("({}): Completed erroneously {} {}", operatorChain, operatorChain.getStats(),
                         result.getDataBlock().getExceptions());
                   } else {
-                    operatorChain.getRoot().toExplainString();
-                    LOGGER.debug("({}): Completed {}", operatorChain, operatorChain.getStats());
+                    if (operatorChain.shouldLogOpStats()) {
+                      LOGGER.info("({}): Completed {}", operatorChain, operatorChain.getStats());
+                    }
                   }
                   operatorChain.close();
+                  if (operatorChain.shouldLogOpStats()) {
+                    operatorChain.getRoot().toExplainString();
+                  }

Review Comment:
   move these to `operatorChain.close()`?



##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/MailboxSendOperator.java:
##########
@@ -120,7 +120,7 @@ public List<MultiStageOperator> getChildOperators() {
   @Override
   public String toExplainString() {
     _dataTableBlockBaseOperator.toExplainString();
-    LOGGER.debug(_operatorStats.toString());
+    LOGGER.info(_operatorStats.toString());

Review Comment:
   we can also move `_operatorStats.startTimer();` and `_operatorStats.endTimer()` to `MultiStageOperator` and overwrite the `nextBlock()` method.



##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/FilterOperator.java:
##########
@@ -75,7 +75,7 @@ public List<MultiStageOperator> getChildOperators() {
   @Override
   public String toExplainString() {
     _upstreamOperator.toExplainString();
-    LOGGER.debug(_operatorStats.toString());
+    LOGGER.info(_operatorStats.toString());

Review Comment:
   although not introduced in this PR. but looks like there are a lot of start/stop timer within each operator on how they record the timer. Let's clean them up by recording the entire time e2e and later when logging, subtract current and upstream timer, this way 
   1. all the timer operations can be done/wrapped inside `MultiStageOperator.nextBlock()` method. 
   2. avoid the unnecessary start/stop on the stopwatch (although minuscule, it is still perform on a per-block basis)
   3. makes the code cleaner
   
   good example: https://github.com/apache/pinot/blob/01f3528ffc1d6db548cc38ad483bb7c061076b6a/pinot-core/src/main/java/org/apache/pinot/core/operator/BaseOperator.java#L42-L44
   



-- 
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: commits-unsubscribe@pinot.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org