Skip to content

Comments

[multistage][stats] clean up some stats#10390

Merged
walterddr merged 2 commits intoapache:masterfrom
walterddr:pr_add_walltime
Mar 8, 2023
Merged

[multistage][stats] clean up some stats#10390
walterddr merged 2 commits intoapache:masterfrom
walterddr:pr_add_walltime

Conversation

@walterddr
Copy link
Contributor

@walterddr walterddr commented Mar 7, 2023

ChangeList

  1. enable wall time tracking. currently it only report CPU/Thread total time
  2. enable tracing E2E, fixed issue with identical requestID with different table type / stageID mixing. This causes concurrency issue on TraceContext

More Details

more context and what's solved/unsolved: #10399

TODOs

  1. if not enableTrace, disable all complexity in intermediary Operators, only MailboxSend and MailboxReceive should be dealing with metadata (e.g. no need to record operator level stats then convert them into stage level) --> to speed up metadata processing
  2. fix operator stats processing issue: CPU time is overlapping, we need to unoverlap
  3. fix operator stats starting time measurement: currently it starts at first block. it should start at first non no-op block
  4. enable some compression for operator stats, for example
    • similar to trace info the entire string should be encoded once and pass around afterwards to avoid duplicate ser/de on the map
    • allow agg on either operator level or server level when dealing with large cardinality of (opID + serverID + stageID + requestID)

@codecov-commenter
Copy link

codecov-commenter commented Mar 7, 2023

Codecov Report

Merging #10390 (a277587) into master (52761fc) will increase coverage by 0.06%.
The diff coverage is 92.30%.

@@             Coverage Diff              @@
##             master   #10390      +/-   ##
============================================
+ Coverage     70.23%   70.30%   +0.06%     
- Complexity     6025     6030       +5     
============================================
  Files          2049     2049              
  Lines        110869   110909      +40     
  Branches      16851    16860       +9     
============================================
+ Hits          77873    77974     +101     
+ Misses        27540    27484      -56     
+ Partials       5456     5451       -5     
Flag Coverage Δ
integration1 24.49% <61.53%> (-0.06%) ⬇️
integration2 24.46% <28.84%> (+0.05%) ⬆️
unittests1 67.93% <78.84%> (+0.06%) ⬆️
unittests2 13.84% <0.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...common/response/broker/BrokerResponseNativeV2.java 33.33% <0.00%> (ø)
...not/query/runtime/operator/MultiStageOperator.java 68.18% <ø> (ø)
...org/apache/pinot/core/util/trace/TraceContext.java 96.36% <80.00%> (-1.75%) ⬇️
...ot/core/query/reduce/ExecutionStatsAggregator.java 98.25% <92.00%> (-0.30%) ⬇️
...a/org/apache/pinot/common/datatable/DataTable.java 92.06% <100.00%> (+0.26%) ⬆️
...ot/common/response/broker/BrokerResponseStats.java 96.77% <100.00%> (+1.12%) ⬆️
...va/org/apache/pinot/query/runtime/QueryRunner.java 84.31% <100.00%> (-0.16%) ⬇️
...he/pinot/query/runtime/operator/OperatorStats.java 92.30% <100.00%> (+1.39%) ⬆️
...t/query/runtime/plan/ServerRequestPlanVisitor.java 84.40% <100.00%> (+0.14%) ⬆️
...g/apache/pinot/server/api/resources/ErrorInfo.java 0.00% <0.00%> (-100.00%) ⬇️
... and 42 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@walterddr walterddr marked this pull request as ready for review March 8, 2023 06:43

public void setStageLevelStats(@Nullable String rawTableName, BrokerResponseStats brokerResponseStats,
@Nullable BrokerMetrics brokerMetrics) {
@Nullable BrokerMetrics brokerMetrics, boolean traceEnabled) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to pass this flag imo. It is already part of the constructor.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch!

Copy link
Contributor

@KKcorps KKcorps left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor comment, Everything else looks good.

@walterddr walterddr changed the title [multistage][stats] adding stats for wall time [multistage][stats] clean up some stats Mar 8, 2023
@walterddr walterddr merged commit 3ffa7c3 into apache:master Mar 8, 2023
@walterddr walterddr mentioned this pull request Mar 9, 2023
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants