Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(batch): distributed tracing for batch queries #10637

Merged
merged 7 commits into from
Jun 29, 2023

Conversation

BugenZhao
Copy link
Member

@BugenZhao BugenZhao commented Jun 28, 2023

Signed-off-by: Bugen Zhao [email protected]I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

Introduce distributed tracing for batch queries.

Will further introduce tracing in the frontend (handler) as the root span in next PRs.

Related: #10000, #10633, #10648.

Preview:

  • Distributed query:
image
  • Local query:
image

Checklist

  • I have written necessary rustdoc comments
  • I have added necessary unit tests and integration tests
  • All checks passed in ./risedev check (or alias, ./risedev c)

Signed-off-by: Bugen Zhao <[email protected]>
Signed-off-by: Bugen Zhao <[email protected]>
@BugenZhao BugenZhao marked this pull request as ready for review June 29, 2023 06:29
@github-actions github-actions bot added the user-facing-changes Contains changes that are visible to users label Jun 29, 2023
@@ -135,6 +136,7 @@ impl<C: BatchTaskContext> InnerSideExecutorBuilder<C> {
}),
}),
epoch: Some(self.epoch.clone()),
tracing_context: TracingContext::from_current_span().to_protobuf(),
Copy link
Contributor

Choose a reason for hiding this comment

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

LocalLookupJoin might cause a lot of tracing spans since it depends on the outer side's data size, but we have auto query mode which could reduce these cases by choosing the DistributedLookupJoin, so LGTM.

Copy link
Contributor

@chenzl25 chenzl25 left a comment

Choose a reason for hiding this comment

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

Will further introduce tracing in the frontend (handler) as the root span in next PRs.

LGTM! For batch queries, it would be great if we have a trace id that can connect all components. We can log the trace id into the query log as well.

@BugenZhao BugenZhao enabled auto-merge June 29, 2023 11:34
@BugenZhao BugenZhao added this pull request to the merge queue Jun 29, 2023
@codecov
Copy link

codecov bot commented Jun 29, 2023

Codecov Report

Merging #10637 (027e679) into main (9fca8fd) will decrease coverage by 0.02%.
The diff coverage is 60.00%.

@@            Coverage Diff             @@
##             main   #10637      +/-   ##
==========================================
- Coverage   70.17%   70.15%   -0.02%     
==========================================
  Files        1280     1280              
  Lines      220033   219965      -68     
==========================================
- Hits       154413   154322      -91     
- Misses      65620    65643      +23     
Flag Coverage Δ
rust 70.15% <60.00%> (-0.02%) ⬇️

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

Impacted Files Coverage Δ
src/batch/src/execution/grpc_exchange.rs 54.34% <0.00%> (-1.21%) ⬇️
src/batch/src/executor/join/local_lookup_join.rs 50.88% <0.00%> (-0.11%) ⬇️
src/batch/src/executor/mod.rs 78.66% <ø> (-0.56%) ⬇️
src/batch/src/rpc/service/task_service.rs 0.00% <0.00%> (ø)
src/common/src/util/tracing.rs 86.95% <0.00%> (-13.05%) ⬇️
src/frontend/src/handler/query.rs 27.50% <0.00%> (ø)
src/frontend/src/scheduler/local.rs 0.00% <0.00%> (ø)
src/rpc_client/src/compute_client.rs 56.02% <0.00%> (-0.41%) ⬇️
src/batch/src/executor/managed.rs 40.00% <40.00%> (ø)
src/batch/src/task/task_manager.rs 76.16% <95.65%> (-1.38%) ⬇️
... and 3 more

... and 6 files with indirect coverage changes

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

@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 29, 2023
@BugenZhao BugenZhao added this pull request to the merge queue Jun 29, 2023
@BugenZhao BugenZhao removed this pull request from the merge queue due to a manual request Jun 29, 2023
@BugenZhao BugenZhao enabled auto-merge June 29, 2023 12:44
@BugenZhao BugenZhao added this pull request to the merge queue Jun 29, 2023
Merged via the queue into main with commit ca2225f Jun 29, 2023
@BugenZhao BugenZhao deleted the bz/batch-distributed-tracing branch June 29, 2023 13:09
@CharlieSYH CharlieSYH added the 📖✗ No user documentation is needed. label Jul 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/feature user-facing-changes Contains changes that are visible to users 📖✗ No user documentation is needed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants