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

Add trace support for TableWriter #10910

Closed
wants to merge 1 commit into from

Conversation

duanmeng
Copy link
Collaborator

@duanmeng duanmeng commented Sep 1, 2024

Create a QueryDataWriter in exec::TableWriter if the query trace is enabled,
recording the input vectors batch by batch. Each operator writes its data to the
directory $rootDir/$pipelineId/$driverId/data. The recorded data will be used
to replay the execution of exec::TableWriter, which will be supported in the
follow-up.

Design notes: https://docs.google.com/document/d/1crIIeVz4tWKYQnBoHoxrv2i-4zAML9HSYLps8h5SDrc/edit#heading=h.y6j2ojtr7hm9

Part of #9668

@duanmeng duanmeng requested a review from xiaoxmeng September 1, 2024 13:09
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 1, 2024
Copy link

netlify bot commented Sep 1, 2024

Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit 36074cc
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/66f5069fd1c82900078e17d8

Copy link
Contributor

@xiaoxmeng xiaoxmeng left a comment

Choose a reason for hiding this comment

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

@duanmeng LGTM % minors!

velox/exec/Driver.h Show resolved Hide resolved
velox/exec/Task.h Outdated Show resolved Hide resolved
velox/exec/Operator.h Outdated Show resolved Hide resolved
velox/exec/Operator.h Outdated Show resolved Hide resolved
Copy link
Contributor

@xiaoxmeng xiaoxmeng left a comment

Choose a reason for hiding this comment

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

@duanmeng LGTM. Thanks!

@duanmeng duanmeng force-pushed the trace_writer branch 2 times, most recently from 6cbfc3e to 3ab77ee Compare September 24, 2024 14:24
Copy link
Contributor

@xiaoxmeng xiaoxmeng left a comment

Choose a reason for hiding this comment

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

@duanmeng LGTM % nits. thanks!

velox/exec/Operator.cpp Outdated Show resolved Hide resolved
const auto queryTraceNodes = queryConfig.queryTraceNodeIds();
if (queryTraceNodes.empty()) {
return trace::QueryTraceConfig(queryConfig.queryTraceDir());
LOG(WARNING) << "Query trace is enabled, but no nodes will be traced.";
Copy link
Contributor

Choose a reason for hiding this comment

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

Query metadata trace enabled

velox/exec/Task.cpp Show resolved Hide resolved
return trace::QueryTraceConfig(traceDir);
}
if (queryConfig.queryTraceMaxBytes() == 0) {
LOG(WARNING)
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's just check in tracing path?

@duanmeng duanmeng force-pushed the trace_writer branch 2 times, most recently from 4bedead to c82b5cd Compare September 26, 2024 01:36
@@ -2855,10 +2855,13 @@ std::optional<trace::QueryTraceConfig> Task::maybeMakeTraceConfig() const {
if (!RE2::FullMatch(taskId_, queryConfig.queryTraceTaskRegExp())) {
return std::nullopt;
}
LOG(INFO) << "Query metadata trace enabled in task: " << taskId_;
Copy link
Contributor

@xiaoxmeng xiaoxmeng Sep 26, 2024

Choose a reason for hiding this comment

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

Let's just remove metadata trace only use case? Thanks!

Trace query metadata for task:  <<

velox/exec/Operator.cpp Outdated Show resolved Hide resolved
velox/exec/trace/QueryTraceConfig.cpp Outdated Show resolved Hide resolved
return;
}

if (operatorCtx_->driverCtx()->queryConfig().queryTraceMaxBytes() == 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Check queryTraceMaxBytes first?

@duanmeng duanmeng force-pushed the trace_writer branch 2 times, most recently from a83758d to fd1f5e6 Compare September 26, 2024 06:47

const auto pipelineId = operatorCtx_->driverCtx()->pipelineId;
const auto driverId = operatorCtx_->driverCtx()->driverId;
LOG(INFO) << "Query data trace enabled in operator: " << operatorType()
Copy link
Contributor

Choose a reason for hiding this comment

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

Trace data for operator type: operatorType(), op id: , pipeline, driver: 

@xiaoxmeng xiaoxmeng changed the title Add tracer in TableWriter Add trace support for TableWriter Sep 26, 2024
@facebook-github-bot
Copy link
Contributor

@xiaoxmeng has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@xiaoxmeng merged this pull request in 3fe248f.

Copy link

Conbench analyzed the 1 benchmark run on commit 3fe248f9.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

athmaja-n pushed a commit to athmaja-n/velox that referenced this pull request Jan 10, 2025
Summary:
Create a `QueryDataWriter` in `exec::TableWriter` if the query trace is enabled,
recording the input vectors batch by batch. Each operator writes its data to the
directory `$rootDir/$pipelineId/$driverId/data`. The recorded data will be used
to replay the execution of `exec::TableWriter`, which will be supported in the
follow-up.

Design notes: https://docs.google.com/document/d/1crIIeVz4tWKYQnBoHoxrv2i-4zAML9HSYLps8h5SDrc/edit#heading=h.y6j2ojtr7hm9

Part of facebookincubator#9668

Pull Request resolved: facebookincubator#10910

Reviewed By: pedroerp

Differential Revision: D63444416

Pulled By: xiaoxmeng

fbshipit-source-id: ddd74ff6dd56de7bce31ec536035b32211453364
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants