Skip to content

Commit

Permalink
Ensure AST and paser errors are recorded within Status objects for mo…
Browse files Browse the repository at this point in the history
…re detailed error output (#1696)

Summary: Ensure AST and parser errors are recorded within Status objects
for more detailed error output

When working with the standalone PEM, there are often pxl constructs
that aren't supported. Running a script that contains these elements
results in compilation failures that contain empty Status messages (as
seen below). The lack of compilation error details makes debugging very
difficult.

```
# Run the standalone PEM followed by the client from src/api/go/pxapi/examples/standalone_pem_example/BUILD.bazel in pixie#1444

$ PX_HOST_ID=testing bazel run  -c dbg  src/api/go/pxapi/examples/standalone_pem_example:standalone_pem_example
Got error : rpc error: code = InvalidArgument desc = Failed to compile script, while streaming
Execution Time: 0s
Bytes received: 0

# See that the standalone PEM logs don't contain any errors. Even if the error was logged the msg is often blank.

I20230906 15:28:52.238765 4180046 vizier_server.h:63] Executing Script
I20230906 15:28:52.239073 4180046 vizier_server.h:124] Compiling and running query
I20230906 15:29:00.377365 4180047 perf_profile_connector.cc:427] PerfProfileConnector statistics: kBPFMapSwitchoverEvent=1 kCumulativeSumOfAllStackTraces=48935 kLossHistoEvent=0

```

Relevant Issues: N/A

Type of change: /kind bug

Test Plan: Verified with additional unit tests and with a variety of
complex pxl scripts
- [x] New tests pass
- [x] Verified that the standalone pem surfaces the lack of plugin
support when running the http metrics Otel script
```
I20230906 15:23:52.953277 4174704 vizier_server.h:63] Executing Script
I20230906 15:23:52.953543 4174704 vizier_server.h:124] Compiling and running query
W20230906 15:23:53.004771 4174704 vizier_server.h:131] Failed to compile script with error='No plugin config found. Make sure the script is run in a plugin context.'
```
- [x] Verified that the standalone pem surfaces the missing tables
(bpftrace and tcp_stats_table)
```
# Standalone PEM logs
W20230906 15:21:51.754878 4173793 vizier_server.h:131] Failed to compile script with error='Table 'tcp_stats_events' not found.'
I20230906 15:21:51.704820 4173793 vizier_server.h:63] Executing Script
I20230906 15:21:51.705090 4173793 vizier_server.h:124] Compiling and running query

[ .. ]

I20230906 15:22:17.746985 4174704 vizier_server.h:63] Executing Script
I20230906 15:22:17.747223 4174704 vizier_server.h:124] Compiling and running query
W20230906 15:22:17.797647 4174704 vizier_server.h:131] Failed to compile script with error='Table 'exec_table' not found.'

# standalone PEM client logs
Got error : rpc error: code = InvalidArgument desc = Failed to compile script with error='Table 'exec_table' not found.', while streaming
Execution Time: 0s
Bytes received: 0

```

---------

Signed-off-by: Dom Del Nano <[email protected]>
  • Loading branch information
ddelnano authored Sep 7, 2023
1 parent dfb7ef4 commit 2f674ad
Show file tree
Hide file tree
Showing 6 changed files with 31 additions and 15 deletions.
9 changes: 4 additions & 5 deletions src/carnot/planner/compiler/test_utils.h
Original file line number Diff line number Diff line change
Expand Up @@ -353,13 +353,12 @@ struct CompilerErrorMatcher {
(*listener) << "Status is ok, no compiler error found.";
return false;
}
if (!status.has_context() && !status.msg().empty()) {
(*listener) << "Status does not have a context.";
if (status.msg().empty()) {
(*listener) << "Status does not have a message.";
return false;
}
if (!status.has_context() && status.msg().empty()) {
(*listener) << absl::Substitute("Status does not have a context, but has a message: '$0'",
status.msg());
if (!status.has_context()) {
(*listener) << "Status does not have a context.";
return false;
}
if (!status.context()->Is<compilerpb::CompilerErrorGroup>()) {
Expand Down
11 changes: 6 additions & 5 deletions src/carnot/planner/ir/ast_utils.h
Original file line number Diff line number Diff line change
Expand Up @@ -48,9 +48,9 @@ namespace planner {
*/
template <typename... Args>
Status CreateAstError(const pypa::AstPtr& ast, Args... args) {
compilerpb::CompilerErrorGroup context =
LineColErrorPb(ast->line, ast->column, absl::Substitute(args...));
return Status(statuspb::INVALID_ARGUMENT, "",
auto msg = absl::Substitute(args...);
compilerpb::CompilerErrorGroup context = LineColErrorPb(ast->line, ast->column, msg);
return Status(statuspb::INVALID_ARGUMENT, msg,
std::make_unique<compilerpb::CompilerErrorGroup>(context));
}

Expand Down Expand Up @@ -116,9 +116,10 @@ Status AddOuterContextToError(Status status, const pypa::AstPtr& ast, Args... ar
}
compilerpb::CompilerErrorGroup error_group;
CHECK(status.context()->UnpackTo(&error_group));
AddLineColError(&error_group, ast->line, ast->column, absl::Substitute(args...));
auto msg = absl::Substitute(args...);
AddLineColError(&error_group, ast->line, ast->column, msg);

return Status(statuspb::INVALID_ARGUMENT, "",
return Status(statuspb::INVALID_ARGUMENT, msg,
std::make_unique<compilerpb::CompilerErrorGroup>(error_group));
}

Expand Down
11 changes: 11 additions & 0 deletions src/carnot/planner/ir/ast_utils_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,17 @@ INSTANTIATE_TEST_SUITE_P(AstToStringTestSuite, AstToStringTest,
return info.param.name;
});

class CreateAstErrorTest : public ::testing::Test {};

TEST(CreateAstErrorTest, CreateAstErrorBasic) {
Parser parser;
ASSERT_OK_AND_ASSIGN(auto ast, parser.Parse("5 + 2", /* parse_doc_strings */ false));
auto error_msg = "This is a test";
auto ast_error = CreateAstError(ast, error_msg);
ASSERT_NOT_OK(ast_error);
ASSERT_EQ(ast_error.msg(), error_msg);
}

} // namespace planner
} // namespace carnot
} // namespace px
6 changes: 3 additions & 3 deletions src/carnot/planner/ir/ir_node.h
Original file line number Diff line number Diff line change
Expand Up @@ -125,9 +125,9 @@ class IRNode {
*/
template <typename... Args>
Status CreateIRNodeError(Args... args) const {
compilerpb::CompilerErrorGroup context =
LineColErrorPb(line(), col(), absl::Substitute(args...));
return Status(statuspb::INVALID_ARGUMENT, "",
auto msg = absl::Substitute(args...);
compilerpb::CompilerErrorGroup context = LineColErrorPb(line(), col(), msg);
return Status(statuspb::INVALID_ARGUMENT, msg,
std::make_unique<compilerpb::CompilerErrorGroup>(context));
}
/**
Expand Down
4 changes: 3 additions & 1 deletion src/carnot/planner/parser/parser.cc
Original file line number Diff line number Diff line change
Expand Up @@ -52,12 +52,14 @@ class PypaErrorHandler {
*/
Status ProcessErrors() {
compilerpb::CompilerErrorGroup error_group;
std::string msg = "";
for (const auto& err : errs_) {
compilerpb::CompilerError* err_pb = error_group.add_errors();
compilerpb::LineColError* lc_err_pb = err_pb->mutable_line_col_error();
CreateLineColError(lc_err_pb, err);
absl::StrAppend(&msg, err.message);
}
return Status(statuspb::INVALID_ARGUMENT, "",
return Status(statuspb::INVALID_ARGUMENT, msg,
std::make_unique<compilerpb::CompilerErrorGroup>(error_group));
}

Expand Down
5 changes: 4 additions & 1 deletion src/experimental/standalone_pem/vizier_server.h
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,10 @@ class VizierServer final : public api::vizierpb::VizierService::Service {
auto s_or_plan = px::carnot::planner::compiler::Compiler().Compile(reader->query_str(),
compiler_state.get());
if (!s_or_plan.ok()) {
return ::grpc::Status(grpc::StatusCode::INVALID_ARGUMENT, "Failed to compile script");
auto error_msg =
absl::Substitute("Failed to compile script with error='$0'", s_or_plan.msg());
LOG(WARNING) << error_msg;
return ::grpc::Status(grpc::StatusCode::INVALID_ARGUMENT, error_msg);
}
auto plan = s_or_plan.ConsumeValueOrDie();
for (auto f : plan.nodes()) {
Expand Down

0 comments on commit 2f674ad

Please sign in to comment.