Skip to content
Closed
Show file tree
Hide file tree
Changes from 47 commits
Commits
Show all changes
50 commits
Select commit Hold shift + click to select a range
3f24850
Added low-level file I/O spans
joosthooz Feb 13, 2023
d106c42
Added spans to datasetwriter for popstagedbatches and the actual IO w…
joosthooz Feb 13, 2023
8aba916
Added a span for writing parquet.
joosthooz Feb 13, 2023
f9da7ad
Removed ReadBatch spans because they were orphans and didn't represen…
joosthooz Feb 13, 2023
10a80d6
File I/O spans now have actual read bytes as property
joosthooz Feb 16, 2023
ab34c08
Update span name
joosthooz Feb 16, 2023
4e48581
Added spans to ipc compression functions because they are called by a…
joosthooz Feb 16, 2023
1a1307a
Added a span to a parallelFor lambda in parquet: GetRecordBatchReader…
joosthooz Feb 16, 2023
56b8535
Creating a single span for a projection instead of 1 for each expression
joosthooz Feb 16, 2023
56c52af
Added span type attributes to task submission spans.
joosthooz Feb 16, 2023
815d317
No longer creating a span when task submission is queued (throttled)
joosthooz Feb 16, 2023
481bbe4
Creating a string_view from the task name basic_string_view
joosthooz Feb 16, 2023
1f3adee
Added the size of the output to some spans.
joosthooz Feb 17, 2023
43a7fba
Added (presumably missing) START_SPAN to OrderBySinkNode
joosthooz Feb 17, 2023
af56a27
Removed unintended line
joosthooz Feb 27, 2023
24b5ff8
Do not create toplevel task span at task submission
joosthooz Feb 27, 2023
37e84e4
Removed arrow::dataset::CsvFileFormat::ScanBatchesAsync::Next span in…
joosthooz Feb 28, 2023
ed397fc
Added the size of the batch outputted by CSV reader to the span.
joosthooz Feb 28, 2023
2f7fa97
Added span for processing the first block.
joosthooz Mar 1, 2023
38c4639
Creating explicit ProcessMorsel and DatasetScan spans
joosthooz Mar 1, 2023
8764d34
Added span for keeping track of re-chunking during scanning
joosthooz Mar 2, 2023
58c9db4
Reverting back to PeekAndMakeAsync,
joosthooz Mar 2, 2023
ee28b21
Posting datasetwriter backpressure events to the asyncscheduler span
joosthooz Mar 2, 2023
1163ef1
Added push and pop spans to the datasetwriter that allow tracking sta…
joosthooz Mar 2, 2023
07343d7
Formatting
joosthooz Mar 2, 2023
a3a126d
Fixed typo in span attribute
joosthooz Mar 17, 2023
e1a0a0f
Added a surrogate arrow::csv::ReadNextAsync span for the first block …
joosthooz Mar 17, 2023
6a9b635
WIP writing some more extensive documentation about tracing Acero
joosthooz Mar 17, 2023
ecd80c4
Merge remote-tracking branch 'origin/main' into GH-33880-improve-trac…
joosthooz Apr 6, 2023
a28a1db
Added helper function ATTRIBUTE_ON_CURRENT_SPAN
joosthooz Jun 2, 2023
a665bb6
Merge remote-tracking branch 'origin/main' into GH-33880-improve-tracing
joosthooz Jun 2, 2023
2129678
Reverted commenting out task submission tracing
joosthooz Jun 5, 2023
06bcc1e
Fix typo
joosthooz Jun 5, 2023
c8fa9f6
Using helper function in some more places
joosthooz Jun 5, 2023
d315264
Removed some unused code from the datasetwriter
joosthooz Jul 11, 2023
407bcb2
Propagating span context to SimpleTask submission functions
joosthooz Jul 11, 2023
ccdf2ec
Removed TODO, some typos
joosthooz Jul 12, 2023
45f03fa
Formatting
joosthooz Jul 12, 2023
4e0d497
Merge branch 'main' into GH-33880-improve-tracing
joosthooz Jul 13, 2023
42fc7b6
Added missing empty macro for ATTRIBUTE_ON_CURRENT_SPAN()
joosthooz Jul 13, 2023
b64ca22
Revert "Propagating span context to SimpleTask submission functions"
joosthooz Jul 11, 2023
898b093
Propagating parent span through task submission
joosthooz Jul 14, 2023
78ea497
Increased queue size for tracing to avoid dropping spans
joosthooz Jul 14, 2023
4028cd6
Removed task submission trace functions
joosthooz Jul 14, 2023
19522e5
Formatting
joosthooz Jul 14, 2023
b9df0be
Updated some namespace usages
joosthooz Jul 17, 2023
07d5012
Wrote some documentation about the structure of traces
joosthooz Jul 17, 2023
14874ca
Merge remote-tracking branch 'origin/main' into GH-33880-improve-tracing
joosthooz Oct 12, 2023
50e808c
Removed some of the more intrusive Otel spans
joosthooz Oct 12, 2023
fc9c659
Removed the low-level File I/O spans, there's just too many of them a…
joosthooz Oct 12, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions cpp/src/arrow/acero/filter_node.cc
Original file line number Diff line number Diff line change
Expand Up @@ -73,12 +73,12 @@ class FilterNode : public MapNode {
Result<ExecBatch> ProcessBatch(ExecBatch batch) override {
ARROW_ASSIGN_OR_RAISE(Expression simplified_filter,
SimplifyWithGuarantee(filter_, batch.guarantee));

arrow::util::tracing::Span span;
START_COMPUTE_SPAN(span, "Filter",
{{"filter.expression", ToStringExtra()},
{"filter.expression.simplified", simplified_filter.ToString()},
{"filter.length", batch.length}});
{"filter.length", batch.length},
{"input_batch.size_bytes", batch.TotalBufferSize()}});

