[multistage][observability] Add an option to log operator stats to log.info#10179
[multistage][observability] Add an option to log operator stats to log.info#1017961yao wants to merge 6 commits intoapache:masterfrom
Conversation
There was a problem hiding this comment.
error seems like a weird log level for this - why not make it INFO and then just have people configure log4j properly for it?
There was a problem hiding this comment.
+1 lets put this in info. we can remove this once join is GA
There was a problem hiding this comment.
Because it is easier to see in IDE..... the whole stats is quite hacky anyway. So I put loggeer.error.
We probably need to rewrite the logging later.
61yao
force-pushed
the
operator_logging
branch
from
fd1c306 to
ffc64ba
Compare
Codecov Report
@@ Coverage Diff @@
## master #10179 +/- ##
============================================
- Coverage 70.45% 68.51% -1.94%
+ Complexity 5796 5135 -661
============================================
Files 2020 2020
Lines 109107 109124 +17
Branches 16555 16562 +7
============================================
- Hits 76874 74769 -2105
- Misses 26853 29052 +2199
+ Partials 5380 5303 -77
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
61yao
changed the title
| public String toExplainString() { | ||
| _dataTableBlockBaseOperator.toExplainString(); | ||
| LOGGER.debug(_operatorStats.toString()); | ||
| LOGGER.info(_operatorStats.toString()); |
There was a problem hiding this comment.
can we move these to MultiStageOperator looks like they are all identical on every concrete impl
There was a problem hiding this comment.
we can also move _operatorStats.startTimer(); and _operatorStats.endTimer() to MultiStageOperator and overwrite the nextBlock() method.
There was a problem hiding this comment.
Can move in next PR since it is urgent.
| if (operatorChain.shouldLogOpStats()) { | ||
| operatorChain.getRoot().toExplainString(); | ||
| } |
There was a problem hiding this comment.
move these to operatorChain.close()?
| public String toExplainString() { | ||
| _upstreamOperator.toExplainString(); | ||
| LOGGER.debug(_operatorStats.toString()); | ||
| LOGGER.info(_operatorStats.toString()); |
There was a problem hiding this 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
- all the timer operations can be done/wrapped inside
MultiStageOperator.nextBlock()method. - avoid the unnecessary start/stop on the stopwatch (although minuscule, it is still perform on a per-block basis)
- makes the code cleaner
good example:
There was a problem hiding this comment.
This is more complicated implementation. Since we need the time now, I'll check in this PR and have followups. Thanks
5 participants
The option is set using logMultistageOperatorStats.
It is tested in MulitstageEngineIntegrationTest.