ARROW_ASSIGN_OR_RAISE(
Datum mask, ExecuteScalarExpression(simplified_filter, batch,
Expand All @@ -87,8 +87,10 @@ class FilterNode : public MapNode {
if (mask.is_scalar()) {
const auto& mask_scalar = mask.scalar_as<BooleanScalar>();
if (mask_scalar.is_valid && mask_scalar.value) {
ATTRIBUTE_ON_CURRENT_SPAN("output_batch.size_bytes", batch.TotalBufferSize());
return batch;
}
ATTRIBUTE_ON_CURRENT_SPAN("output_batch.size_bytes", 0);
return batch.Slice(0, 0);
}

Expand All @@ -101,7 +103,10 @@ class FilterNode : public MapNode {
if (value.is_scalar()) continue;
ARROW_ASSIGN_OR_RAISE(value, Filter(value, mask, FilterOptions::Defaults()));
}
return ExecBatch::Make(std::move(values));
auto filtered_batch = ExecBatch::Make(std::move(values));
ATTRIBUTE_ON_CURRENT_SPAN("output_batch.size_bytes",
filtered_batch->TotalBufferSize());
return filtered_batch;
}

protected:
Expand Down
13 changes: 8 additions & 5 deletions cpp/src/arrow/acero/project_node.cc
Original file line number Diff line number Diff line change
Expand Up @@ -79,19 +79,22 @@ class ProjectNode : public MapNode {

Result<ExecBatch> ProcessBatch(ExecBatch batch) override {
std::vector<Datum> values{exprs_.size()};
arrow::util::tracing::Span span;
START_COMPUTE_SPAN(span, "Project",
{{"project.length", batch.length},
{"input_batch.size_bytes", batch.TotalBufferSize()}});
for (size_t i = 0; i < exprs_.size(); ++i) {
arrow::util::tracing::Span span;
START_COMPUTE_SPAN(span, "Project",
{{"project.type", exprs_[i].type()->ToString()},
{"project.length", batch.length},
{"project.expression", exprs_[i].ToString()}});
std::string project_name = "project[" + std::to_string(i) + "]";
ATTRIBUTE_ON_CURRENT_SPAN(project_name + ".type", exprs_[i].type()->ToString());
ATTRIBUTE_ON_CURRENT_SPAN(project_name + ".expression", exprs_[i].ToString());
ARROW_ASSIGN_OR_RAISE(Expression simplified_expr,
SimplifyWithGuarantee(exprs_[i], batch.guarantee));

ARROW_ASSIGN_OR_RAISE(
values[i], ExecuteScalarExpression(simplified_expr, batch,
plan()->query_context()->exec_context()));
}
ATTRIBUTE_ON_CURRENT_SPAN("output_batch.size_bytes", batch.TotalBufferSize());
return ExecBatch{std::move(values), batch.length};
}

Expand Down
1 change: 1 addition & 0 deletions cpp/src/arrow/acero/sink_node.cc
Original file line number Diff line number Diff line change
Expand Up @@ -539,6 +539,7 @@ struct OrderBySinkNode final : public SinkNode {

Status Finish() override {
arrow::util::tracing::Span span;
START_SPAN(span, std::string(kind_name()) + "::Finish");
ARROW_RETURN_NOT_OK(DoFinish());
return SinkNode::Finish();
}
Expand Down
9 changes: 6 additions & 3 deletions cpp/src/arrow/acero/source_node.cc
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,8 @@ struct SourceNode : ExecNode, public TracedNode {
plan_->query_context()->ScheduleTask(
[this, morsel_length, use_legacy_batching, initial_batch_index, morsel,
has_ordering = !ordering_.is_unordered()]() {
arrow::util::tracing::Span span;
START_SPAN(span, "SourceNode::ProcessMorsel");
int64_t offset = 0;
int batch_index = initial_batch_index;
do {
Expand Down Expand Up @@ -163,6 +165,7 @@ struct SourceNode : ExecNode, public TracedNode {

Status StartProducing() override {
NoteStartProducing(ToStringExtra());

{
// If another exec node encountered an error during its StartProducing call
// it might have already called StopProducing on all of its inputs (including this
Expand All @@ -184,6 +187,9 @@ struct SourceNode : ExecNode, public TracedNode {
options.should_schedule = ShouldSchedule::IfDifferentExecutor;
ARROW_ASSIGN_OR_RAISE(Future<> scan_task, plan_->query_context()->BeginExternalTask(
"SourceNode::DatasetScan"));
arrow::util::tracing::Span span;
START_SPAN(span, "SourceNode::DatasetScan");

if (!scan_task.is_valid()) {
// Plan has already been aborted, no need to start scanning
return Status::OK();
Expand All @@ -195,9 +201,6 @@ struct SourceNode : ExecNode, public TracedNode {
}
lock.unlock();

arrow::util::tracing::Span fetch_batch_span;
auto fetch_batch_scope =
START_SCOPED_SPAN(fetch_batch_span, "SourceNode::ReadBatch");
return generator_().Then(
[this](
const std::optional<ExecBatch>& morsel_or_end) -> Future<ControlFlow<int>> {
Expand Down
2 changes: 1 addition & 1 deletion cpp/src/arrow/compute/function.cc
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,7 @@ struct FunctionExecutorImpl : public FunctionExecutor {
}

Result<Datum> Execute(const std::vector<Datum>& args, int64_t passed_length) override {
util::tracing::Span span;
arrow::util::tracing::Span span;

auto func_kind = func.kind();
const auto& func_name = func.name();
Expand Down
75 changes: 72 additions & 3 deletions cpp/src/arrow/csv/reader.cc
Original file line number Diff line number Diff line change
Expand Up @@ -43,12 +43,14 @@
#include "arrow/type.h"
#include "arrow/type_fwd.h"
#include "arrow/util/async_generator.h"
#include "arrow/util/byte_size.h"
#include "arrow/util/future.h"
#include "arrow/util/iterator.h"
#include "arrow/util/logging.h"
#include "arrow/util/macros.h"
#include "arrow/util/task_group.h"
#include "arrow/util/thread_pool.h"
#include "arrow/util/tracing_internal.h"
#include "arrow/util/utf8_internal.h"
#include "arrow/util/vector.h"

Expand Down Expand Up @@ -401,6 +403,8 @@ class BlockParsingOperator {
num_rows_seen_(first_row) {}

Result<ParsedBlock> operator()(const CSVBlock& block) {
util::tracing::Span span;
START_SPAN(span, "arrow::csv::BlockParsingOperator");
constexpr int32_t max_num_rows = std::numeric_limits<int32_t>::max();
auto parser = std::make_shared<BlockParser>(
io_context_.pool(), parse_options_, num_csv_cols_, num_rows_seen_, max_num_rows);
Expand Down Expand Up @@ -431,6 +435,7 @@ class BlockParsingOperator {
num_rows_seen_ += parser->total_num_rows();
}
RETURN_NOT_OK(block.consume_bytes(parsed_size));
ATTRIBUTE_ON_CURRENT_SPAN("parsed_size", parsed_size);
return ParsedBlock{std::move(parser), block.block_index,
static_cast<int64_t>(parsed_size) + block.bytes_skipped};
}
Expand All @@ -448,6 +453,8 @@ class BlockParsingOperator {
class BlockDecodingOperator {
public:
Future<DecodedBlock> operator()(const ParsedBlock& block) {
util::tracing::Span span;
START_SPAN(span, "arrow::csv::BlockDecodingOperator");
DCHECK(!state_->column_decoders.empty());
std::vector<Future<std::shared_ptr<Array>>> decoded_array_futs;
for (auto& decoder : state_->column_decoders) {
Expand All @@ -456,6 +463,23 @@ class BlockDecodingOperator {
auto bytes_parsed_or_skipped = block.bytes_parsed_or_skipped;
auto decoded_arrays_fut = All(std::move(decoded_array_futs));
auto state = state_;
#ifdef ARROW_WITH_OPENTELEMETRY
opentelemetry::nostd::shared_ptr<opentelemetry::trace::Span> raw_span =
::arrow::internal::tracing::UnwrapSpan(span.details.get());
return decoded_arrays_fut.Then(
[state, bytes_parsed_or_skipped, raw_span](
const std::vector<Result<std::shared_ptr<Array>>>& maybe_decoded_arrays)
-> Result<DecodedBlock> {
ARROW_ASSIGN_OR_RAISE(auto decoded_arrays,
arrow::internal::UnwrapOrRaise(maybe_decoded_arrays));

ARROW_ASSIGN_OR_RAISE(auto batch,
state->DecodedArraysToBatch(std::move(decoded_arrays)));
raw_span->SetAttribute("arrow.csv.output_batch_size_bytes",
util::TotalBufferSize(*batch));
return DecodedBlock{std::move(batch), bytes_parsed_or_skipped};
});

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think there is too much duplicated code in these alternate paths. Is there any way we can just pass span to the callback's capture list and use ATTRIBUTE_ON_CURRENT_SPAN? Also, what happened to END_SPAN_ON_FUTURE_COMPLETION?

#else
return decoded_arrays_fut.Then(
[state, bytes_parsed_or_skipped](
const std::vector<Result<std::shared_ptr<Array>>>& maybe_decoded_arrays)
Expand All @@ -467,6 +491,7 @@ class BlockDecodingOperator {
state->DecodedArraysToBatch(std::move(decoded_arrays)));
return DecodedBlock{std::move(batch), bytes_parsed_or_skipped};
});
#endif
}

static Result<BlockDecodingOperator> Make(io::IOContext io_context,
Expand Down Expand Up @@ -881,7 +906,27 @@ class StreamingReaderImpl : public ReaderMixin,
}

Future<std::shared_ptr<RecordBatch>> ReadNextAsync() override {
return record_batch_gen_();
util::tracing::Span span;
START_SPAN(span, "arrow::csv::ReadNextAsync");
auto future = record_batch_gen_();
#ifdef ARROW_WITH_OPENTELEMETRY
auto longer_living_span = std::make_unique<util::tracing::Span>(std::move(span));
future.AddCallback(
[span = std::move(longer_living_span)](
const arrow::Result<std::shared_ptr<arrow::RecordBatch>>& result) {
opentelemetry::nostd::shared_ptr<opentelemetry::trace::Span> raw_span =
::arrow::internal::tracing::UnwrapSpan(span->details.get());
if (result.ok()) {
auto result_batch = result.ValueOrDie();
if (result_batch) {
raw_span->SetAttribute("batch.size_bytes",
::arrow::util::TotalBufferSize(*result_batch));
}
}
END_SPAN((*span));
});
#endif
return future;
}

protected:
Expand All @@ -892,6 +937,14 @@ class StreamingReaderImpl : public ReaderMixin,
return Status::Invalid("Empty CSV file");
}

util::tracing::Span init_span;
START_SPAN(init_span, "arrow::csv::InitAfterFirstBuffer");

// Create a arrow::csv::ReadNextAsync span so that grouping by that name does not
// ignore the work performed for this first block.
util::tracing::Span read_span;
auto scope = START_SCOPED_SPAN(read_span, "arrow::csv::ReadNextAsync");

std::shared_ptr<Buffer> after_header;
ARROW_ASSIGN_OR_RAISE(auto header_bytes_consumed,
ProcessHeader(first_buffer, &after_header));
Expand All @@ -911,9 +964,25 @@ class StreamingReaderImpl : public ReaderMixin,
auto rb_gen = MakeMappedGenerator(std::move(parsed_block_gen), std::move(decoder_op));

auto self = shared_from_this();
return rb_gen().Then([self, rb_gen, max_readahead](const DecodedBlock& first_block) {
return self->InitFromBlock(first_block, std::move(rb_gen), max_readahead, 0);
auto init_finished = rb_gen().Then([self, rb_gen, max_readahead
#ifdef ARROW_WITH_OPENTELEMETRY
,
init_span = std::move(init_span),
read_span = std::move(read_span)
#endif

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why is this ifdef block needed? If tracing is disabled then init_span and read_span are an empty unique_ptr. Copying it around should be trivial.

](const DecodedBlock& first_block) {
auto fut = self->InitFromBlock(first_block, std::move(rb_gen), max_readahead, 0);
#ifdef ARROW_WITH_OPENTELEMETRY
opentelemetry::nostd::shared_ptr<opentelemetry::trace::Span> raw_span =
::arrow::internal::tracing::UnwrapSpan(read_span.details.get());
raw_span->SetAttribute("batch.size_bytes",
util::TotalBufferSize(*first_block.record_batch));
#endif

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why not ATTRIBUTE_ON_CURRENT_SPAN?

END_SPAN(read_span);
END_SPAN(init_span);
return fut;
});
return init_finished;
}

Future<> InitFromBlock(const DecodedBlock& block,
Expand Down
8 changes: 8 additions & 0 deletions cpp/src/arrow/dataset/dataset_internal.h
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,10 @@
#include "arrow/scalar.h"
#include "arrow/type.h"
#include "arrow/util/async_generator.h"
#include "arrow/util/byte_size.h"
#include "arrow/util/checked_cast.h"
#include "arrow/util/iterator.h"
#include "arrow/util/tracing_internal.h"

namespace arrow {
namespace dataset {
Expand Down Expand Up @@ -144,6 +146,12 @@ inline RecordBatchGenerator MakeChunkedBatchGenerator(RecordBatchGenerator gen,
[batch_size](const std::shared_ptr<RecordBatch>& batch)
-> ::arrow::AsyncGenerator<std::shared_ptr<::arrow::RecordBatch>> {
const int64_t rows = batch->num_rows();
util::tracing::Span span;
START_SPAN(span, "MakeChunkedBatchGenerator",
{{"target_batch_size_rows", batch_size},
{"batch.size_rows", rows},
{"batch.size_bytes", util::TotalBufferSize(*batch)},
{"output_batches", rows / batch_size + (rows % batch_size != 0)}});
if (rows <= batch_size) {
return ::arrow::MakeVectorGenerator<std::shared_ptr<RecordBatch>>({batch});
}
Expand Down
Loading